Add null terminators to strings (#36)

* Add null terminators to strings

* Adds null terminators to the end of written strings to fix issue #16
* Adds optional null terminator validation while deserializing gated
  behind a new `extra-validation` feature flag.
* Adds extensive testing for the fix.
* Slightly changes the debug output for deserialized tables.

* Add safety comments

planus-cli/src/codegen/templates/rust/table.template

@@ -121,13 +121,11 @@ impl<'a> ::core::fmt::Debug for {{info.ref_name}}<'a> {
         let mut f = f.debug_struct("{{info.ref_name}}");
         {% for field in fields.declaration_order() -%}
             {%- if field.info.read_type.starts_with("::core::option::Option<") -%}
-            if let ::core::result::Result::Ok(::core::option::Option::Some({{field.info.name}})) = self.{{field.info.name}}() {
+            if let ::core::option::Option::Some({{field.info.name}}) = self.{{field.info.name}}().transpose() {
                 f.field("{{field.info.name}}", &{{field.info.name}});
             }
             {%- else -%}
-            if let ::core::result::Result::Ok({{field.info.name}}) = self.{{field.info.name}}() {
-                f.field("{{field.info.name}}", &{{field.info.name}});
-            }
+            f.field("{{field.info.name}}", &self.{{field.info.name}}());
             {%- endif -%}
         {%- endfor %}
         f.finish()

planus/Cargo.toml

@@ -19,3 +19,4 @@ rand = "0.8.4"
 [features]
 default = ["std"]
 std = []
+extra-validation = []

planus/src/errors.rs

@@ -18,6 +18,7 @@ impl std::error::Error for Error {
 }
 
 #[derive(Copy, Clone, Debug)]
+#[non_exhaustive]
 pub enum ErrorKind {
     InvalidOffset,
     InvalidLength,
@@ -26,6 +27,7 @@ pub enum ErrorKind {
     InvalidVtableLength { length: u16 },
     InvalidUtf8 { source: core::str::Utf8Error },
     MissingRequired,
+    MissingNullTerminator,
 }
 
 impl core::fmt::Display for ErrorKind {
@@ -40,6 +42,7 @@ impl core::fmt::Display for ErrorKind {
             }
             ErrorKind::InvalidUtf8 { source } => write!(f, "Invalid utf-8: {}", source),
             ErrorKind::MissingRequired => write!(f, "Missing required field"),
+            ErrorKind::MissingNullTerminator => write!(f, "Missing null terminator"),
         }
     }
 }
@@ -55,6 +58,7 @@ impl std::error::Error for ErrorKind {
             ErrorKind::InvalidVtableLength { .. } => None,
             ErrorKind::InvalidUtf8 { source } => Some(source),
             ErrorKind::MissingRequired => None,
+            ErrorKind::MissingNullTerminator => None,
         }
     }
 }

planus/src/lib.rs

@@ -12,7 +12,7 @@ pub mod table_reader;
 #[doc(hidden)]
 pub mod table_writer;
 
-use alloc::{borrow::Cow, boxed::Box, string::String, vec::Vec};
+use alloc::{boxed::Box, string::String, vec::Vec};
 use core::{convert::TryInto, marker::PhantomData, mem::MaybeUninit};
 
 pub use errors::Error;
@@ -903,10 +903,16 @@ where
         for v in self.iter() {
             tmp.push(v.prepare(builder));
         }
+        // SAFETY: We need to make sure we always write the 4+stride*len bytes in the closure
         unsafe {
+            // TODO: This will not be correctly aligned if P::ALIGNMENT_MASK is bigger than u32::ALIGNMENT_MASK
             builder.write_with(
-                4 + T::STRIDE.checked_mul(self.len()).unwrap(),
-                P::ALIGNMENT_MASK.max(3),
+                T::STRIDE
+                    .checked_mul(self.len())
+                    .unwrap()
+                    .checked_add(4)
+                    .unwrap(),
+                P::ALIGNMENT_MASK.max(u32::ALIGNMENT_MASK),
                 |buffer_position, bytes| {
                     let bytes = bytes.as_mut_ptr();
 
@@ -918,7 +924,7 @@ where
                     T::write_values(&tmp, bytes.add(4), buffer_position - 4);
                 },
             )
-        };
+        }
         builder.current_offset()
     }
 }
@@ -1069,11 +1075,29 @@ impl WriteAsOptional<Offset<str>> for str {
 impl WriteAsOffset<str> for str {
     #[inline]
     fn prepare(&self, builder: &mut Builder) -> Offset<str> {
-        let offset = <[u8] as WriteAsOffset<[u8]>>::prepare(self.as_bytes(), builder);
-        Offset {
-            offset: offset.offset,
-            phantom: PhantomData,
+        let size_including_len_and_null = self.len().checked_add(5).unwrap();
+        // SAFETY: We make sure to write the 4+len+1 bytes inside the closure.
+        unsafe {
+            builder.write_with(
+                size_including_len_and_null,
+                u32::ALIGNMENT_MASK,
+                |buffer_position, bytes| {
+                    let bytes = bytes.as_mut_ptr();
+
+                    (self.len() as u32).write(
+                        Cursor::new(&mut *(bytes as *mut [MaybeUninit<u8>; 4])),
+                        buffer_position,
+                    );
+                    std::ptr::copy_nonoverlapping(
+                        self.as_bytes().as_ptr() as *const MaybeUninit<u8>,
+                        bytes.add(4),
+                        self.len(),
+                    );
+                    bytes.add(4 + self.len()).write(MaybeUninit::new(0));
+                },
+            )
         }
+        builder.current_offset()
     }
 }
 
@@ -1096,6 +1120,10 @@ impl<'buf> VectorRead<'buf> for str {
         let add_context =
             |e: ErrorKind| e.with_error_location("[str]", "get", buffer.offset_from_start);
         let (slice, len) = array_from_buffer(buffer, offset).map_err(add_context)?;
+        #[cfg(feature = "extra-validation")]
+        if slice.as_slice().get(len) != Some(&0) {
+            return Err(add_context(ErrorKind::MissingNullTerminator));
+        }
         let slice = slice
             .as_slice()
             .get(..len)
@@ -1138,7 +1166,15 @@ impl<'buf> TableRead<'buf> for &'buf str {
         buffer: SliceWithStartOffset<'buf>,
         offset: usize,
     ) -> core::result::Result<Self, ErrorKind> {
-        let slice: &[u8] = TableRead::from_buffer(buffer, offset)?;
+        let (buffer, len) = array_from_buffer(buffer, offset)?;
+        #[cfg(feature = "extra-validation")]
+        if buffer.as_slice().get(len) != Some(&0) {
+            return Err(ErrorKind::MissingNullTerminator);
+        }
+        let slice = buffer
+            .as_slice()
+            .get(..len)
+            .ok_or(ErrorKind::InvalidLength)?;
         Ok(core::str::from_utf8(slice)?)
     }
 }
@@ -1223,13 +1259,3 @@ impl<'buf> TableRead<'buf> for &'buf [u8] {
         buffer.as_slice().get(..len).ok_or(ErrorKind::InvalidLength)
     }
 }
-
-impl<'buf> TableRead<'buf> for Cow<'buf, str> {
-    fn from_buffer(
-        buffer: SliceWithStartOffset<'buf>,
-        offset: usize,
-    ) -> core::result::Result<Self, ErrorKind> {
-        let bytes = <&'buf [u8] as TableRead<'buf>>::from_buffer(buffer, offset)?;
-        Ok(String::from_utf8_lossy(bytes))
-    }
-}

test/rust/Cargo.toml

@@ -4,12 +4,12 @@ version = "0.1.0"
 edition = "2021"
 license = "MIT/Apache-2.0"
 repository = "https://github.com/TethysSvensson/planus"
-build="build.rs"
+build = "build.rs"
 rust-version = "1.57"
 
 [dependencies]
 anyhow = "1.0.51"
-planus = { path = "../../planus" }
+planus = { path = "../../planus", features = ["extra-validation"] }
 flatbuffers = "0.8.4"
 serde = "1.0.132"
 

test/rust/test_files/deprecated/deserialize/contains_deprecated_data.txt

@@ -1,4 +1,8 @@
 RootRef {
-    x: 1,
-    z: 3,
+    x: Ok(
+        1,
+    ),
+    z: Ok(
+        3,
+    ),
 }

test/rust/test_files/deprecated/deserialize/simple.txt

@@ -1,4 +1,8 @@
 RootRef {
-    x: 1,
-    z: 2,
+    x: Ok(
+        1,
+    ),
+    z: Ok(
+        2,
+    ),
 }

test/rust/test_files/enums/deserialize/change_uint13_Y.txt

@@ -1,33 +1,95 @@
 RootRef {
-    field_uint1: X,
-    field_uint2: X,
-    field_uint3: X,
-    field_uint4: X,
-    field_uint5: X,
-    field_uint6: X,
-    field_uint7: X,
-    field_uint8: X,
-    field_uint10: X,
-    field_uint11: Y,
-    field_uint12: Z,
-    field_uint13: Y,
-    field_uint14: X,
-    field_uint15: Y,
-    field_uint16: X,
-    field_int1: X,
-    field_int2: X,
-    field_int3: X,
-    field_int4: X,
-    field_int5: X,
-    field_int6: X,
-    field_int7: X,
-    field_int8: X,
-    field_int10: X,
-    field_int11: Y,
-    field_int12: Z,
-    field_int13: X,
-    field_int14: X,
-    field_int15: Y,
-    field_int16: X,
-    filler: 1,
+    field_uint1: Ok(
+        X,
+    ),
+    field_uint2: Ok(
+        X,
+    ),
+    field_uint3: Ok(
+        X,
+    ),
+    field_uint4: Ok(
+        X,
+    ),
+    field_uint5: Ok(
+        X,
+    ),
+    field_uint6: Ok(
+        X,
+    ),
+    field_uint7: Ok(
+        X,
+    ),
+    field_uint8: Ok(
+        X,
+    ),
+    field_uint10: Ok(
+        X,
+    ),
+    field_uint11: Ok(
+        Y,
+    ),
+    field_uint12: Ok(
+        Z,
+    ),
+    field_uint13: Ok(
+        Y,
+    ),
+    field_uint14: Ok(
+        X,
+    ),
+    field_uint15: Ok(
+        Y,
+    ),
+    field_uint16: Ok(
+        X,
+    ),
+    field_int1: Ok(
+        X,
+    ),
+    field_int2: Ok(
+        X,
+    ),
+    field_int3: Ok(
+        X,
+    ),
+    field_int4: Ok(
+        X,
+    ),
+    field_int5: Ok(
+        X,
+    ),
+    field_int6: Ok(
+        X,
+    ),
+    field_int7: Ok(
+        X,
+    ),
+    field_int8: Ok(
+        X,
+    ),
+    field_int10: Ok(
+        X,
+    ),
+    field_int11: Ok(
+        Y,
+    ),
+    field_int12: Ok(
+        Z,
+    ),
+    field_int13: Ok(
+        X,
+    ),
+    field_int14: Ok(
+        X,
+    ),
+    field_int15: Ok(
+        Y,
+    ),
+    field_int16: Ok(
+        X,
+    ),
+    filler: Ok(
+        1,
+    ),
 }

test/rust/test_files/ids/deserialize/simple.txt

@@ -1,10 +1,20 @@
 RootRef {
-    v1: 1,
-    v2: 2,
-    v3: InnerType(
-        InnerTypeRef {
-            v3_inner: 3,
-        },
-    ),
-    v4: 4,
+    v1: Ok(
+        1,
+    ),
+    v2: Ok(
+        2,
+    ),
+    v3: Ok(
+        InnerType(
+            InnerTypeRef {
+                v3_inner: Ok(
+                    3,
+                ),
+            },
+        ),
+    ),
+    v4: Ok(
+        4,
+    ),
 }

test/rust/test_files/string_list.fbs

@@ -0,0 +1,3 @@
+table Root {
+  x: [string];
+}

test/rust/test_files/string_list/deserialize/invalid_0_0.txt

@@ -0,0 +1,19 @@
+RootRef {
+    x: Ok(
+        [
+            Err(
+                Error {
+                    source_location: ErrorLocation {
+                        type_: "[str]",
+                        method: "get",
+                        byte_offset: 24,
+                    },
+                    error_kind: MissingNullTerminator,
+                },
+            ),
+            Ok(
+                "",
+            ),
+        ],
+    ),
+}