Fix alignment of large structs in unions (#289)

The template the generates the `planus::WriteAsOffset` impl for structs
(inside the file `planus-codegen/src/templates/rust/struct.template`)
called the function `builder.write_with` using an `alignment` argument
instead of an `alignment_mask` argument. This meant that the
calculations for how many bytes of padding is needed for this struct
will be wrong.

This can cause too little padding for any structs that was put inside
unions, and additionally it could cause more than strictly necessary
amounts of padding for all other subsequently serialized values.

This bug did **not** cause any undefined behavior inside planus itself.
Other programs were dependent on having valid requirements, then these
programs might not work correctly for data generated by planus.

All code affected by this problem will panic if run with debug
assertions on.

Author: TethysSvensson

CHANGELOG.md

@@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- \[Rust\]: Fix the alignment of structs in unions [#289](https://github.com/planus-org/planus/pull/289)
+
 ### Removed
 
 ## [1.1.0] - 2025-03-02

RELEASE-CHECKLIST.md

@@ -5,6 +5,7 @@
 - Grep for the old version number
 - Run `cargo make regenerate-examples`
 - Update CHANGELOG.md, including links at the bottom
+- Remember to update `Cargo.lock`
 - Commit changes to a branch and make a PR
 - Run `cargo publish --dry-run` on PR branch
 - Wait for CI and merge PR to main branch

crates/planus-codegen/src/templates/rust/struct.template

@@ -43,7 +43,7 @@ impl ::planus::WriteAsOffset<{{ info.owned_name }}> for {{ info.owned_name }} {
         unsafe {
             builder.write_with(
                 {{ size }},
-                {{ alignment }},
+                {{ alignment - 1 }},
                 |buffer_position, bytes| {
                     let bytes = bytes.as_mut_ptr();
 

examples/rust/src/monster_generated.rs

@@ -462,7 +462,7 @@ mod root {
                 #[inline]
                 fn prepare(&self, builder: &mut ::planus::Builder) -> ::planus::Offset<Vec3> {
                     unsafe {
-                        builder.write_with(12, 4, |buffer_position, bytes| {
+                        builder.write_with(12, 3, |buffer_position, bytes| {
                             let bytes = bytes.as_mut_ptr();
 
                             ::planus::WriteAsPrimitive::write(

test/Cargo.lock

@@ -0,0 +1,12 @@
+struct Struct {
+  value: uint64;
+}
+
+union Union {
+  Struct,
+  String: string
+}
+
+table Root {
+  unions: [Union];
+}

test/rust-test-2021/api_files/union_vector_iteration.fbs

@@ -0,0 +1,89 @@
+use planus::ReadAsRoot;
+
+#[track_caller]
+fn eq<I1, I2>(iter1: I1, iter2: I2)
+where
+    I1: Clone + Iterator + ExactSizeIterator,
+    I2: Clone + Iterator + ExactSizeIterator,
+    I1::Item: core::fmt::Debug + PartialEq<I2::Item>,
+    I2::Item: core::fmt::Debug,
+{
+    if !iter1.clone().eq(iter2.clone()) {
+        let iter1 = iter1.collect::<Vec<_>>();
+        let iter2 = iter2.collect::<Vec<_>>();
+        panic!(
+            "Not equal: {iter1:?} {iter2:?}",
+            iter1 = iter1,
+            iter2 = iter2
+        );
+    }
+    let mut len = iter2.len();
+    assert_eq!(len, iter1.len());
+    let mut iter2 = iter2;
+    while let Some(_) = iter2.next() {
+        assert_eq!(iter2.len(), len - 1);
+        len -= 1;
+    }
+}
+
+fn to_owned_vec<'buf>(vec: planus::UnionVector<'buf, UnionRef<'buf>>) -> Vec<Union> {
+    vec.iter().map(|v| v.unwrap().try_into().unwrap()).collect()
+}
+
+#[track_caller]
+fn cmp<I1, I2, F, T>(iter1: I1, iter2: I2, f: F)
+where
+    I1: Clone + Iterator + DoubleEndedIterator + ExactSizeIterator,
+    I2: Clone + Iterator + DoubleEndedIterator + ExactSizeIterator,
+    I1::Item: core::fmt::Debug + PartialEq<T>,
+    F: Copy + Fn(I2::Item) -> T,
+    T: core::fmt::Debug,
+{
+    eq(iter1.clone(), iter2.clone().map(f));
+    eq(iter1.clone().rev(), iter2.clone().rev().map(f));
+    for i in 1..=iter1.len() + 1 {
+        eq(iter1.clone().step_by(i), iter2.clone().step_by(i).map(f));
+        eq(
+            iter1.clone().step_by(i).rev(),
+            iter2.clone().step_by(i).rev().map(f),
+        );
+        eq(
+            iter1.clone().rev().step_by(i),
+            iter2.clone().rev().step_by(i).map(f),
+        );
+    }
+}
+
+fn run_test(values: &[Union]) {
+    let mut builder = planus::Builder::new();
+    for i in 0..=values.len() {
+        builder.clear();
+        let root = Root::create(&mut builder, &values[..i]);
+        let data = builder.finish(root, None);
+        let root = RootRef::read_as_root(data).unwrap();
+        let vector = root.unions().unwrap().unwrap();
+        let owned: Vec<Union> = to_owned_vec(vector);
+        assert_eq!(owned, &values[..i]);
+
+        cmp(
+            owned.iter().cloned(),
+            vector.iter(),
+            |u: Result<UnionRef<'_>, _>| -> Union { u.unwrap().try_into().unwrap() },
+        );
+
+        for i in 1..=values.len() + 1 {
+            cmp(owned.chunks(i), vector.chunks(i), to_owned_vec);
+            cmp(owned.chunks_exact(i), vector.chunks_exact(i), to_owned_vec);
+            cmp(owned.rchunks(i), vector.rchunks(i), to_owned_vec);
+            cmp(
+                owned.rchunks_exact(i),
+                vector.rchunks_exact(i),
+                to_owned_vec,
+            );
+            cmp(owned.windows(i), vector.windows(i), to_owned_vec);
+        }
+    }
+}
+
+run_test(&[1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12].map(|value| Union::Struct(Struct { value })));
+run_test(&[1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12].map(|value| Union::String(format!("{value}"))));

test/rust-test-2021/api_files/union_vector_iteration.rs

@@ -0,0 +1,7 @@
+struct Struct64 {
+  value: uint64;
+}
+
+table Root {
+  value: Struct64;
+}

test/rust-test-2021/test_files/struct64.fbs

@@ -0,0 +1,7 @@
+Root {
+    value: Some(
+        Struct64 {
+            value: 17,
+        },
+    ),
+}

test/rust-test-2021/test_files/struct64/serialize/value.bin

@@ -0,0 +1,4 @@
+obj    @ 0x04..0x10
+vtable @ 0x10..0x16
+field[0] @ 0x08..0x10:
+  11 00 00 00 00 00 00 00

test/rust-test-2021/test_files/struct64/serialize/value.dbg.txt

@@ -0,0 +1,5 @@
+{
+  "value": {
+    "value": 17
+  }
+}

test/rust-test-2021/test_files/struct64/serialize/value.dump.txt

@@ -0,0 +1,7 @@
+RootRef {
+    value: Ok(
+        Struct64Ref {
+            value: 17,
+        },
+    ),
+}

test/rust-test-2021/test_files/struct64/serialize/value.json

@@ -6,6 +6,7 @@ union Union {
   String: string,
   InnerTable,
   InnerStruct,
+  InnerStruct64,
 }
 
 table Root {
@@ -19,3 +20,7 @@ table InnerTable {
 struct InnerStruct {
   value: uint8;
 }
+
+struct InnerStruct64 {
+  value: uint64;
+}

test/rust-test-2021/test_files/struct64/serialize/value.rust-dbg.txt

@@ -0,0 +1,6 @@
+obj    @ 0x04..0x0d
+vtable @ 0x10..0x18
+field[0] @ 0x0c..0x0d:
+  04
+field[1] @ 0x08..0x0c:
+  10 00 00 00

test/rust-test-2021/test_files_no_flatc/union.fbs

@@ -0,0 +1,7 @@
+{
+  "value": {
+    "InnerStruct64": {
+      "value": 17
+    }
+  }
+}

test/rust-test-2021/test_files_no_flatc/union/serialize/inner_struct64.bin

@@ -0,0 +1,9 @@
+RootRef {
+    value: Ok(
+        InnerStruct64(
+            InnerStruct64Ref {
+                value: 17,
+            },
+        ),
+    ),
+}