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 rustsec/advisory-db#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:

flatbuffers/tests/include_test/sub/include_test2_generated.rs

Lines 50 to 63 in 6da1cf7

	impl flatbuffers::EndianScalar for FromInclude {
	#[inline]
	fn to_little_endian(self) -> Self {
	let n = i64::to_le(self as i64);
	let p = &n as *const i64 as *const FromInclude;
	unsafe { *p }
	}
	#[inline]
	fn from_little_endian(self) -> Self {
	let n = i64::from_le(self as i64);
	let p = &n as *const i64 as *const FromInclude;
	unsafe { *p }
	}
	}

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).

[rockstar]

#6393 claims to have fixed this issue. Is that correct? Did it fix this issue?

[eduardosm]

It didn't. I left some comments there.

[krojew]

@rw @CasperN any info on this? cargo audit can block flatbuffers in CI.

[CasperN]

iirc, the consensus was that we should mark it as unsafe due to the lack of bounds checking (which is done in the verifier).

[nico-abram]

What's the intended (public API) usage of the 2 read_scalar and the emplace_scalar functions? Are they not something that should be private/internal?

@eduardosm Could you explain how emplace_scalar allows unaligned writes? (I only see it writing to the &mut [u8] and afaik that has no alignment requirements). It does have problems for types that can only be represented as T in native endiannes, but that would be fixed by Casper's fn emplace_little_endian(self, buf: &mut [u8]); suggestion if I understand corretly

I see another problem with emplace_scalar: It doesn't seem to be checking the length of the slice to ensure we're not writing out of bounds. Would the check be detrimental to performance? If yes, it should be made unsafe, and probably renamed to something like emplace_scalar_unchecked (And maybe add a safe version that checks the length of the slice called emplace_scalar)

Example:

fn main() {
    flatbuffers::emplace_scalar(&mut [], 0u8);
}

[CasperN]

Its meant to be internal. Like most of the flatbuffers crate, its to be used by our generated code.

It doesn't seem to be checking the length of the slice to ensure we're not writing out of bounds. Would the check be detrimental to performance? If yes, it should be made unsafe, and probably renamed to something like emplace_scalar_unchecked (And maybe add a safe version that checks the length of the slice called emplace_scalar)

Yep, I think that's the plan, though no one is working on that atm.