Repository navigation
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
Conversation
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.
jagadish-chandra-vantimi
requested a review
from dbaileychess
as a code owner
October 22, 2026 17:16
| 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 |
There was a problem hiding this comment.
ccr-2c078064-cqg1pk
| # 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 |
There was a problem hiding this comment.
ccr-2c078064-cqg1pk
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
SetStringshrinking erased the wrong bytes.Straddlemishandled objects starting exactly at the insertion point.CopyTablemishandled union members and union vectors (an out-of-bounds read on a verified buffer) and unknown union tags.CopyTableand resizing lost struct-vector,force_alignand nested-buffer alignment.AddFlatBufferkept the added buffer only 8-aligned.CreateUninitializedVector(len, elemsize, buf)usedelemsizeas the alignment.posandtest4. This needs a release note: new generated code needs the new runtime.