Repository navigation
EncodedS2ShapeIndex: NULL deref in Iterator::cell() on malformed input (DoS) #674
Description
Activity
- added a commit that references this issue
on Sep 16, 2026 Thanks for these reports. It's not really a DoS since these are assumed to only operate on trusted data. We should document the assumptions better. We already have fixes for some of these internally. (Everyone must have LLMs sniffing for the same things.) I'll look at the PR.
Taking the trusted-data point — you are right that "DoS" overstates it when the input is assumed trusted, and I'll stop framing the four as availability bugs.
On documentation: I checked, and
encoded_s2shape_index.hsays nothing about the assumption today.EncodedS2ShapeIndex::Initreturnstruefor a decoding that produced a cell which then fails to decode lazily, so a reader has no way to learn from the header that malformed input is out of contract rather than merely unhandled. A short paragraph onInit— what it validates, what it assumes, and that afalsereturn is the only failure it reports — would cover the class rather than each symptom. Happy to write that if it is the placement you want, since it is the part of these reports that is not a code change.On the PRs, both are still open and both report
mergeable_state: clean:- S2LaxPolygonShape::Init: Validate loop_starts #681 (
S2LaxPolygonShape::Init: Validate loop_starts) — +73,s2lax_polygon_shape.ccand its test. The offset checkchain_edge()needs. - S2ShapeIndexCell::Decode: Bound untrusted counts #682 (
S2ShapeIndexCell::Decode: Bound untrusted counts) — +88/-2,s2shape_index.ccand its test. The three unchecked counts.
Both were split out of #675 at your request so the agreed parts could land separately, and neither depends on the other. The "return an empty cell instead of nullptr" change you did not want is excluded — if #682 lands, the null dereference in #674 stops being reachable, but I am not treating that as the fix for it.
If the internal fixes you mentioned already cover either file, say so and I will withdraw that one rather than leave it sitting.
- S2LaxPolygonShape::Init: Validate loop_starts #681 (
Written up as #686 — a note on
EncodedS2ShapeIndex::Initstating that atruereturn does not mean the encoding is sound, because cells are decoded lazily and a malformed one is not reached until later. Comment-only, +12 lines, no behaviour change.It follows the phrasing
encoded_s2point_vector.halready uses for its ownInit, and names the actual failure rather than gesturing at it:S2ShapeIndexCell::Decode()returning false leavesGetCell()null, andIterator::cell()dereferences it (encoded_s2shape_index.h:328). Callers that cannot assume a trusted stream are told to validate beforeInit().Cross-referenced on #676, #677 and #679 so the five read as one contract gap rather than five separate crashes. #681 and #682 remain the code halves, both clean.
Status check on this one, since its direct fix is not the one that is still open.
The null dereference is fixed by #680 (
Return an empty cell rather than nullptr when an encoded cell fails to decode), and #680 is closed, not merged. So this report is currently unaddressed by anything live — the two PRs still open against the family are #681 (theloop_startsvalidation, which is #678's out-of-bounds read) and #682 (the bounded counts, which are #676, #677 and #679).I left the empty-cell change out of #682 on purpose, because the two are independent and bundling them was what made #675 too large to land. If you would rather have it folded into #682 than re-opened separately, say so and I will do that instead of re-submitting #680 as-is.
Also updated #681 and #682 to name the issues they actually close — they previously referenced only #675, so neither would have auto-closed anything.
NULL pointer dereference in
EncodedS2ShapeIndex::Iterator::cell()on malformed inputDecoding a malformed
EncodedS2ShapeIndexcrashes with SIGSEGV (NULL deref), even thoughInit()returns success.Root cause
Cells are decoded lazily. When a cell's contents are malformed,
S2ShapeIndexCell::Decode()returnsfalse, soGetCell()returnsnullptr:Iterator::cell()unconditionally dereferences it:Reproduction
Build
libs2(release, ASan) and run the documented traversal on the 149-byteinput below.
Init()returnstrue, thenit.cell()dereferences a null pointer.Crash input (base64):
ASan (release
-DNDEBUG,-fsanitize=address):Suggested fix
GetCell()must not return a pointer that callers blindly dereference. Eithernull-check in
Iterator::cell()and otherGetCell()callers, or makeInit()validate every cell's contents so malformed input is rejected with
falseupfront (preserving the "lazy decode is infallible" invariant).
(Note: a separate
ABSL_CHECK(IsValid())abort inS2Polyline::DecodeUncompressed()is debug-only via
FLAGS_s2debugand is not part of this report.)