Skip to content

Fix reflection buffer-editing bugs and alignment in reflection and the Dart, Rust and Swift builders - #9429

Open
jagadish-chandra-vantimi wants to merge 13 commits into
google:masterfrom
jagadish-chandra-vantimi:ccr-2c078064-cqg1pk
Open

jagadish-chandra-vantimi wants to merge 13 commits into
google:masterfrom
jagadish-chandra-vantimi:ccr-2c078064-cqg1pk

Conversation

@jagadish-chandra-vantimi

Copy link
Copy Markdown

Fixes existing bugs that corrupt or misalign FlatBuffers data in the C++ reflection editing and copying code (src/reflection.cpp), a deprecated C++ builder overload, and the Dart, Rust and Swift builders. All of them are on master today. They were found while reviewing #9267 and are kept separate so that PR stays focused. Each commit covers one area, so this can be split per language if you prefer.

Full write-up of every bug, with reproductions and test results: jagadish-chandra-vantimi#2

Summary

  • C++ reflection:
    • SetString shrinking erased the wrong bytes.
    • Straddle mishandled objects starting exactly at the insertion point.
    • Resizing ignored vectors of unions.
    • CopyTable mishandled union members and union vectors (an out-of-bounds read on a verified buffer) and unknown union tags.
    • CopyTable and resizing lost struct-vector, force_align and nested-buffer alignment.
    • AddFlatBuffer kept the added buffer only 8-aligned.
  • C++ builder: the deprecated CreateUninitializedVector(len, elemsize, buf) used elemsize as the alignment.
  • Dart: 64-bit lists and generated structs were misaligned, which corrupted e.g. Monster pos and test4. This needs a release note: new generated code needs the new runtime.
  • Rust: 8-byte scalars and enums were only 4-aligned on 32-bit x86.
  • Swift:
    • Structs ignored `

std::make_unique is C++14, which broke the C++11 configurations of the
"Build Linux C++" job. Construct the unique_ptrs directly instead, and
take ownership of the object returned by UnPack(), which was leaked
(reported by LeakSanitizer).
- CopyTable created vectors of structs aligned to GetTypeSize(Obj) (4)
  instead of the struct's minalign, so the copied elements of structs
  with 8-byte or larger alignment could be misaligned. Use the struct's
  minalign, as struct fields already do.
- ResizeContext (SetString, ResizeVector, ResizeAnyVector) rounded the
  size change to sizeof(largest_scalar_t), which moved structs with
  force_align 16 or 32 off their alignment. Round to the largest valid
  struct alignment in the schema instead.

Both only use alignments that pass VerifyAlignmentRequirements, so a
malformed schema cannot produce a zero or huge alignment.
The overload without an explicit alignment passed elemsize as the
alignment. For a 12-byte struct that set minalign_ to 12, which made the
finished buffer only 4-aligned and misaligned 8-byte data elsewhere in
it. Use the largest power of two dividing elemsize, which is never less
than the element's own alignment.
- SetString: shrinking a string erased bytes in front of its first
  character, i.e. its length and the end of the preceding object (for
  example the last element of a vector before it). Shrink from the end
  of the old characters instead.
- ResizeContext: an object starting exactly at the insertion point moves
  with everything after it, but Straddle treated it as straddling. So
  resizing a vector directly followed by a table's vtable broke that
  table. Use a strict lower bound. Also fix the sign of the adjustment
  for a vtable that precedes its table across the insertion point.
- ResizeContext skipped vectors of unions, leaving their offsets stale,
  and treated every union member as a table, reading struct members as
  tables and string members through an invalid object index. Handle
  union vectors and all union member kinds.
- CopyTable treated every union member as a table: struct members were
  copied as empty (or read out of bounds) and string members used an
  invalid object index. Vectors of unions were copied as raw offsets
  into the source buffer. Copy each member according to its kind, and
  keep CopyTable's two passes in step when a union has no value.
- CopyTable and ResizeContext ignored the extra alignment of vectors
  with force_align and of nested FlatBuffer/FlexBuffer vectors (known
  from schemas with builtin attributes), and ResizeContext ignored
  force_align on vector fields.
- AddFlatBuffer only kept the added buffer 8-byte aligned, misaligning
  structs with force_align 16 or 32 in it.

Each fix has a regression test in reflection_test.cpp that fails (or
crashes) without it.
- When CopyTable copies data with a schema that doesn't know some union
  members (for example newer data with an older schema), it can't copy
  their values, but it still copied their tags. A single union then had
  a tag without a value, which fails verification, and a union vector
  had zero offsets with real tags, which reads as empty values. Make
  such tags NONE.
- force_align on vectors of strings, tables and unions was still
  dropped; apply it to every vector kind.
- AddFlatBuffer looped forever if FLATBUFFERS_MAX_ALIGNMENT was
  overridden to 4 or less; always align to at least the largest scalar.
The Dart Builder wrote several kinds of data at positions that violate
the FlatBuffers alignment rules, which C++ (and other) readers rely on:

* writeListFloat64/Int64/Uint64 called
  _prepare(8, n, additionalBytes: 4), which aligns the 32-bit length to
  8 bytes and therefore places every element at 4 mod 8. The format
  requires the elements to be 8-byte aligned with the length directly
  before element 0 (see FlatBufferBuilder::StartVector).

* Generated struct writers emitted only explicit field padding and
  relied on put*() to align each field, but never aligned the struct
  itself. When a struct started at a position that was not a multiple of
  its alignment, put*() inserted extra padding inside the struct,
  shifting fields: e.g. Vec3 (force_align: 8) or Test written into a
  table after odd-sized data were read back with wrong values.

* Struct vectors were not aligned before writing their elements, so
  endStructVector()'s putUint32 could insert padding between the length
  and the first struct (e.g. a [Test] written right after a table with
  a 6-byte vtable was read back shifted by two bytes).

Fix the 64-bit list writers to align the elements to 8 and then write
the length immediately before them. Add Builder.prep(size,
additionalBytes) (like Java/Python prep / C++ PreAlign) and
Builder.startStructVector(count, structSize, alignment) (like C++
StartVector). The Dart generator now emits fbBuilder.prep(minalign,
bytesize) at the start of every struct writer and aligns struct
vectors: pack() calls startStructVector(), and writeListOfStructs()
gets optional named structSize/alignment parameters, which generated
code passes. Calls without them behave as before, so existing callers
still compile. Generated code must be regenerated; new generated code
requires this version of the Dart runtime.

Regenerate the checked-in Dart code with scripts/generate_code.py
(tests/*.dart) and apply the same generator changes to the formatted
copies in dart/test. Update the Dart tutorial snippet for struct
vectors to call startStructVector().

Add Dart regression tests for the alignment of 64-bit scalar vectors,
struct vectors and Monster tables written by generated code at every
possible buffer offset; they fail without this fix (misaligned
elements, and corrupted test4/native_inline values).
Push::alignment() defaults to align_of::<Self::Output>(), and neither
the Push impls for the scalar types nor the generated Push impls for
enums overrode it. align_of is target-dependent: on 32-bit x86 (i686)
align_of::<f64>(), align_of::<i64>() and align_of::<u64>() are 4. So on
such targets FlatBufferBuilder aligned 8-byte scalars (and enums with an
8-byte base type, e.g. `enum E : ulong`) to only 4 bytes, both as table
fields and as vector elements, and min_align and the finished buffer
size only to 4. The FlatBuffers format requires scalars to be aligned to
their size relative to the start of the buffer; C++ readers access them
as aligned types, and the C++ verifier rejects such misaligned table
fields.

Fix this by overriding alignment() in impl_push_for_endian_scalar! to
return size_of::<T>(), and by making flatc's Rust generator emit an
alignment() for enum Push impls that defers to the base type's Push
alignment. Generated structs already use the schema's minalign
(including force_align), so they are unchanged.

Regenerate the checked-in Rust code with scripts/generate_code.py; the
only change is the new alignment() in every enum/union-type Push impl.

Add tests that check Push::alignment() of all scalar types and of
generated 1- and 8-byte enums, the padding written when pushing i64,
f64 and an 8-byte enum after a u8, and that 8-byte vector elements and
8-byte scalar/enum table fields of a Monster end up 8-aligned relative
to the start of the buffer. On i686 these fail without this change
(along with the existing push_u8_u64_alignment test) and pass with it.
The Swift builder aligned structs to MemoryLayout<T>.alignment, both in
create(struct:) (struct fields in tables) and in vectors of structs
(createVector(ofStructs:) and the generated startVectorOf* functions).
Swift has no way to express force_align, so the natural alignment of a
generated struct can be lower than its FlatBuffers minalign. For example
`struct F (force_align: 16) { a:int; }` is generated as an Int32 followed
by UInt32/UInt64 padding, which is only 8-byte aligned, and a struct that
merely contains F inherits a minalign of 16 without having force_align
itself. Such structs ended up misaligned relative to the start of the
buffer, and the buffer was not padded to their alignment.

Vectors of structs also passed `count * size` as the vector length and the
alignment as the element size to startVector(_:elementSize:), so the
length prefix was aligned for `count * size * alignment` bytes. For 2-byte
aligned structs whose total size is 2 mod 4 (e.g. `struct S { a:short; }`
with an odd count), endVector then had to insert padding between the
length and the first element, corrupting the vector.

Fix:
- NativeStruct gets a `static var alignment: Int` requirement (defaulting
  to MemoryLayout<Self>.alignment), and generated structs implement it
  with the struct's minalign.
- Add startVector(_:elementSize:alignment:), mirroring C++
  StartVector(len, elemsize, alignment): the length is aligned for
  `len * elementSize` bytes and the elements to `alignment`.
- create(struct:) and createVector(ofStructs:) use T.alignment, and the
  generated startVectorOf* functions call the new startVector.

The new test also showed that the generated Object API pack wrote struct
vectors in reverse order: it added the elements front to back with
create(struct:), but the buffer is built back to front. pack now uses
createVector(ofStructs:), which keeps the order (and the alignment).

Buffers that were already correctly aligned are byte-for-byte unchanged.
Code generated by an older flatc still compiles against the new runtime
and behaves as before. Regenerate it to get the fix.

Add tests/swift/fuzzer/CodeGenerationTests/force_align_test.fbs and
FlatBuffersForceAlignTests, which check struct field and vector element
alignment, the position of the vector length, the buffer size and the
round-tripped values through the builder and Object APIs. They fail
without this change.
rust/reflection/src/reflection_generated.rs is not regenerated by
scripts/generate_code.py, so it missed the alignment() that generated
enum Push impls now have. Without it, building reflection Schema tables
on 32-bit targets wrote the u64 AdvancedFeatures field only 4-aligned.
The benchmarks only link benchmark_main; building the dependency's test
suite fails under its -Werror with newer compilers (Clang 18 reports
-Wunused-but-set-variable in test/options_test.cc).
The test-only gRPC dependencies pull in swift-crypto through
swift-certificates, which accepts "3.12.3"..<"6.0.0". swift-crypto 5.0.0
(September 2026) requires a Swift 6.2 toolchain with extensible enum
(@nonexhaustive) support, which the Swift 6.2 that CI installs does not
have, so "Test Swift Linux (6.2)" failed with "unknown attribute
'nonexhaustive'" in swift-crypto. Swift 6.0 and 6.1 were unaffected only
because they cannot read swift-crypto 5's 6.2 manifest.

Constrain swift-crypto to the releases below 5.0.0 for all toolchains.
CopyTable aligned vectors with the flexbuffer or nested_flatbuffer
attribute to the schema's largest alignment. The first thing CopyTable
copies ends up at the end of the new buffer, so when that was a short
FlexBuffer (or a nested buffer needing less alignment), the padding
landed after everything the copy refers to. SizeVerifier then computes a
size 4 bytes short: the Bazel test, which builds the tests with
FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE, failed on the copied monster
(724 != 728) because its 4-byte "flex" field is copied first.

FlexBuffers are now copied without extra alignment, as before this
branch and as the generated code builds them. A nested buffer is aligned
to the largest power of two dividing its size (capped at the schema's
largest alignment): a finished buffer is padded to a multiple of its
alignment, so this still covers it, and needs no padding after it when it
comes first.

Add CopyTableTrailingPaddingTest, which checks this in every build.
Comment thread Package.swift
from: "2.0.0"),
// Pulled in by grpc-swift-nio-transport (via swift-certificates).
// swift-crypto 5 uses @nonexhaustive, which needs a newer Swift 6.2
// toolchain than CI installs, so stay on the 3.x/4.x releases that

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ccr-2c078064-cqg1pk

Comment thread benchmarks/CMakeLists.txt
# For Windows: Prevent overriding the parent project's compiler/linker
# settings.
set(gtest_force_shared_crt ON CACHE BOOL "" FORCE)
# Don't build Google Benchmark's own tests; they fail to compile with newer

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ccr-2c078064-cqg1pk

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☔️

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ codegen Involving generating code from schema dart documentation Documentation python rust swift

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants