Skip to content

S2LaxPolygonShape::Init: Validate loop_starts - #681

Open
sushant-me wants to merge 2 commits into
google:masterfrom
sushant-me:harden-lax-polygon-loop-starts
Open

sushant-me wants to merge 2 commits into
google:masterfrom
sushant-me:harden-lax-polygon-loop-starts

Conversation

@sushant-me

Copy link
Copy Markdown

Split out of #675, per the request there. Independent of #680.

The bug

chain_edge() computes vertices_[loop_starts_[i] + j], and neither
S2LaxPolygonShape::Init nor EncodedS2LaxPolygonShape::Init validated the
offsets they had just decoded. An encoding whose loop_starts are inconsistent
with the vertex array therefore reads outside it.

Confirmed with AddressSanitizer rather than inferred — the read is real but does
not fault in a normal build:

ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 24
  #0 EncodedS2PointVector::At               src/s2/encoded_s2point_vector.h:166
  #1 EncodedS2PointVector::operator[]       src/s2/encoded_s2point_vector.h:183
  #2 EncodedS2LaxPolygonShape::chain_edge   src/s2/s2lax_polygon_shape.h:306
  #3 EncodedS2LaxPolygonShape::edge         src/s2/s2lax_polygon_shape.cc:360

The change

Require the offsets to start at 0, be non-decreasing, and end at the vertex
count, in both the eager and the lazy decode path.

Verification

The test builds a valid two-loop shape, encodes it, and then flips only the
"starts at 0" rule — leaving the offsets non-decreasing and ending at the vertex
count, so a check covering only monotonicity or only the final offset still
passes it. It asserts the layout of the tail before corrupting it, so a change
to the encoding fails loudly instead of silently patching the wrong byte.

$ ./s2lax_polygon_shape_test --gtest_filter='*RejectsInconsistentLoopStarts*'

before:  [  FAILED  ] 1 test.
after:   [  PASSED  ] 1 test.

Asserting on Init()'s return value rather than on a traversal is deliberate:
an out-of-bounds read does not fault without a sanitizer, so a test that only
walked the shape would pass both before and after.

Suites on this branch: s2lax_polygon_shape_test 21/21,
encoded_s2shape_index_test 9/9, mutable_s2shape_index_test 29/29.

Both S2LaxPolygonShape::Init and EncodedS2LaxPolygonShape::Init decoded
`loop_starts` without checking it, and `chain_edge()` indexes
`vertices_[loop_starts_[i] + j]` -- so offsets that are inconsistent with the
vertex array read outside it.

Measured under AddressSanitizer, against a malformed encoded index:

    ERROR: AddressSanitizer: heap-buffer-overflow
    READ of size 24
      #0 EncodedS2PointVector::At               encoded_s2point_vector.h:166
      google#2 EncodedS2LaxPolygonShape::chain_edge   s2lax_polygon_shape.h:306
      google#3 EncodedS2LaxPolygonShape::edge         s2lax_polygon_shape.cc:360

The fix requires the offsets to start at 0, be non-decreasing, and end at the
vertex count, in both decode paths.

The test asserts that Init() rejects such bytes, which is what makes the
regression visible in a normal build: an out-of-bounds read does not fault
without a sanitizer, so a traversal-based test would pass either way.

    s2lax_polygon_shape_test --gtest_filter='*RejectsInconsistentLoopStarts*'
      before: [  FAILED  ] 1 test.
      after:  [  PASSED  ] 1 test.

It corrupts only the "starts at 0" rule, leaving the offsets non-decreasing and
ending at the vertex count, so a check that covered only monotonicity or only
the final offset would still pass it.

Suites on this branch: s2lax_polygon_shape_test 21/21,
encoded_s2shape_index_test 9/9, mutable_s2shape_index_test 29/29.

Split out of google#675 as requested there, and independent of google#680.
The macos-15-intel job ended with "The operation was canceled." inside the
build step; the test steps in the same job reported 100% of 1878 tests passed.
No source change: this commit only re-triggers the workflow.
@sushant-me

Copy link
Copy Markdown
Author

Note on the cancelled check: cmake (macos-15-intel) ended with

##[error]The operation was canceled.

inside the cmake --build . --parallel 4 step — the runner was cancelled mid-build (the job ran 17m6s).
The test steps in that same job then reported 100% tests passed out of 1878 and 100% tests passed out of 20, and lint and both workflow-security jobs are green. So the red X is the runner, not the change.

I pushed an empty commit to re-trigger the workflow (no source change); the new runs are waiting on
approval. I cannot re-run jobs myself on this repository, so if it is more convenient to just hit
"Re-run failed jobs" on the old run, that would be equivalent.

@jmr jmr changed the title Validate loop_starts when decoding a lax polygon shape S2LaxPolygonShape::Init: Validate loop_starts Sep 25, 2026

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