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

[Naios]

• f124e41
• C++ API
• MSVC 2019 (16.11.17) is probably applicable to Clang and GCC as well

---

I'm encountering a rare issue where a completely valid buffer gets generated into badly aligned memory locations.
This behavior is hard to reproduce through a minimalistic test case, but I will explain in detail what probably happens here.

To trigger the issue we need two structs, one with a large size and small alignment (BadAlignmentSmall) and another one with a smaller size and large alignment (BadAlignmentLarge) that are somehow written to our flatbuffer:

struct BadAlignmentSmall {
  var_0: uint;
  var_1: uint;
  var_2: uint;
};

// sizeof(BadAlignmentSmall) == 12
// alignof(BadAlignmentSmall) == 4

struct BadAlignmentLarge {
  var_0: ulong;
};

// sizeof(BadAlignmentLarge) == 8
// alignof(BadAlignmentLarge) == 8

Regardless of the buffer_minalign variable in vector_downward, the final flatbuffers buffer gets aligned by the alignment tracked through FlatBufferBuilder::minalign_ which is recorded through TrackMinAlign (by using the upper bound max of its input value and the current minalign_).

  void Align(size_t elem_size) {
    TrackMinAlign(elem_size);
    buf_.fill(PaddingBytes(buf_.size(), elem_size));
  }

In the flatbuffers codebase we find the following calls to Align:

• Align(sizeof(T));
• 2x Align(AlignOf<T>());

Because Align is called with the size and the alignment in different places it can happen that minalign_ is finally 12 which is incompatible with our real required alignment of 8.
This causes the verifier with enabled alignment checks to fail because the buffer gets misaligned on the final call to Finish().

To fix this we have two options:

• replace Align(sizeof(T)) by Align(alignof(T)) (or Align(AlignOf<T>()) respectively), if this was a typo
• Use a better function to union two alignments (e.g. max common power of two), max should not be used for that in case you pass the size to Align (size 12 & alignment 8 should result in alignment 16, not 12) - assuming 16 would be a valid real-world alignment.

  void TrackMinAlign(size_t elem_size) {
    if (elem_size > minalign_)  minalign_ = elem_size;
  }

[dbaileychess]

Yes, the Align(sizeof(T)) seems like it can be changed to Align(AlignOf<T>()) as we align to the largest type of a struct, or to the size of the item if not a struct, which AlignOf does.

Would you like to make a PR and add some tests?

[Naios]

I was able ro reproduce the issue see the following stacktrace how minalign_ gets set to 12:

 	flatbuffers::FlatBufferBuilder::TrackMinAlign(unsigned __int64 12) Line 250	C++
 	flatbuffers::FlatBufferBuilder::PreAlign(unsigned __int64 108, unsigned __int64 12) Line 452	C++
 	flatbuffers::FlatBufferBuilder::StartVector(unsigned __int64 9, unsigned __int64 12) Line 601	C++
 	flatbuffers::FlatBufferBuilder::CreateUninitializedVector(unsigned __int64 9, unsigned __int64 12, unsigned char * * 0x000000b7296fe148) Line 1035	C++
>	flatbuffers::FlatBufferBuilder::CreateUninitializedVectorOfStructs<BadAlignmentSmall>(unsigned __int64 9, BadAlignmentSmall * * 0x000000b7296fe148) Line 1060	C++

See #7520 for the test case.

Unfortunatly this is not related to Align(sizeof(T)) directly. It is set by

  void StartVector(size_t len, size_t elemsize) {
    NotNested();
    nested = true;
    PreAlign<uoffset_t>(len * elemsize);
    PreAlign(len * elemsize, elemsize);  // Just in case elemsize > uoffset_t.
  }

Calling

  void PreAlign(size_t len, size_t alignment) {
    if (len == 0) return;
    TrackMinAlign(alignment);
    buf_.fill(PaddingBytes(GetSize() + len, alignment));
  }

with alignment = elemsize

Probably this can be fixed by adding the alignment to the StartVector call, do you agree?

---

// sizeof(BadAlignmentSmall) == 12
// alignof(BadAlignmentSmall) == 4
struct BadAlignmentSmall {
  var_0: uint;
  var_1: uint;
  var_2: uint;
}

table OuterSmall {
  small: 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;

[CasperN]

Rust has a similar problem; #7531