Skip to content

EncodedS2ShapeIndex: NULL deref in Iterator::cell() on malformed input (DoS) #674

Description

@sushant-me

NULL pointer dereference in EncodedS2ShapeIndex::Iterator::cell() on malformed input

Decoding a malformed EncodedS2ShapeIndex crashes with SIGSEGV (NULL deref), even though Init() returns success.

Root cause

Cells are decoded lazily. When a cell's contents are malformed, S2ShapeIndexCell::Decode() returns false, so GetCell() returns nullptr:

// src/s2/encoded_s2shape_index.cc
auto cell = make_unique<S2ShapeIndexCell>();
Decoder decoder = encoded_cells_.GetDecoder(i);
if (!cell->Decode(num_shape_ids(), &decoder)) {
  return nullptr;   // decode failure => null
}

Iterator::cell() unconditionally dereferences it:

// src/s2/encoded_s2shape_index.h:316-318
inline const S2ShapeIndexCell& EncodedS2ShapeIndex::Iterator::cell() const {
  ABSL_DCHECK(!done());
  return *index_->GetCell(cell_pos_);   // NULL deref when GetCell()==nullptr
}

Reproduction

Build libs2 (release, ASan) and run the documented traversal on the 149-byte
input below. Init() returns true, then it.cell() dereferences a null pointer.

Decoder decoder(data, size);
EncodedS2ShapeIndex index;
index.Init(&decoder, s2shapeutil::LazyDecodeShapeFactory(&decoder)); // true
for (EncodedS2ShapeIndex::Iterator it(&index, S2ShapeIndex::BEGIN);
     !it.done(); it.Next()) {
  const S2ShapeIndexCell& cell = it.cell();   // SEGV
  (void)cell.num_clipped();
}

Crash input (base64):

EDmHAykACDQQEAABDI5AxsXB7z+Jcwt+P8I6GgK2gbjWT7Y/l7YwB2iR7z9c3IQEEr/BPwKBwrjWT7a/AgED/wIAYbRsOgOd7T/i3IKfho7VP4lzC34aOsY/CRFj46865T//hOpwPtDhP////////98/KDCol30D7D//8r7xPSDaP5AGk8F9kNA/KOAIEAgFEwACALg=

ASan (release -DNDEBUG, -fsanitize=address):

ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000
  #0 gtl::compact_array_base<S2ClippedShape>::size() const  compact_array.h:221
  #1 S2ShapeIndexCell::num_clipped() const                  s2shape_index.h:139
  #2 main                                                  s2_repro.cc:21

Suggested fix

GetCell() must not return a pointer that callers blindly dereference. Either
null-check in Iterator::cell() and other GetCell() callers, or make Init()
validate every cell's contents so malformed input is rejected with false up
front (preserving the "lazy decode is infallible" invariant).

(Note: a separate ABSL_CHECK(IsValid()) abort in S2Polyline::DecodeUncompressed()
is debug-only via FLAGS_s2debug and is not part of this report.)

Activity

  1. jmr commented on Sep 18, 2026

    @jmr
    Member

    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.

  2. sushant-me commented on Oct 5, 2026

    @sushant-me
    Author

    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.h says nothing about the assumption today. EncodedS2ShapeIndex::Init returns true for 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 on Init — what it validates, what it assumes, and that a false return 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:

    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.

  3. sushant-me commented on Oct 5, 2026

    @sushant-me
    Author

    Written up as #686 — a note on EncodedS2ShapeIndex::Init stating that a true return 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.h already uses for its own Init, and names the actual failure rather than gesturing at it: S2ShapeIndexCell::Decode() returning false leaves GetCell() null, and Iterator::cell() dereferences it (encoded_s2shape_index.h:328). Callers that cannot assume a trusted stream are told to validate before Init().

    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.

  4. sushant-me commented on Oct 5, 2026

    @sushant-me
    Author

    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 (the loop_starts validation, 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.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions