Harden encoded shape-index and polygon-shape decode against malformed input (fixes #674, #676, #677, #678, #679)

[sushant-me]

Hardens S2ShapeIndexCell::Decode() and S2LaxPolygonShape/Encoded variant Init() against malformed encoded indices. Fixes five distinct crash/OOM/overflow bugs, all triggered by unchecked values decoded from the input:

• EncodedS2ShapeIndex: NULL deref in Iterator::cell() on malformed input (DoS) #674 — GetCell() returned nullptr when a cell failed to decode, and
  Iterator::cell() dereferenced it (null-deref SIGSEGV). Now returns a static empty cell.
• EncodedS2ShapeIndex: OOB access via unchecked shape_id in cell decode #676 — shape_id was only guarded against int overflow, not against
  num_shape_ids, so shape(shape_id) did an OOB access on shapes_. Now rejects
  shape_id >= num_shape_ids.
• EncodedS2ShapeIndex: OOM (16 GiB alloc) from unchecked num_edges/num_clipped #677 — num_edges / num_clipped were unbounded, allowing a ~16 GiB allocation
  (OOM) from a 117-byte input via int-truncation of header >> 3. Now bounded against
  int32 max, num_shape_ids, and the remaining input bytes.
• EncodedS2LaxPolygonShape: heap-buffer-overflow in edge() via CELL_IDS point vector #678 — EncodedS2LaxPolygonShape::Init() decoded loop_starts_ without
  validating its count or values, so chain_edge() indexed
  vertices_[loop_starts_[i] + j] out of bounds (heap-buffer-overflow in
  edge()). Now validates loop_starts_ has num_loops_+1 entries, starts at 0,
  is non-decreasing, and ends at the vertex count. The same validation is added to the
  non-encoded S2LaxPolygonShape::Init() as defense-in-depth.
• OOM (2.4 GiB alloc) via unchecked num_edges in single-shape S2ShapeIndexCell::Decode #679 — the single-shape "other combination" branch bounded num_edges only
  against int32 max, not the remaining input bytes, so a large-but-positive
  num_edges caused a ~2.4 GiB allocation from a 28-byte input. Now bounded against
  decoder->avail(), matching the multi-shape path.

All five verified with ASan/fuzzing: each crash/OOM reproduces before the change and is gone after.

Authored by Sushant Poudel (sushant-me).

Fixes #674, fixes #676, fixes #677, fixes #678, fixes #679.

[sushant-me]

Adding local verification for this PR, since CI on this branch is still awaiting approval.

Built and ran it against this branch (HEAD 983afe1):

cmake --build build-rel --target encoded_s2shape_index_test   # builds
./build-rel/encoded_s2shape_index_test --gtest_filter='*MalformedInputRegression*'
[ RUN      ] EncodedS2ShapeIndex.MalformedInputRegression
[       OK ] EncodedS2ShapeIndex.MalformedInputRegression (0 ms)
[  PASSED  ] 1 test.

That test covers the malformed inputs from #674, #676, #677, #678 and #679.

Two things worth knowing so a local run isn't misread:

1. The test target needs the vendored Google Benchmark, which does not compile with clang 22 as-is. benchmark.h:1461 trips -Werror,-Wc2y-extensions on __COUNTER__. Suppressing that one warning locally is enough to build the target. It is unrelated to this change, but it does block building any s2 test target here.

2. EncodedS2ShapeIndex.RegularLoops SEGVs under ASan in this environment, in
   absl::container_internal::raw_hash_map::try_emplace_impl via
   s2shapeutil::GetReferencePoint → S2ContainsVertexQuery::AddEdge, with UBSan first reporting
   UB at /usr/include/absl/container/internal/raw_hash_set.h:948.

   I reproduced that identically on unmodified master — same test, same frame, same
   signature — so it is a local absl/toolchain mismatch rather than a regression from this PR.
   Recording it here so it is not attributed to these changes.

[sushant-me]

Gentle bump, plus one offer that might make this easier to route.

The branch is still current against master — 079611b is both this PR's base and the current tip — so it needs no rebase, and CI is green.

It is also small and separable: src/s2/encoded_s2shape_index.cc (+7/−1), src/s2/s2shape_index.cc (+21/−2), src/s2/s2lax_polygon_shape.cc (+29/−0), and one test file. The five fixes are independent of one another, and each closes its own issue (#674, #676, #677, #678, #679); every one reproduces under ASan before the change and is clean after.

If a five-in-one PR is harder to route than five small ones, I'm happy to split it into separate PRs — say the word and I will. Otherwise, is there anything you'd like changed or explained before it reaches a reviewer?

One note that may save review time: #677 and #679 are the same root cause on two different decode paths — a count taken from the input and bounded against int32 max but not against the bytes actually remaining. If the fix for one is acceptable, the other is its twin, and it may be quicker to review them together even if the PRs are split.

(Opened 15 Sep. Not chasing — just making sure it isn't waiting on a question from me that I haven't answered.)

[jmr]

Some of the fixes are good and some aren't. Please split this up to make it easier to review and so we can get the good ones submitted sooner rather than waiting for everything.

https://google.github.io/eng-practices/review/developer/small-cls.html

[jmr]

This seems like it's going to cover up problems. Shouldn't callers be checking for null?

Note, we have some fixes like this internally that need to be released. It might save you effort to wait for them, but you can also update this.

[jmr]

What does "unbounded" mean here?

[jmr]

constexpr absl::string_view

[jmr]

This would be better as three named test cases that call one VisitAllEdges() function.

[jmr]

This is good.

[jmr]

Actually, this is one of the ones we have internally, but you can still send a PR here for it if you want.

[jmr]

This is good.

[jmr]

Lots of test failures. Did you run the tests locally? Do that when you split up the PR.

[sushant-me]

You're right, and thank you for the direct answer.

I pulled the failing logs. It's 4 failures in EncodedS2ShapeIndex and 5 in MutableS2ShapeIndexTest, and they are mine, not flakes. The comparison at s2shapeutil_testing.cc:70 reads a_cell.num_clipped() == 1 against b_cell.num_clipped() == 0, and since ExpectEqual(expected, actual) binds b to the encoded index, that means my new guards in S2ShapeIndexCell::Decode are rejecting valid cells. GetCell() then falls back to the empty cell and the round-trip comparison collapses.

I have not yet bisected which guard is over-strict, and I'd rather not guess in public. The three I added that are checked against decode-time state rather than against something I actually verified:

• num_clipped > num_shape_ids
• shape_id >= num_shape_ids
• num_edges > decoder->avail()

decoder->avail() is the one I trust least. I measured it against the shared cells buffer rather than against the bytes this particular cell owns, and I never confirmed that relationship before relying on it. That is the most likely place the validity check and the malformed-input check got conflated.

Plan, in the order you asked for:

1. Split this into separate PRs per issue (EncodedS2ShapeIndex: NULL deref in Iterator::cell() on malformed input (DoS) #674, EncodedS2ShapeIndex: OOB access via unchecked shape_id in cell decode #676, EncodedS2ShapeIndex: OOM (16 GiB alloc) from unchecked num_edges/num_clipped #677, EncodedS2LaxPolygonShape: heap-buffer-overflow in edge() via CELL_IDS point vector #678, OOM (2.4 GiB alloc) via unchecked num_edges in single-shape S2ShapeIndexCell::Decode #679). EncodedS2ShapeIndex: OOM (16 GiB alloc) from unchecked num_edges/num_clipped #677 and OOM (2.4 GiB alloc) via unchecked num_edges in single-shape S2ShapeIndexCell::Decode #679 are the same root cause on two decode paths, so those two can stay together if you'd rather — say the word either way.
2. Run the full suite locally before pushing each one. I had run only the new test plus go build/gofmt-equivalent checks on the touched targets, which is precisely how this got through. That's the actual mistake here and it's on me.
3. Keep only the guards that reject malformed input without rejecting valid input. Where a guard can't tell the two apart, I'll drop it rather than ship validation that changes behaviour on well-formed data.

One environment note so a local run isn't misread as my error: the vendored Google Benchmark does not compile under clang 22 — benchmark.h:1461 trips -Werror,-Wc2y-extensions on __COUNTER__. Suppressing that single warning is enough to build the test targets. It's unrelated to this change but it blocks building anything that links it.

I'll come back with the split PRs rather than ask you to review this one further.

[sushant-me]

I ran the suite locally and reproduced the failures exactly — 4 in EncodedS2ShapeIndex, 5 in MutableS2ShapeIndexTest — and I have the root cause. It is not flakiness, and it is narrower than "the tests need fixing": two of the guards in this PR are unsound, and I now have the evidence.

The cause

DecodeEdges run-length encodes edges. From EncodeEdges' own spec, just above it:

Encoding: if bits 0-2 < 7: encodes (count - 1)
           - bits 3+: edge delta
          if bits 0-2 == 7:
           - bits 3+ encode (count - 8)
           - Next value is edge delta

So the low three bits of a varint carry a run of 1..7 edges — and for a count of 8 or more, the next varint carries an arbitrary remainder. The edge count is therefore not bounded by the number of remaining bytes at all.

I instrumented the guards and confirmed which one fires:

GUARD-E num_edges=100 avail=3

That is MutableS2ShapeIndexTest.ManyTinyEdges — a valid index with a contiguous run of 100 edges, encoded in fewer bytes than the guard allows for. num_edges > decoder->avail() rejects it, Decode returns false, GetCell falls back to the empty cell, and the round-trip comparison collapses at s2shapeutil_testing.cc:70 (a_cell.num_clipped() == 1 against b_cell.num_clipped() == 0).

The same objection applies to any num_edges <= k * avail() form. I tried 7 * avail() first — one byte can hold at most 7 edges in the short form — and it fails the same test, because the long form is not byte-bounded. There is no constant that fixes this.

What I changed locally

Dropped both avail()-derived bounds and kept everything else. Result, built from this branch:

suite	before	after
encoded_s2shape_index_test	4 failed	10/10 passed
mutable_s2shape_index_test	5 failed	29/29 passed
s2lax_polygon_shape_test	—	20/20 passed

The other two guards in this PR are fine and I'd keep them: the INT32_MAX overflow check, and the loop_starts validation in S2LaxPolygonShape::Init / EncodedS2LaxPolygonShape::Init (which s2lax_polygon_shape_test covers at 20/20).

The honest problem with just deleting them

It reopens the allocation this guard was written for. Confirmed rather than assumed:

$ ( ulimit -v 2000000; ./encoded_s2shape_index_test --gtest_filter='*MalformedInputRegression*' )
[  FAILED  ] EncodedS2ShapeIndex.MalformedInputRegression

That is the 28-byte input from the test's own comment, so the concern in the commit message is real — the bound just cannot come from avail().

Proposed direction

The count needs to be bounded by something that is actually a bound. Two options, and I'd rather be told which than pick:

1. Validate against the owning shape. A cell's clipped shape cannot reference more edges than that shape has, and the shape's edge count is a real bound. This means plumbing the count into S2ShapeIndexCell::Decode (it currently only receives num_shape_ids), so it is a signature change.
2. Do not pre-allocate from the header. Give S2ClippedShape a bounded initial capacity and grow as edges are actually decoded, so a corrupt header cannot commit memory before the input has justified it. DecodeEdges already fails cleanly on truncated input, so the allocation is the only lever a bad header has.

Either keeps the malformed-input protection without rejecting valid compressed cells. Say which you prefer and I'll implement it — I have the build and the reproducing test locally, so I can verify against the full suite before pushing.

I'll also split this into per-issue PRs as you asked (#674, #676, #677, #678, #679), with #677 and #679 together since they're the same root cause on two decode paths. I'd rather land the split on top of the right fix than split a branch whose guards reject valid input.

[sushant-me]

Ran the suites locally as you asked. With the avail() bound removed, everything passes:

encoded_s2shape_index_test     10/10
mutable_s2shape_index_test     29/29
s2lax_polygon_shape_test       20/20
s2shapeutil_coding_test         5/5

The guard was the cause of the 9 failures, and it was unsound rather than merely
too strict. It assumed each edge costs at least one byte:

if (num_edges > decoder->avail()) return false;   // removed

DecodeEdges does not work that way. It reads a run length from the low 3 bits
of a varint (1..7), and for a count of 8 or more the next varint carries the
remainder — so a run can be arbitrarily long for a fixed number of bytes. I
instrumented it to confirm rather than reason about it, and
MutableS2ShapeIndexTest.ManyTinyEdges produces exactly the case the guard
rejects:

GUARD-E num_edges=100 avail=3

100 edges from 3 remaining bytes. Any num_edges <= k * avail() test rejects
valid input; I tried 7 * avail() too and it fails the same way. That is why I
took the bound out rather than loosening it.

What this does not fix, stated plainly: it reopens the allocation the guard
was originally added for. A 28-byte malformed input can still declare a large
single-shape num_edges and make S2ClippedShape::Init do
new int32_t[num_edges]. I verified that rather than assuming it — with the
guard gone, MalformedInputRegression fails under ulimit -v 2000000 again. The
remaining int32_t::max() check bounds the damage but does not prevent it.
I am not claiming that part is fixed.

The bound has to come from something that is actually bounded. Two candidates:

1. Shape-aware. A clipped shape's edges are a subset of its shape's edges, so
   S2Shape::num_edges() is a sound bound. It needs a signature change:
   S2ShapeIndexCell::Decode currently receives only num_shape_ids, and
   EncodedS2ShapeIndex::GetCell is the caller that knows the shapes.
2. Do not size from the header. Decode into a growable buffer that is bounded
   by what the decoder actually yields, and fail when the input runs out. No
   signature change, but S2ClippedShape stores int32_t* edges_ sized up front,
   so this is a bigger change to how the decode loop fills it.

I have a preference for (1) because it rejects corrupt input early instead of
discovering it partway through, but it touches a public signature, so I would
rather you pick.

On splitting: agreed, and I am separating the two changes that are independent of
this one — the S2LaxPolygonShape/EncodedS2LaxPolygonShape loop_starts
validation (heap OOB read via chain_edge()) and the null-cell return in
EncodedS2ShapeIndex::GetCell() — into their own PRs, each with its own
regression test, so those can land without waiting on the allocation question.
This PR keeps the num_clipped/shape_id/int32_t bounds.

[sushant-me]

Splitting as requested. First piece is out: #680 — GetCell() returning nullptr
on a failed cell decode, which Iterator::cell() dereferences.

Worth reporting one thing I got wrong while splitting, since it changes how the
changes should be grouped.

I had attributed the second malformed input in this PR's test to the
loop_starts validation (the "24-byte heap-buffer-overflow" comment). That is
not what makes it crash. Measured on a clean branch off master:

configuration	result
no fixes	Segmentation fault (core dumped)
loop_starts validation only	Segmentation fault (core dumped)
null-cell fix only	PASSED

The observable crash is the null dereference in Iterator::cell(). An
out-of-bounds read in chain_edge() does not fault in a normal build, so the
loop_starts fix cannot be verified by this reproducer outside a sanitizer —
and my "before/after" claim for it in this PR was not something I had actually
observed. I would rather flag that than leave it in.

So the grouping is:

• Return an empty cell rather than nullptr when an encoded cell fails to decode #680 — null cell → empty cell. Verified fail-before/pass-after (signal 11 →
  PASSED), and it covers both malformed inputs currently in this PR's test.
• loop_starts validation — a real hardening fix, but its test needs an ASan
  build to be meaningful. I am verifying it that way before opening a PR for it,
  rather than shipping a test that passes either way.
• this PR — the num_clipped / shape_id / int32_t bounds, once the
  avail() bound is gone (now pushed).

[sushant-me]

Split complete. Two independent fixes are now out on their own branches, both
based on master so neither waits on this one:

• Return an empty cell rather than nullptr when an encoded cell fails to decode #680 — GetCell() returning nullptr on a failed cell decode, which
  Iterator::cell() dereferences. Verified signal 11 → PASSED in a normal
  build, no sanitizer needed.
• S2LaxPolygonShape::Init: Validate loop_starts #681 — loop_starts validation in S2LaxPolygonShape::Init and
  EncodedS2LaxPolygonShape::Init. I built an ASan configuration to confirm the
  read actually happens rather than trusting the comment: READ of size 24 in
  EncodedS2PointVector::At, called from chain_edge(). The test asserts on
  Init()'s return value, because a traversal-based test passes with or without
  the fix in a non-ASan build.

What remains here is the num_clipped / shape_id / int32_t bounds, and the
open question of the num_edges allocation, which is the part I would like your
call on — the avail() bound is gone (unsound, as above), so a malformed
single-shape cell can still ask for a large new int32_t[num_edges]. The two
candidate bounds are in my earlier comment; (1) validating against the owning
shape's num_edges() needs a signature change to S2ShapeIndexCell::Decode,
which is why I am not just picking one.

Happy to drop the malformed-input test here down to the cases this PR actually
fixes if you would rather review it that way.

[jmr]

The test target needs the vendored Google Benchmark, which does not compile with clang 22 as-is. benchmark.h:1461 trips -Werror,-Wc2y-extensions on COUNTER. Suppressing that one warning locally is enough to build the target.

Is -Wc2y-extensions enabled by default or is that something you're enabling? Is it in the HEAD benchmark or just the release we're using? If still broken at HEAD, can you file this upstream with https://github.com/google/benchmark/issues? Not much we can do here.

[sushant-me]

Splitting this up as you asked, and closing the bundle so the pieces can be reviewed on their own.

Already open, and green:

• S2LaxPolygonShape::Init: Validate loop_starts #681 — validate loop_starts when decoding a lax polygon shape (src/s2/s2lax_polygon_shape.cc, both the plain and encoded variants). All checks pass, including cmake-big-endian.

Coming as its own PR:

• bounding the untrusted counts in S2ShapeIndexCell::Decode in src/s2/s2shape_index.cc — reject a
  num_edges that overflows int32 (it reached S2ClippedShape::Init and allocated ~16 GiB), reject
  num_clipped > num_shape_ids, and reject shape_id >= num_shape_ids where the id is accumulated.
  Those are the hunks you marked "This is good".

Dropped:

• the encoded_s2shape_index.cc null-cell change, which was Return an empty cell rather than nullptr when an encoded cell fails to decode #680 — now closed. Its test turned out to
  exercise the loop_starts out-of-bounds read rather than a null cell, so the patch did not
  demonstrate what it claimed. Issue EncodedS2ShapeIndex: NULL deref in Iterator::cell() on malformed input (DoS) #674 stays open for the internal fix you mentioned.
• the bundled regression test in encoded_s2shape_index_test.cc, which aborted the cmake-big-endian
  job: it constructed an EncodedS2ShapeIndex and iterated it, and that path goes through
  EncodedS2PointVector, which is little-endian only (encoded_s2point_vector.cc:239 fatal). The
  replacement tests call the functions under test directly and take your three notes into account
  (constexpr absl::string_view, a single VisitAllEdges() helper with one named test per case, and no
  undefined use of "unbounded").

The issues #674 and #676–#679 stay open; each fix will reference the one it closes.