Fix OOB write in Vector/Array Mutate() on out-of-range index

include/flatbuffers/array.h

@@ -90,7 +90,14 @@ class Array {
   }
 
   // Change elements if you have a non-const pointer to this object.
-  void Mutate(uoffset_t i, const T& val) { MutateImpl(scalar_tag(), i, val); }
+  // Returns false (and performs no write) if `i` is out of range, so an
+  // out-of-range caller-supplied index cannot cause an out-of-bounds write
+  // even in a release (NDEBUG) build where FLATBUFFERS_ASSERT is compiled
+  // out. The assert in MutateImpl is kept for early detection in debug
+  // builds.
+  bool Mutate(uoffset_t i, const T& val) {
+    return MutateImpl(scalar_tag(), i, val);
+  }
 
   // The raw data in little endian format. Use with care.
   const uint8_t* Data() const { return data_; }
@@ -114,13 +121,18 @@ class Array {
   }
 
  protected:
-  void MutateImpl(flatbuffers::true_type, uoffset_t i, const T& val) {
+  bool MutateImpl(flatbuffers::true_type, uoffset_t i, const T& val) {
     FLATBUFFERS_ASSERT(i < size());
+    if (i >= size()) return false;
     WriteScalar(data() + i, val);
+    return true;
   }
 
-  void MutateImpl(flatbuffers::false_type, uoffset_t i, const T& val) {
+  bool MutateImpl(flatbuffers::false_type, uoffset_t i, const T& val) {
+    FLATBUFFERS_ASSERT(i < size());
+    if (i >= size()) return false;
     *(GetMutablePointer(i)) = val;
+    return true;
   }
 
   void CopyFromSpanImpl(flatbuffers::true_type,

include/flatbuffers/vector.h

@@ -249,19 +249,29 @@ class Vector {
 
   // Change elements if you have a non-const pointer to this object.
   // Scalars only. See reflection.h, and the documentation.
-  void Mutate(SizeT i, const T& val) {
+  // Returns false (and performs no write) if `i` is out of range, so an
+  // out-of-range caller-supplied index cannot cause an out-of-bounds write
+  // even in a release (NDEBUG) build where FLATBUFFERS_ASSERT is compiled
+  // out. The assert is kept for early detection in debug builds.
+  bool Mutate(SizeT i, const T& val) {
     FLATBUFFERS_ASSERT(i < size());
+    if (i >= size()) return false;
     WriteScalar(data() + i, val);
+    return true;
   }
 
   // Change an element of a vector of tables (or strings).
   // "val" points to the new table/string, as you can obtain from
   // e.g. reflection::AddFlatBuffer().
-  void MutateOffset(SizeT i, const uint8_t* val) {
+  // Returns false (and performs no write) if `i` is out of range; see the
+  // comment on Mutate() above.
+  bool MutateOffset(SizeT i, const uint8_t* val) {
     FLATBUFFERS_ASSERT(i < size());
+    if (i >= size()) return false;
     static_assert(sizeof(T) == sizeof(SizeT), "Unrelated types");
     WriteScalar(data() + i,
                 static_cast<SizeT>(val - (Data() + i * sizeof(SizeT))));
+    return true;
   }
 
   // Get a mutable pointer to tables/strings inside this vector.

tests/test.cpp

@@ -1572,6 +1572,82 @@ void VectorSpanTest() {
   }
 }
 
+// Regression test for the Vector<T>::Mutate() / MutateOffset() and
+// Array<T,length>::Mutate() out-of-bounds write: previously the only guard
+// on the caller-supplied index was FLATBUFFERS_ASSERT(i < size()), which is
+// compiled out under NDEBUG. Mutate()/MutateOffset() now return bool and
+// fail closed (return false, perform no write) on an out-of-range index,
+// unconditionally (i.e. even in a release/NDEBUG build).
+void MutateBoundsCheckTest() {
+  flatbuffers::FlatBufferBuilder builder;
+
+  auto mloc = CreateMonster(
+      builder, nullptr, 0, 0, builder.CreateString("Monster"),
+      builder.CreateVector<uint8_t>({0, 1, 2, 3, 4, 5, 6, 7, 8, 9}));
+
+  FinishMonsterBuffer(builder, mloc);
+
+  auto mutable_monster = GetMutableMonster(builder.GetBufferPointer());
+  auto inventory = mutable_monster->mutable_inventory();
+  TEST_NOTNULL(inventory);
+  TEST_EQ(inventory->size(), 10);
+
+  // In-range Mutate() still succeeds and writes correctly (no regression).
+  TEST_EQ(inventory->Mutate(0, 42), true);
+  TEST_EQ((*inventory)[0], 42);
+  TEST_EQ(inventory->Mutate(0, 0), true);
+  TEST_EQ((*inventory)[0], 0);
+
+  TEST_EQ(inventory->Mutate(inventory->size() - 1, 99), true);
+  TEST_EQ((*inventory)[inventory->size() - 1], 99);
+  TEST_EQ(inventory->Mutate(inventory->size() - 1, 9), true);
+
+  // Out-of-range Mutate() (size()+10) now fails closed: returns false, no
+  // write, no crash, no memory corruption -- this is the security fix.
+  // FLATBUFFERS_ASSERT(i < size()) is intentionally kept alongside the
+  // bounds check for early detection in debug builds, so it still aborts
+  // there by design; the fail-closed return-false path this test exists to
+  // check is the one that matters -- the release (NDEBUG) build, where the
+  // assert is compiled out and the bounds check is the only thing standing
+  // between an out-of-range index and an OOB write. Gate on NDEBUG so this
+  // test exercises that path (built+run under ASan; see PoC re-verification
+  // in the disclosure package for the equivalent standalone repro) without
+  // tripping the assert in ordinary debug test runs.
+#if defined(NDEBUG)
+  TEST_EQ(inventory->Mutate(inventory->size() + 10, 0x41414141), false);
+
+  // The buffer must be unchanged by the rejected out-of-range mutation.
+  for (flatbuffers::uoffset_t i = 0; i < inventory->size(); ++i) {
+    TEST_EQ((*inventory)[i], i);
+  }
+#endif
+
+  // Same coverage for MutateOffset(): build a small vector-of-strings
+  // buffer and confirm both the in-range and out-of-range behavior.
+  {
+    flatbuffers::FlatBufferBuilder sbuilder;
+    std::vector<flatbuffers::Offset<flatbuffers::String>> strings;
+    strings.push_back(sbuilder.CreateString("hello"));
+    strings.push_back(sbuilder.CreateString("world"));
+    auto svec_offset = sbuilder.CreateVector(strings);
+    sbuilder.Finish(svec_offset);
+
+    auto* svec = flatbuffers::GetMutableRoot<
+        flatbuffers::Vector<flatbuffers::Offset<flatbuffers::String>>>(
+        sbuilder.GetBufferPointer());
+    TEST_EQ(svec->size(), 2);
+
+#if defined(NDEBUG)
+    // Out-of-range MutateOffset() must fail closed rather than writing past
+    // the end of the vector. See the NDEBUG note above.
+    TEST_EQ(
+        svec->MutateOffset(svec->size() + 10,
+                            reinterpret_cast<const uint8_t*>(svec->Get(0))),
+        false);
+#endif
+  }
+}
+
 void NativeInlineTableVectorTest() {
   TestNativeInlineTableT test;
   for (int i = 0; i < 10; ++i) {
@@ -1853,6 +1929,7 @@ int FlatBufferTests(const std::string& tests_data_path) {
   PrivateAnnotationsLeaks();
   JsonUnsortedArrayTest();
   VectorSpanTest();
+  MutateBoundsCheckTest();
   NativeInlineTableVectorTest();
   FixedSizedScalarKeyInStructTest();
   StructKeyInStructTest();