Skip to content

S2Polyline: validate vertex count before updating state in decode - #711

Open
sayed0200 wants to merge 1 commit into
google:masterfrom
sayed0200:s2polyline-decode-state
Open

sayed0200 wants to merge 1 commit into
google:masterfrom
sayed0200:s2polyline-decode-state

Conversation

@sayed0200

Copy link
Copy Markdown

S2Polyline::DecodeUncompressed reads the vertex count from the encoded header and writes it straight into num_vertices_, and only afterwards checks whether the decoder still holds that many S2Points. On a truncated or crafted encoding that check fails and Decode() returns false, but the polyline is now left claiming N vertices while vertices_ still points at whatever buffer it had before, so a later vertex(), ApproxEquals(), Encode() or copy construction reads past that allocation. I hit it by cutting an uncompressed encoding one vertex short and decoding it into a two-vertex polyline: num_vertices() came back as 4 and AddressSanitizer reported a heap-buffer-overflow read 24 bytes past the 48-byte vertex buffer. The same check also multiplies the count by sizeof(S2Point) in size_t, which wraps on 32-bit targets and can admit a count whose allocation is far smaller than num_vertices_. S2Loop::DecodeInternal already does all of its checks before touching vertex state, and S2Polyline::DecodeCompressed decodes into a local vector, so the uncompressed path was the odd one out. The fix reads the count into a local, compares it against avail() / sizeof(S2Point), and only then updates num_vertices_ and allocates; a regression test covers the truncated case and fails before this change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant