Fix misalignment of small structs in a Vector (C++) (google#7883)

* Rare fix: kick fix test * Rare fix: real fix * Rare fix: separate test * Rare fix: remove comments * Rare fix: updates * Rare fix: less * Rare fix: size_t switch to uoffset_t * Rare fix: swap exp/val * Rare fix: add annotated before/after * Rare fix: remove unnecessary changes --------- Co-authored-by: Derek Bailey <derekbailey@google.com>
jochenparm · Oct 29, 2024 · 08e07b4 · 08e07b4
1 parent 82b0a05
commit 08e07b4
Show file tree

Hide file tree

Showing 8 changed files with 138 additions and 6 deletions.
diff --git a/include/flatbuffers/flatbuffer_builder.h b/include/flatbuffers/flatbuffer_builder.h
@@ -913,8 +913,7 @@ template<bool Is64Aware = false> class FlatBufferBuilderImpl {
     typedef typename VectorT<T>::size_type LenT;
     typedef typename OffsetT<VectorT<const T *>>::offset_type offset_type;
 
-    StartVector<OffsetT, LenT>(len * sizeof(T) / AlignOf<T>(), sizeof(T),
-                               AlignOf<T>());
+    StartVector<OffsetT, LenT>(len, sizeof(T), AlignOf<T>());
     if (len > 0) {
       PushBytes(reinterpret_cast<const uint8_t *>(v), sizeof(T) * len);
     }
@@ -1384,8 +1383,7 @@ template<bool Is64Aware = false> class FlatBufferBuilderImpl {
   // Must be completed with EndVectorOfStructs().
   template<typename T, template<typename> class OffsetT = Offset>
   T *StartVectorOfStructs(size_t vector_size) {
-    StartVector<OffsetT>(vector_size * sizeof(T) / AlignOf<T>(), sizeof(T),
-                         AlignOf<T>());
+    StartVector<OffsetT>(vector_size, sizeof(T), AlignOf<T>());
     return reinterpret_cast<T *>(buf_.make_space(vector_size * sizeof(T)));
   }
 

diff --git a/tests/alignment_test.cpp b/tests/alignment_test.cpp
@@ -2,6 +2,7 @@
 
 #include "tests/alignment_test_generated.h"
 #include "flatbuffers/flatbuffer_builder.h"
+#include "flatbuffers/util.h"
 #include "test_assert.h"
 
 namespace flatbuffers {
@@ -24,7 +25,51 @@ void AlignmentTest() {
   builder.Finish(root);
 
   Verifier verifier(builder.GetBufferPointer(), builder.GetSize());
-  TEST_ASSERT(VerifyBadAlignmentRootBuffer(verifier));
+  TEST_ASSERT(verifier.VerifyBuffer<BadAlignmentRoot>(nullptr));
+
+
+  // ============= Test Small Structs Vector misalignment ========
+
+  builder.Clear();
+
+  // creating 5 structs with 2 bytes each
+  // 10 bytes in total for Vector data is needed
+  std::vector<JustSmallStruct> small_vector = {
+    { 2, 1 }, { 3, 1 }, { 4, 1 }
+  };
+  // CreateVectorOfStructs is used in the generated CreateSmallStructsDirect()
+  // method, but we test it directly
+  Offset<Vector<const JustSmallStruct *>> small_structs_offset =
+      builder.CreateVectorOfStructs<JustSmallStruct>(small_vector);
+  Offset<SmallStructs> small_structs_root =
+      CreateSmallStructs(builder, small_structs_offset);
+
+  builder.Finish(small_structs_root);
+
+  // Save the binary that we later can annotate with `flatc --annotate` command
+  // NOTE: the conversion of the JSON data to --binary via `flatc --binary`
+  //       command is not changed with that fix and was always producing the
+  //       correct binary data.
+  // SaveFile("alignment_test_{before,after}_fix.bin",
+  //          reinterpret_cast<char *>(builder.GetBufferPointer()),
+  //          builder.GetSize(), true);
+
+  Verifier verifier_small_structs(builder.GetBufferPointer(),
+                                  builder.GetSize());
+  TEST_ASSERT(verifier_small_structs.VerifyBuffer<SmallStructs>(nullptr));
+
+  // Reading SmallStructs vector values back and compare with original
+  auto root_msg =
+      flatbuffers::GetRoot<SmallStructs>(builder.GetBufferPointer());
+
+  TEST_EQ(root_msg->small_structs()->size(), small_vector.size());
+  for (flatbuffers::uoffset_t i = 0; i < root_msg->small_structs()->size();
+       ++i) {
+    TEST_EQ(small_vector[i].var_0(),
+            root_msg->small_structs()->Get(i)->var_0());
+    TEST_EQ(small_vector[i].var_1(),
+            root_msg->small_structs()->Get(i)->var_1());
+  }
 }
 
 }  // namespace tests

diff --git a/tests/alignment_test.fbs b/tests/alignment_test.fbs
@@ -21,4 +21,15 @@ table BadAlignmentRoot {
   small: [BadAlignmentSmall];
 }
 
-root_type BadAlignmentRoot;
+// sizeof(JustSmallStruct) == 2
+// alignof(JustSmallStruct) == 1
+struct JustSmallStruct {
+  var_0: uint8;
+  var_1: uint8;
+}
+
+table SmallStructs {
+  small_structs: [JustSmallStruct];
+}
+
+root_type SmallStructs;
diff --git a/tests/alignment_test.json b/tests/alignment_test.json
@@ -0,0 +1,16 @@
+{
+    "small_structs": [
+        {
+            "var_0": 2,
+            "var_1": 1
+        },
+        {
+            "var_0": 3,
+            "var_1": 1
+        },
+        {
+            "var_0": 4,
+            "var_1": 1
+        }
+    ]
+}
diff --git a/tests/alignment_test_after_fix.afb b/tests/alignment_test_after_fix.afb
@@ -0,0 +1,31 @@
+// Annotated Flatbuffer Binary
+//
+// Schema file: alignment_test.fbs
+// Binary file: alignment_test_after_fix.bin
+
+header:
+  +0x00 | 0C 00 00 00             | UOffset32  | 0x0000000C (12) Loc: 0x0C | offset to root table `SmallStructs`
+
+padding:
+  +0x04 | 00 00                   | uint8_t[2] | ..                        | padding
+
+vtable (SmallStructs):
+  +0x06 | 06 00                   | uint16_t   | 0x0006 (6)                | size of this vtable
+  +0x08 | 08 00                   | uint16_t   | 0x0008 (8)                | size of referring table
+  +0x0A | 04 00                   | VOffset16  | 0x0004 (4)                | offset to field `small_structs` (id: 0)
+
+root_table (SmallStructs):
+  +0x0C | 06 00 00 00             | SOffset32  | 0x00000006 (6) Loc: 0x06  | offset to vtable
+  +0x10 | 04 00 00 00             | UOffset32  | 0x00000004 (4) Loc: 0x14  | offset to field `small_structs` (vector)
+
+vector (SmallStructs.small_structs):
+  +0x14 | 03 00 00 00             | uint32_t   | 0x00000003 (3)            | length of vector (# items)
+  +0x18 | 02                      | uint8_t    | 0x02 (2)                  | struct field `[0].var_0` of 'JustSmallStruct' (UByte)
+  +0x19 | 01                      | uint8_t    | 0x01 (1)                  | struct field `[0].var_1` of 'JustSmallStruct' (UByte)
+  +0x1A | 03                      | uint8_t    | 0x03 (3)                  | struct field `[1].var_0` of 'JustSmallStruct' (UByte)
+  +0x1B | 01                      | uint8_t    | 0x01 (1)                  | struct field `[1].var_1` of 'JustSmallStruct' (UByte)
+  +0x1C | 04                      | uint8_t    | 0x04 (4)                  | struct field `[2].var_0` of 'JustSmallStruct' (UByte)
+  +0x1D | 01                      | uint8_t    | 0x01 (1)                  | struct field `[2].var_1` of 'JustSmallStruct' (UByte)
+
+padding:
+  +0x1E | 00 00                   | uint8_t[2] | ..                        | padding
diff --git a/tests/alignment_test_after_fix.bin b/tests/alignment_test_after_fix.bin
diff --git a/tests/alignment_test_before_fix.afb b/tests/alignment_test_before_fix.afb
@@ -0,0 +1,31 @@
+// Annotated Flatbuffer Binary
+//
+// Schema file: alignment_test.fbs
+// Binary file: alignment_test_before_fix.bin
+
+header:
+  +0x00 | 0C 00 00 00             | UOffset32   | 0x0000000C (12) Loc: 0x0C | offset to root table `SmallStructs`
+
+padding:
+  +0x04 | 00 00                   | uint8_t[2]  | ..                        | padding
+
+vtable (SmallStructs):
+  +0x06 | 06 00                   | uint16_t    | 0x0006 (6)                | size of this vtable
+  +0x08 | 08 00                   | uint16_t    | 0x0008 (8)                | size of referring table
+  +0x0A | 04 00                   | VOffset16   | 0x0004 (4)                | offset to field `small_structs` (id: 0)
+
+root_table (SmallStructs):
+  +0x0C | 06 00 00 00             | SOffset32   | 0x00000006 (6) Loc: 0x06  | offset to vtable
+  +0x10 | 04 00 00 00             | UOffset32   | 0x00000004 (4) Loc: 0x14  | offset to field `small_structs` (vector)
+
+vector (SmallStructs.small_structs):
+  +0x14 | 03 00 00 00             | uint32_t    | 0x00000003 (3)            | length of vector (# items)
+  +0x18 | 00                      | uint8_t     | 0x00 (0)                  | struct field `[0].var_0` of 'JustSmallStruct' (UByte)
+  +0x19 | 00                      | uint8_t     | 0x00 (0)                  | struct field `[0].var_1` of 'JustSmallStruct' (UByte)
+  +0x1A | 02                      | uint8_t     | 0x02 (2)                  | struct field `[1].var_0` of 'JustSmallStruct' (UByte)
+  +0x1B | 01                      | uint8_t     | 0x01 (1)                  | struct field `[1].var_1` of 'JustSmallStruct' (UByte)
+  +0x1C | 03                      | uint8_t     | 0x03 (3)                  | struct field `[2].var_0` of 'JustSmallStruct' (UByte)
+  +0x1D | 01                      | uint8_t     | 0x01 (1)                  | struct field `[2].var_1` of 'JustSmallStruct' (UByte)
+
+unknown (no known references):
+  +0x1E | 04 01                   | ?uint8_t[2] | ..                        | WARN: could be corrupted padding region.
diff --git a/tests/alignment_test_before_fix.bin b/tests/alignment_test_before_fix.bin