From 2c1be7782f9a49ee745b6aa29d09d8d30f50bb7b Mon Sep 17 00:00:00 2001 From: Denis Blank Date: Tue, 13 Sep 2022 11:11:38 +0200 Subject: [PATCH] [C++] Fix final buffer alignment when using an array of structs * A struct can have an arbitrary size and therefore sizeof(struct) == alignof(struct) does not hold anymore as for value primitives. * This patch fixes this by introducing alignment parameters to various CreateVector*/StartVector calls. * Closes #7516 --- include/flatbuffers/flatbuffer_builder.h | 38 +++++++++++++++--------- src/idl_parser.cpp | 4 ++- src/reflection.cpp | 3 +- 3 files changed, 29 insertions(+), 16 deletions(-) diff --git a/include/flatbuffers/flatbuffer_builder.h b/include/flatbuffers/flatbuffer_builder.h index f9432338f07..66c33ffccdf 100644 --- a/include/flatbuffers/flatbuffer_builder.h +++ b/include/flatbuffers/flatbuffer_builder.h @@ -449,7 +449,7 @@ class FlatBufferBuilder { } template void PreAlign(size_t len) { AssertScalarT(); - PreAlign(len, sizeof(T)); + PreAlign(len, AlignOf()); } /// @endcond @@ -589,11 +589,15 @@ class FlatBufferBuilder { return PushElement(static_cast(len)); } - void StartVector(size_t len, size_t elemsize) { + void StartVector(size_t len, size_t elemsize, size_t alignment) { NotNested(); nested = true; PreAlign(len * elemsize); - PreAlign(len * elemsize, elemsize); // Just in case elemsize > uoffset_t. + PreAlign(len * elemsize, alignment); // Just in case elemsize > uoffset_t. + } + + template void StartVector(size_t len) { + return StartVector(len, sizeof(T), AlignOf()); } // Call this right before StartVector/CreateVector if you want to force the @@ -627,7 +631,7 @@ class FlatBufferBuilder { // If this assert hits, you're specifying a template argument that is // causing the wrong overload to be selected, remove it. AssertScalarT(); - StartVector(len, sizeof(T)); + StartVector(len); if (len == 0) { return Offset>(EndVector(len)); } // clang-format off #if FLATBUFFERS_LITTLEENDIAN @@ -668,7 +672,7 @@ class FlatBufferBuilder { template Offset>> CreateVector(const Offset *v, size_t len) { - StartVector(len, sizeof(Offset)); + StartVector>(len); for (auto i = len; i > 0;) { PushElement(v[--i]); } return Offset>>(EndVector(len)); } @@ -688,7 +692,7 @@ class FlatBufferBuilder { // an array. Instead, read elements manually. // Background: https://isocpp.org/blog/2012/11/on-vectorbool Offset> CreateVector(const std::vector &v) { - StartVector(v.size(), sizeof(uint8_t)); + StartVector(v.size()); for (auto i = v.size(); i > 0;) { PushElement(static_cast(v[--i])); } @@ -762,7 +766,7 @@ class FlatBufferBuilder { for (auto it = begin; it != end; ++it) { buf_.scratch_push_small(CreateString(*it)); } - StartVector(size, sizeof(Offset)); + StartVector>(size); for (auto i = 1; i <= size; i++) { // Note we re-evaluate the buf location each iteration to account for any // underlying buffer resizing that may occur. @@ -782,7 +786,7 @@ class FlatBufferBuilder { /// where the vector is stored. template Offset> CreateVectorOfStructs(const T *v, size_t len) { - StartVector(len * sizeof(T) / AlignOf(), AlignOf()); + StartVector(len * sizeof(T) / AlignOf(), sizeof(T), AlignOf()); if (len > 0) { PushBytes(reinterpret_cast(v), sizeof(T) * len); } @@ -1025,9 +1029,9 @@ class FlatBufferBuilder { /// written to at a later time to serialize the data into a `vector` /// in the buffer. uoffset_t CreateUninitializedVector(size_t len, size_t elemsize, - uint8_t **buf) { + size_t alignment, uint8_t **buf) { NotNested(); - StartVector(len, elemsize); + StartVector(len, elemsize, alignment); buf_.make_space(len * elemsize); auto vec_start = GetSize(); auto vec_end = EndVector(len); @@ -1035,6 +1039,12 @@ class FlatBufferBuilder { return vec_end; } + FLATBUFFERS_ATTRIBUTE([[deprecated("call the version above instead")]]) + uoffset_t CreateUninitializedVector(size_t len, size_t elemsize, + uint8_t **buf) { + return CreateUninitializedVector(len, elemsize, elemsize, buf); + } + /// @brief Specialized version of `CreateVector` for non-copying use cases. /// Write the data any time later to the returned buffer pointer `buf`. /// @tparam T The data type of the data that will be stored in the buffer @@ -1046,14 +1056,14 @@ class FlatBufferBuilder { template Offset> CreateUninitializedVector(size_t len, T **buf) { AssertScalarT(); - return CreateUninitializedVector(len, sizeof(T), + return CreateUninitializedVector(len, sizeof(T), AlignOf(), reinterpret_cast(buf)); } template Offset> CreateUninitializedVectorOfStructs(size_t len, T **buf) { - return CreateUninitializedVector(len, sizeof(T), + return CreateUninitializedVector(len, sizeof(T), AlignOf(), reinterpret_cast(buf)); } @@ -1064,7 +1074,7 @@ class FlatBufferBuilder { Offset> CreateVectorScalarCast(const U *v, size_t len) { AssertScalarT(); AssertScalarT(); - StartVector(len, sizeof(T)); + StartVector(len); for (auto i = len; i > 0;) { PushElement(static_cast(v[--i])); } return Offset>(EndVector(len)); } @@ -1173,7 +1183,7 @@ class FlatBufferBuilder { // Allocates space for a vector of structures. // Must be completed with EndVectorOfStructs(). template T *StartVectorOfStructs(size_t vector_size) { - StartVector(vector_size * sizeof(T) / AlignOf(), AlignOf()); + StartVector(vector_size * sizeof(T) / AlignOf(), sizeof(T), AlignOf()); return reinterpret_cast(buf_.make_space(vector_size * sizeof(T))); } diff --git a/src/idl_parser.cpp b/src/idl_parser.cpp index cadcb6a09fc..d8015cb6e58 100644 --- a/src/idl_parser.cpp +++ b/src/idl_parser.cpp @@ -1614,6 +1614,7 @@ CheckedError Parser::ParseVector(const Type &type, uoffset_t *ovalue, }); ECHECK(err); + const size_t alignment = InlineAlignment(type); const size_t len = count * InlineSize(type) / InlineAlignment(type); const size_t elemsize = InlineAlignment(type); const auto force_align = field->attributes.Lookup("force_align"); @@ -1623,7 +1624,8 @@ CheckedError Parser::ParseVector(const Type &type, uoffset_t *ovalue, if (align > 1) { builder_.ForceVectorAlignment(len, elemsize, align); } } - builder_.StartVector(len, elemsize); + // TODO Fix using element alignment as size (`elemsize`)! + builder_.StartVector(len, elemsize, alignment); for (uoffset_t i = 0; i < count; i++) { // start at the back, since we're building the data backwards. auto &val = field_stack_.back().first; diff --git a/src/reflection.cpp b/src/reflection.cpp index 9abfd9fac4c..6ea2d20b121 100644 --- a/src/reflection.cpp +++ b/src/reflection.cpp @@ -689,9 +689,10 @@ Offset CopyTable(FlatBufferBuilder &fbb, FLATBUFFERS_FALLTHROUGH(); // fall thru default: { // Scalars and structs. auto element_size = GetTypeSize(element_base_type); + auto element_alignment = element_size; // For primitive elements if (elemobjectdef && elemobjectdef->is_struct()) element_size = elemobjectdef->bytesize(); - fbb.StartVector(vec->size(), element_size); + fbb.StartVector(vec->size(), element_size, element_alignment); fbb.PushBytes(vec->Data(), element_size * vec->size()); offset = fbb.EndVector(vec->size()); break;