[C++] Rare bad buffer content alignment if sizeof(T) != alignof(T) (#7520)

Naios · web-flow · commit 72aa85a759e9 · 2022-09-21T11:05:05.000-07:00
* [C++] Add a failing unit test for #7516 (Rare bad buffer content alignment if sizeof(T) != alignof(T)) * [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
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -230,6 +230,8 @@ set(FlatBuffers_Tests_SRCS
   tests/util_test.cpp
   tests/native_type_test_impl.h
   tests/native_type_test_impl.cpp
+  tests/alignment_test.h
+  tests/alignment_test.cpp
   include/flatbuffers/code_generators.h
   src/code_generators.cpp
   # file generate by running compiler on tests/monster_test.fbs
@@ -251,6 +253,8 @@ set(FlatBuffers_Tests_SRCS
   ${CMAKE_CURRENT_BINARY_DIR}/tests/optional_scalars_generated.h
   # file generate by running compiler on tests/native_inline_table_test.fbs
   ${CMAKE_CURRENT_BINARY_DIR}/tests/native_inline_table_test_generated.h
+  # file generate by running compiler on tests/alignment_test.fbs
+  ${CMAKE_CURRENT_BINARY_DIR}/tests/alignment_test_generated.h
 )
 
 set(FlatBuffers_Tests_CPP17_SRCS
@@ -623,6 +627,7 @@ if(FLATBUFFERS_BUILD_TESTS)
   compile_flatbuffers_schema_to_binary(tests/arrays_test.fbs)
   compile_flatbuffers_schema_to_embedded_binary(tests/monster_test.fbs "--no-includes;--gen-compare")
   compile_flatbuffers_schema_to_cpp(tests/native_inline_table_test.fbs "--gen-compare")
+  compile_flatbuffers_schema_to_cpp(tests/alignment_test.fbs "--gen-compare")
   if(NOT (MSVC AND (MSVC_VERSION LESS 1900)))
     compile_flatbuffers_schema_to_cpp(tests/monster_extra.fbs) # Test floating-point NAN/INF.
   endif()
diff --git a/include/flatbuffers/flatbuffer_builder.h b/include/flatbuffers/flatbuffer_builder.h
@@ -449,7 +449,7 @@ class FlatBufferBuilder {
   }
   template<typename T> void PreAlign(size_t len) {
     AssertScalarT<T>();
-    PreAlign(len, sizeof(T));
+    PreAlign(len, AlignOf<T>());
   }
   /// @endcond
 
@@ -589,11 +589,15 @@ class FlatBufferBuilder {
     return PushElement(static_cast<uoffset_t>(len));
   }
 
-  void StartVector(size_t len, size_t elemsize) {
+  void StartVector(size_t len, size_t elemsize, size_t alignment) {
     NotNested();
     nested = true;
     PreAlign<uoffset_t>(len * elemsize);
-    PreAlign(len * elemsize, elemsize);  // Just in case elemsize > uoffset_t.
+    PreAlign(len * elemsize, alignment);  // Just in case elemsize > uoffset_t.
+  }
+
+  template<typename T> void StartVector(size_t len) {
+    return StartVector(len, sizeof(T), AlignOf<T>());
   }
 
   // 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<T>();
-    StartVector(len, sizeof(T));
+    StartVector<T>(len);
     if (len == 0) { return Offset<Vector<T>>(EndVector(len)); }
     // clang-format off
     #if FLATBUFFERS_LITTLEENDIAN
@@ -668,7 +672,7 @@ class FlatBufferBuilder {
 
   template<typename T>
   Offset<Vector<Offset<T>>> CreateVector(const Offset<T> *v, size_t len) {
-    StartVector(len, sizeof(Offset<T>));
+    StartVector<Offset<T>>(len);
     for (auto i = len; i > 0;) { PushElement(v[--i]); }
     return Offset<Vector<Offset<T>>>(EndVector(len));
   }
@@ -688,7 +692,7 @@ class FlatBufferBuilder {
   // an array. Instead, read elements manually.
   // Background: https://isocpp.org/blog/2012/11/on-vectorbool
   Offset<Vector<uint8_t>> CreateVector(const std::vector<bool> &v) {
-    StartVector(v.size(), sizeof(uint8_t));
+    StartVector<uint8_t>(v.size());
     for (auto i = v.size(); i > 0;) {
       PushElement(static_cast<uint8_t>(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<String>));
+    StartVector<Offset<String>>(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<typename T>
   Offset<Vector<const T *>> CreateVectorOfStructs(const T *v, size_t len) {
-    StartVector(len * sizeof(T) / AlignOf<T>(), AlignOf<T>());
+    StartVector(len * sizeof(T) / AlignOf<T>(), sizeof(T), AlignOf<T>());
     if (len > 0) {
       PushBytes(reinterpret_cast<const uint8_t *>(v), sizeof(T) * len);
     }
@@ -1025,16 +1029,22 @@ 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);
     *buf = buf_.data_at(vec_start);
     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<typename T>
   Offset<Vector<T>> CreateUninitializedVector(size_t len, T **buf) {
     AssertScalarT<T>();
-    return CreateUninitializedVector(len, sizeof(T),
+    return CreateUninitializedVector(len, sizeof(T), AlignOf<T>(),
                                      reinterpret_cast<uint8_t **>(buf));
   }
 
   template<typename T>
   Offset<Vector<const T *>> CreateUninitializedVectorOfStructs(size_t len,
                                                                T **buf) {
-    return CreateUninitializedVector(len, sizeof(T),
+    return CreateUninitializedVector(len, sizeof(T), AlignOf<T>(),
                                      reinterpret_cast<uint8_t **>(buf));
   }
 
@@ -1064,7 +1074,7 @@ class FlatBufferBuilder {
   Offset<Vector<T>> CreateVectorScalarCast(const U *v, size_t len) {
     AssertScalarT<T>();
     AssertScalarT<U>();
-    StartVector(len, sizeof(T));
+    StartVector<T>(len);
     for (auto i = len; i > 0;) { PushElement(static_cast<T>(v[--i])); }
     return Offset<Vector<T>>(EndVector(len));
   }
@@ -1173,7 +1183,7 @@ class FlatBufferBuilder {
   // Allocates space for a vector of structures.
   // Must be completed with EndVectorOfStructs().
   template<typename T> T *StartVectorOfStructs(size_t vector_size) {
-    StartVector(vector_size * sizeof(T) / AlignOf<T>(), AlignOf<T>());
+    StartVector(vector_size * sizeof(T) / AlignOf<T>(), sizeof(T), AlignOf<T>());
     return reinterpret_cast<T *>(buf_.make_space(vector_size * sizeof(T)));
   }
 
diff --git 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
@@ -689,9 +689,10 @@ Offset<const Table *> 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;
diff --git a/tests/BUILD.bazel b/tests/BUILD.bazel
@@ -8,6 +8,9 @@ cc_test(
     name = "flatbuffers_test",
     testonly = 1,
     srcs = [
+        "alignment_test.cpp",
+        "alignment_test.h",
+        "alignment_test_generated.h",
         "evolution_test.cpp",
         "evolution_test.h",
         "evolution_test/evolution_v1_generated.h",
@@ -50,6 +53,7 @@ cc_test(
         "-DBAZEL_TEST_DATA_PATH",
     ],
     data = [
+        ":alignment_test.fbs",
         ":arrays_test.bfbs",
         ":arrays_test.fbs",
         ":arrays_test.golden",
@@ -90,6 +94,7 @@ cc_test(
         "include/",
     ],
     deps = [
+        ":alignment_test_cc_fbs",
         ":arrays_test_cc_fbs",
         ":monster_extra_cc_fbs",
         ":monster_test_cc_fbs",
@@ -214,3 +219,8 @@ flatbuffer_cc_library(
         "--cpp-ptr-type flatbuffers::unique_ptr",
     ],
 )
+
+flatbuffer_cc_library(
+    name = "alignment_test_cc_fbs",
+    srcs = ["alignment_test.fbs"],
+)
diff --git a/tests/alignment_test.cpp b/tests/alignment_test.cpp
@@ -0,0 +1,31 @@
+#include "alignment_test.h"
+
+#include "flatbuffers/flatbuffer_builder.h"
+#include "alignment_test_generated.h"
+#include "test_assert.h"
+
+namespace flatbuffers {
+namespace tests {
+
+void AlignmentTest() {
+  FlatBufferBuilder builder;
+
+  BadAlignmentLarge large;
+  Offset<OuterLarge> outer_large = CreateOuterLarge(builder, &large);
+
+  BadAlignmentSmall *small;
+  Offset<Vector<const BadAlignmentSmall *>> small_offset =
+      builder.CreateUninitializedVectorOfStructs(9, &small);
+  (void)small; // We do not have to write data to trigger the test failure
+
+  Offset<BadAlignmentRoot> root =
+      CreateBadAlignmentRoot(builder, outer_large, small_offset);
+
+  builder.Finish(root);
+
+  Verifier verifier(builder.GetBufferPointer(), builder.GetSize());
+  TEST_ASSERT(VerifyBadAlignmentRootBuffer(verifier));
+}
+
+}  // namespace tests
+}  // namespace flatbuffers
diff --git a/tests/alignment_test.fbs b/tests/alignment_test.fbs
@@ -0,0 +1,24 @@
+// sizeof(BadAlignmentSmall) == 12
+// alignof(BadAlignmentSmall) == 4
+struct BadAlignmentSmall {
+  var_0: uint;
+  var_1: uint;
+  var_2: uint;
+}
+
+// sizeof(BadAlignmentLarge) == 8
+// alignof(BadAlignmentLarge) == 8
+struct BadAlignmentLarge {
+  var_0: ulong;
+}
+
+table OuterLarge {
+  large: BadAlignmentLarge;
+}
+
+table BadAlignmentRoot {
+  large: OuterLarge;
+  small: [BadAlignmentSmall];
+}
+
+root_type BadAlignmentRoot;
diff --git a/tests/alignment_test.h b/tests/alignment_test.h
@@ -0,0 +1,12 @@
+#ifndef TESTS_ALIGNMENT_TEST_H
+#define TESTS_ALIGNMENT_TEST_H
+
+namespace flatbuffers {
+namespace tests {
+
+void AlignmentTest();
+
+}  // namespace tests
+}  // namespace flatbuffers
+
+#endif
diff --git a/tests/alignment_test_generated.h b/tests/alignment_test_generated.h
diff --git a/tests/test.cpp b/tests/test.cpp
