S2LaxPolygonShape::Init: Validate loop_starts

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<uint32_t>(num_vertices_)) {
+          return Error("Invalid loop offsets");
+        }
+      }
+      if (loop_starts_[num_loops_] != static_cast<uint32_t>(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<size_t>(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;
 }

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<vector<S2Point>> 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<unsigned char>(encoded[n - 4])) << "varint header";
+  ASSERT_EQ(0u, static_cast<unsigned char>(encoded[n - 3]));
+  ASSERT_EQ(3u, static_cast<unsigned char>(encoded[n - 2]));
+  ASSERT_EQ(7u, static_cast<unsigned char>(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));
+}