Reject out-of-range reflection Type.index values during verification

[huynhtrungcsc]

Reject out-of-range reflection Type.index values during verification

Summary

flatbuffers::Verify(const reflection::Schema& schema, const reflection::Object& root_table, const uint8_t* buf, size_t len, ...) uses reflection::Type::index() to look up the referenced reflection::Object or reflection::Enum from schema.objects() / schema.enums(). Four verifier-reachable call sites in src/reflection.cpp perform this lookup without bounds-checking the index:

• VerifyUnion: schema.enums()->Get(union_field.type()->index())
• VerifyUnion: schema.objects()->Get(elem_type->index()) (union element)
• VerifyVector: schema.objects()->Get(vec_field.type()->index())
• VerifyObject: schema.objects()->Get(field_def->type()->index())

flatbuffers::Vector::Get(uoffset_t i) does not bounds-check i, and reflection::Type::index() returns a signed int32_t with a default of -1 when the field is absent. A schema buffer can be structurally well-formed and pass reflection::VerifySchemaBuffer(...) while still carrying semantically out-of-range Type.index values. When such a schema is then passed to the data verifier, the unchecked lookup reads past the end of the objects/enums vector and the subsequent dereference (e.g. child_obj->is_struct()) walks off mapped memory.

This PR adds two static inline helpers in the anonymous namespace of src/reflection.cpp:

static inline const reflection::Object* GetObjectByIndex(
    const reflection::Schema& schema, int32_t index);
static inline const reflection::Enum* GetEnumByIndex(
    const reflection::Schema& schema, int32_t index);

Each helper returns nullptr if the corresponding vector is null, the index is negative, or the index is past the end of the vector. The four verifier-reachable sites are rerouted through these helpers and return false cleanly on nullptr. The defensive style mirrors the existing GetObjectByIndex / GetEnumByIndex helpers in src/bfbs_gen.h (lines 169–188).

There are no public API or public header changes. The helpers are file-local; no symbol is exported.

Test

Added ReflectionTypeIndexBoundsTest in tests/reflection_test.cpp (prototype in tests/reflection_test.h, registered alongside ReflectionTest and ForAllFieldsReverseTest in tests/test.cpp).

The test:

1. Loads the existing fixture tests/monster_test.bfbs into a mutable byte buffer and asserts reflection::VerifySchemaBuffer(...) accepts it.
2. Loads the existing fixture tests/monsterdata_test.mon and asserts flatbuffers::Verify(schema, *schema.root_table(), data, size) returns true for the unmodified pair.
3. Locates MyGame.Example.Monster and its pos field (confirmed reflection::Obj).
4. Mutates only the reflection::Type::VT_INDEX scalar of the pos field in place to static_cast<int32_t>(schema.objects()->size() + 100) via Table::GetAddressOf(VT_INDEX) + flatbuffers::WriteScalar<int32_t> — exactly 4 bytes, no offset rewriting.
5. Asserts the mutated BFBS still passes reflection::VerifySchemaBuffer(...) (structurally valid).
6. Asserts flatbuffers::Verify(...) now returns false with the patch applied.
7. Restores the original index and asserts flatbuffers::Verify(...) returns true again, demonstrating the rejection is precise to the malformed input.

No new test fixtures are added; only the existing monster_test.bfbs and monsterdata_test.mon are reused.

Negative control. Keeping the regression test in place but reverting src/reflection.cpp to its pre-patch state causes flattests to terminate with SIGSEGV (process exit code 139, ctest reports ***Exception: SegFault). Restoring src/reflection.cpp to its patched state causes the full suite, including ReflectionTypeIndexBoundsTest, to pass cleanly.

Validation

cmake --build build_clean -j2
ctest --test-dir build_clean --output-on-failure
git diff --check

Observed results:

• flattests passed 1/1 (100% tests passed, 0 tests failed out of 1, total time ~0.18s).

• git diff --check returned clean (exit 0, no whitespace or merge-marker errors).

• Diffstat (4 files only, no build outputs): +123 / −4.

  src/reflection.cpp        | 38 +++++++++-
  tests/reflection_test.cpp | 87 ++++++++++++++++++++++
  tests/reflection_test.h   |  1 +
  tests/test.cpp            |  1 +
  4 files changed, 123 insertions(+), 4 deletions(-)
  

Scope

This PR is intentionally narrow:

• In scope. The four verifier-reachable Type.index() lookups in VerifyUnion, VerifyVector, and VerifyObject. These are the only sites reachable through the public flatbuffers::Verify(schema, root_table, buf, len, ...) entry point.
• Out of scope. Other unchecked Type.index() uses in CopyTable, GetAnyValueS, ResizeContext::ResizeTable, and GetUnionType. Those paths have different threat models (they operate on inputs already trusted to match the schema and are not part of the verifier surface) and changing them would require API-level consideration. They can be addressed in a follow-up if maintainers prefer.
• Behavior on valid schemas is unchanged. For any Type.index that is non-negative and within the corresponding vector, the helpers degenerate to vector->Get(index) — identical to the prior code. The full pre-existing test suite continues to pass.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.