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)); +}