Flatbuffers: Rust: read_scalar(_at) is unsound

[eduardosm]

The read_scalar and read_scalar_at functions are unsound because the allow to do things such as:

flatbuffers::read_scalar::<bool>(&[3]));

or

fn main() {
    #[derive(Copy, Clone, PartialEq, Debug)]
    struct Nz(std::num::NonZeroI32);
    impl flatbuffers::EndianScalar for Nz {
        fn to_little_endian(self) -> Self { self }
        fn from_little_endian(self) -> Self { self }
    }
    println!("{:?}", flatbuffers::read_scalar::<Nz>(&[0, 0, 0, 0]));
}

or even worse:

fn main() {
    #[derive(Copy, Clone, PartialEq, Debug)]
    struct S(&'static str);
    impl flatbuffers::EndianScalar for S {
        fn to_little_endian(self) -> Self { self }
        fn from_little_endian(self) -> Self { self }
    }
    println!("{:?}", flatbuffers::read_scalar::<S>(&[1; std::mem::size_of::<S>()]));
}

(this last one causes a segmentation fault)

read_scalar is behaving similar to transmute, leading to undefined behavior for types where not all bit patterns are valid (such as bool or NonZero*) or allowing to create dangling pointers.

I suggest three breaking solutions:

• Make read_scalar and read_scalar_at functions unsafe.
• Make the EndianScalar trait unsafe.
• Make the EndianScalar trait something like:

pub trait EndianScalar {
    fn emplace_little_endian(self, buf: &mut [u8]);
    fn read_little_endian(buf: &[u8]) -> Self;
}

which, for example, would be implemented for u32 like this:

impl EndianScalar for u32 {
    fn emplace_little_endian(self, buf: &mut [u8]) {
        buf[..4].copy_from_slice(&self.to_le_bytes());
    }
    fn read_little_endian(buf: &[u8]) -> u32 {
        u32::from_le_bytes([buf[0], buf[1], buf[2], buf[3]])
    }
}

This last solution, even though it is the most disrupting one, has the advantage of not requiring any unsafe at all.

[rw]

Thanks for finding this and for recommending solutions!

Do you think that the way we use this in generated code is unsound? It seems that we use it correctly in generated code; as such, adopting your solution of marking these functions as unsafe is the way to go.

I'll note that these functions are part of the public API but we don't expect anyone to use them manually, due to the code generation process flatc uses.

[rw]

Perhaps we could go all-in on zero-unsafe and interpret these values manually, without unsafe at all, like we do in Go: https://github.com/google/flatbuffers/blob/master/go/encode.go#L55

[eduardosm]

Looking at https://github.com/RustSec/advisory-db/issues/281, it seems that these functions also allow doing unaligned memory reads.

Looking at emplace_scalar, it is also unsound because it allows doing unaligned memory writes. It also allows reading the padding from structs (which I believe that are uninitialized).

Regarding the generated code, looking at:
https://github.com/google/flatbuffers/blob/6da1cf79d90eb242e7da5318241d42279a3df3ba/tests/include_test/sub/include_test2_generated.rs#L50-L63
Besides allowing to read invalid enum values (with read_scalar), it also allows swapping the byte order of enums in big endian machines.

I would go for zero-unsafe code. Reading and writing integers can be done as I suggested, and enums can use match to convert integer to enum and as to convert enum to integer.

I also think that it would great to get rid of unsafe from the generated code because it would allow consumers of flatbuffers to use forbid(unsafe_code).