Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from Sep 29, 2023

Conversation

bexcite
Copy link
Contributor

@bexcite bexcite commented Mar 29, 2023

Fixes the bug that was introduced by: #7520

Closes: #8024

The issue:
Small structs that are less than uoffset_t in size without any special alignment rules may result in a broken Vector layout that reads back with different values.

Example of the flatbuffers:

// sizeof(JustSmallStruct) == 2
// alignof(JustSmallStruct) == 1
struct JustSmallStruct {
  var_0: uint8;
  var_1: uint8;
}

table SmallStructs {
  small_structs: [JustSmallStruct];
}

now if a Vector of just 3 values of JustSmallStruct: { 2, 1 }, { 3, 1 }, { 4, 1 } is created with FlatBufferBuilder::CreateVectorOfStructs<T>() we get this binary Vector that is when read back get zeros in the first JustSmallStruct item:

03 00 00 00 00 00 02 01 03 01 04 01
          ^    ^
          |    |
  vector len   |
               |
         followed by two zero bytes of an alignment
         that was added because uoffset_t required
         4 bytes alignment!?

More confusing is that FlatBufferBuilder::CreateUninitializedVectorOfStructs<T>() method doesn't have this behavior and not adding "2 bytes zeros" before writing the vector len element.

The Fix:
This MR makes builder.CreateVectorOfStructs<T>() behaves correctly for small structures vectors and it's now the same as the FlatBufferBuilder::CreateUninitializedVectorOfStructs<T>().

@google-cla
Copy link

google-cla bot commented Mar 29, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@dbaileychess
Copy link
Collaborator

Can we add an annotated flatbuffer to this alignment test? (See docs). This would be useful to see how the data is laid out in the binary which is important. Ideally we would check in an annotated binary before this fix, so we can show how it gets fix (sort of like how you did in your PR description).

@bexcite
Copy link
Contributor Author

bexcite commented Apr 8, 2023

@dbaileychess thanks for the feedback, I'll look to the annotated flatbuffer things and will fix the build errors that I see on some compilers/OSs.

@bexcite bexcite requested a review from dbaileychess May 18, 2023 17:00
@bexcite
Copy link
Contributor Author

bexcite commented May 18, 2023

I should admit this alignment_test looks a little bit convoluted, because I decided to extend the original test that essentially broke the alignment ..., maybe it make sense to extract alignment_test related files to a separate folder?

@github-actions
Copy link

github-actions bot commented Jun 9, 2023

This pull request is stale because it has been open 3 weeks with no activity. Please comment or label not-stale, or this will be closed in 7 days.

@github-actions github-actions bot added the stale label Jun 9, 2023
@github-actions github-actions bot removed the stale label Jun 12, 2023
@bexcite
Copy link
Contributor Author

bexcite commented Jun 12, 2023

@dbaileychess I don't think that I can add not-stale label myself, but the PR seems still relevant. Could someone look at it and give a feedback what else I need to do to move it forward? As of now I think that everything was addressed, besides some explicitly left conversations/comments that I put there.

Thanks!

@github-actions
Copy link

github-actions bot commented Jul 4, 2023

This pull request is stale because it has been open 3 weeks with no activity. Please comment or label not-stale, or this will be closed in 7 days.

@bexcite
Copy link
Contributor Author

bexcite commented Jul 5, 2023

Added the issue, because I've realized that I posted a fix for the "non-existent" issue so people maybe not looking at it as a bug:

Closes: #8024

@github-actions github-actions bot removed the stale label Jul 5, 2023
tests/alignment_test_generated.h Outdated Show resolved Hide resolved
tests/alignment_test.json Show resolved Hide resolved
@dbaileychess dbaileychess merged commit 15f16f1 into google:master Sep 29, 2023
49 checks passed
@dbaileychess
Copy link
Collaborator

Sorry for the delay, was out on paternity leave.

@bexcite
Copy link
Contributor Author

bexcite commented Sep 30, 2023

Hooray! Thanks for accepting and congrats!

urshanselmann pushed a commit to komastudios/flatbuffers that referenced this pull request Jan 4, 2024
* 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>
candysonya pushed a commit to candysonya/flatbuffers that referenced this pull request Jan 8, 2024
* 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>
frc971-automation pushed a commit to frc971/971-Robot-Code that referenced this pull request Feb 17, 2024
This pulls in the functional fix in google/flatbuffers#7883

This fixes creation of vectors of structs with the "raw" flatbuffer API.
I observed this when writing tests for the static flatbuffer API and
discovering a discrepancy where the static flatbuffer API behaved
correctly and the existing API did not.

Change-Id: I124192b7f23cad4ae9b50366921ddb4e6a2590cd
Signed-off-by: James Kuszmaul <james.kuszmaul@bluerivertech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Misalignment of small structs in a Vector (C++)
2 participants