From eb03b4b23da43efddf58b6703c19cff8bc4bc8eb Mon Sep 17 00:00:00 2001 From: Sushant Poudel Date: Sat, 19 Sep 2026 04:00:09 +0545 Subject: [PATCH 1/2] fix: validate loop_starts when decoding a lax polygon shape 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 #2 EncodedS2LaxPolygonShape::chain_edge s2lax_polygon_shape.h:306 #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 #675 as requested there, and independent of #680. --- src/s2/s2lax_polygon_shape.cc | 29 ++++++++++++++++++++ src/s2/s2lax_polygon_shape_test.cc | 44 ++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+) diff --git a/src/s2/s2lax_polygon_shape.cc b/src/s2/s2lax_polygon_shape.cc index bfc9e414..f229fe76 100644 --- a/src/s2/s2lax_polygon_shape.cc +++ b/src/s2/s2lax_polygon_shape.cc @@ -246,6 +246,20 @@ bool S2LaxPolygonShape::Init(Decoder* decoder, S2Error* absl_nullable error) { for (size_t i = 0; i < loop_starts.size(); ++i) { loop_starts_[i] = loop_starts[i]; } + + // Validate loop_starts: it must start at 0, be non-decreasing, and end + // exactly at num_vertices_. Otherwise chain_edge() would index + // vertices_[loop_starts_[i] + j] out of bounds on malformed input. + if (loop_starts_[0] != 0) return Error("Invalid loop offsets"); + for (int i = 1; i <= num_loops_; ++i) { + if (loop_starts_[i] < loop_starts_[i - 1] || + loop_starts_[i] > static_cast(num_vertices_)) { + return Error("Invalid loop offsets"); + } + } + if (loop_starts_[num_loops_] != static_cast(num_vertices_)) { + return Error("Invalid loop offsets"); + } } } @@ -305,6 +319,21 @@ bool EncodedS2LaxPolygonShape::Init(Decoder* decoder) { if (num_loops_ > 1) { if (!loop_starts_.Init(decoder)) return false; + // Validate loop_starts: it must have num_loops_+1 entries, start at 0, + // be non-decreasing, and end exactly at the vertex count. Otherwise + // chain_edge() would index vertices_[loop_starts_[i] + j] out of bounds + // on malformed input. + if (loop_starts_.size() != static_cast(num_loops_) + 1) { + return false; + } + if (loop_starts_[0] != 0) return false; + for (int i = 1; i <= num_loops_; ++i) { + if (loop_starts_[i] < loop_starts_[i - 1] || + loop_starts_[i] > vertices_.size()) { + return false; + } + } + if (loop_starts_[num_loops_] != vertices_.size()) return false; } return true; } diff --git a/src/s2/s2lax_polygon_shape_test.cc b/src/s2/s2lax_polygon_shape_test.cc index 42506769..0c526a97 100644 --- a/src/s2/s2lax_polygon_shape_test.cc +++ b/src/s2/s2lax_polygon_shape_test.cc @@ -578,3 +578,47 @@ BENCHMARK(BM_DecodeS2LaxPolygonShape) ->ArgPair(0, 10000) ->ArgPair(1, 10) ->ArgPair(1, 10000); + +// A malformed encoding whose `loop_starts` do not partition the vertex array. +// +// `chain_edge()` indexes `vertices_[loop_starts_[i] + j]`, so offsets that are +// inconsistent with the vertex count read outside the vertex array. Before +// validation was added, `Init()` accepted these bytes and returned true, and +// the out-of-bounds read happened later inside `edge()` -- which a normal build +// does not fault on, so the bug is invisible outside a sanitizer. Asserting +// that `Init()` rejects the bytes is what makes it testable in any build. +TEST(S2LaxPolygonShape, RejectsInconsistentLoopStarts) { + vector> loops; + loops.push_back(s2textformat::ParsePointsOrDie("0:0, 0:1, 1:0")); + loops.push_back(s2textformat::ParsePointsOrDie("5:5, 5:6, 6:6, 6:5")); + S2LaxPolygonShape shape(loops); + ASSERT_EQ(2, shape.num_loops()); + ASSERT_EQ(7, shape.num_vertices()); + + Encoder encoder; + shape.Encode(&encoder, s2coding::CodingHint::COMPACT); + std::string encoded(encoder.base(), encoder.length()); + + // With more than one loop, `loop_starts` is encoded last. The offsets are + // {0, 3, 7}, and 7 fits in one byte, so the tail is the one-byte varint + // header 12 followed by those three bytes. Assert the layout rather than + // assuming it: if the encoding changes, this test should fail loudly instead + // of silently corrupting an unrelated byte. + ASSERT_GE(encoded.size(), 4u); + const size_t n = encoded.size(); + ASSERT_EQ(12u, static_cast(encoded[n - 4])) << "varint header"; + ASSERT_EQ(0u, static_cast(encoded[n - 3])); + ASSERT_EQ(3u, static_cast(encoded[n - 2])); + ASSERT_EQ(7u, static_cast(encoded[n - 1])); + + // Make the offsets start at 1 instead of 0. They are still non-decreasing and + // still end at the vertex count, so only the "starts at 0" rule is violated, + // which is precisely the case a partial check would miss. + std::string malformed = encoded; + malformed[n - 3] = 1; + + Decoder decoder(malformed.data(), malformed.size()); + S2LaxPolygonShape decoded; + S2Error error; + EXPECT_FALSE(decoded.Init(&decoder, error)); +} From bb0b9c2b0b2840469cbbefa06ea69020bee9107c Mon Sep 17 00:00:00 2001 From: Sushant Poudel Date: Sat, 19 Sep 2026 19:09:37 +0545 Subject: [PATCH 2/2] ci: re-run after the macOS job was cancelled during the build 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.