Skip to content

Return an empty cell rather than nullptr when an encoded cell fails to decode - #680

Closed
sushant-me wants to merge 1 commit into
google:masterfrom
sushant-me:return-empty-cell-on-decode-failure
Closed

sushant-me wants to merge 1 commit into
google:masterfrom
sushant-me:return-empty-cell-on-decode-failure

Conversation

@sushant-me

Copy link
Copy Markdown

Split out of #675, per the request there to separate the changes so the
independent fixes can land without waiting on the rest.

The bug

EncodedS2ShapeIndex::GetCell() returned nullptr when
S2ShapeIndexCell::Decode() failed, and Iterator::cell() is:

inline const S2ShapeIndexCell& EncodedS2ShapeIndex::Iterator::cell() const {
  ABSL_DCHECK(!done());
  return *index_->GetCell(cell_pos_);   // nullptr dereference
}

Iterating an index that contains a corrupt cell therefore dereferenced a null
pointer. The bytes arrive through EncodedS2ShapeIndex::Init(), so this is a
crash on malformed input.

The change

Return a static empty cell rather than nullptr. The contents of such a cell are
meaningless either way; what this restores is the guarantee that traversal does
not crash — the same guarantee the rest of the decode path keeps by returning
false.

Verification

Both inputs in the new test were run before and after:

$ ./encoded_s2shape_index_test --gtest_filter='*MalformedCellDoesNotDereferenceNull*'

before:  Segmentation fault (core dumped)    [exit 139]
after:   [  PASSED  ] 1 test.                [exit 0]

$ ./encoded_s2shape_index_test
after:   [  PASSED  ] 10 tests.

The test asserts the crash directly rather than through a sanitizer, because the
failure mode is a segfault in a normal build.

Not included

This does not touch num_edges allocation. A separate concern in #675 is that a
malformed single-shape cell can make S2ClippedShape::Init allocate
num_edges * 4 bytes from an untrusted count; that needs a sound bound and is
being settled separately, since a bound derived from Decoder::avail() is not
sound (edges are run-length encoded, so their count is independent of the
remaining byte count).

EncodedS2ShapeIndex::GetCell() returned nullptr when S2ShapeIndexCell::Decode()
failed, and Iterator::cell() dereferences the result:

    return *index_->GetCell(cell_pos_);

so iterating an index that contains a corrupt cell dereferenced a null pointer.
The bytes reach this path through Init(), so it is a crash on malformed input
rather than an internal invariant violation.

Return a static empty cell instead. The decoded contents of such a cell are
meaningless either way -- what this restores is the guarantee that traversal
does not crash, which is the contract the rest of the decode path already keeps
by returning false.

Both inputs in the regression test segfaulted (signal 11) before this change and
pass after it:

    encoded_s2shape_index_test --gtest_filter='*MalformedCellDoesNotDereferenceNull*'
      before: Segmentation fault (core dumped)   [exit 139]
      after:  PASSED                             [exit 0]

Full suite on this branch: encoded_s2shape_index_test 10/10.

Split out of google#675 as requested there, so this can be reviewed on its own
without waiting on the num_edges allocation question, which is separate.
auto cell = make_unique<S2ShapeIndexCell>();
Decoder decoder = encoded_cells_.GetDecoder(i);
if (!cell->Decode(num_shape_ids(), &decoder)) {
return nullptr;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I said in the other PR, we should continue to return nullptr and check that at the call site. The bug is the missing null check, not what's returned here.

@sushant-me

Copy link
Copy Markdown
Author

Closing this, because the test I added here showed the patch does not fix the crash it claims to.

Run under ASan at 2d73972, the malformed input in this PR still overflows:

ERROR: AddressSanitizer: heap-buffer-overflow on address ... READ of size 24
    #1 s2coding::EncodedS2PointVector::operator[](int) const   encoded_s2point_vector.h:191
    #2 EncodedS2LaxPolygonShape::chain_edge(int, int) const    s2lax_polygon_shape.h:306
    #3 EncodedS2LaxPolygonShape::edge(int) const               s2lax_polygon_shape.cc:360
    #4 EncodedS2ShapeIndex_MalformedCellDoesNotDereferenceNull_Test::TestBody()
                                                               encoded_s2shape_index_test.cc:861

So the crash this PR's own test exercises is an unvalidated loop_starts array in the encoded lax
polygon shape, not a null cell. Returning an empty cell from GetCell() changes the symptom on one
path and leaves the out-of-bounds read in place — your reading that it "covers up problems" was right.

That also explains the confusing CI result: the test passes on my machine without ASan and SEGFAULTs
in CI on every platform, because an out-of-bounds read only faults when the allocation happens to sit
at the end of a mapped page. I should have run it under ASan before opening this.

The fix for that input is the loop_starts validation, already open as #681. The remaining piece of
#675 — bounding the untrusted counts in S2ShapeIndexCell::Decode (the num_edges overflow, the
num_clipped bound and the shape_id bounds, the parts you marked good) — is coming as its own small
PR. Issue #674 is a genuine null dereference and I am leaving it open for the internal fix you
mentioned rather than shipping this as the fix for it.

@sushant-me sushant-me closed this Sep 19, 2026
@sushant-me
sushant-me deleted the return-empty-cell-on-decode-failure branch September 19, 2026 13:24
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.

2 participants