Rust soundness fixes

[tustvold]

There are various internal APIs exposed by the flatbuffer crate, and in the code it generates that are unsound, in that they permit undefined behaviour through safe APIs. It is extremely unlikely any downstreams are actually making use of these APIs and even less likely that they are doing so in ways that result in UB, however, the medium of RUSTSEC is not very effective to convey this subtlety. I think it is a shame, not to mention unfair, that this crate gets labelled "unsafe" or "dangerous" as a result of this, and so I'd like to do something to help fix this. Part of this is a selfish desire to stop having the same discussions repeatedly.

As originally proposed by @CasperN (https://github.com/google/flatbuffers/issues/6627#issuecomment-955808525) this makes the methods taking unvalidated &[u8] unsafe. The result is that if users are only using safe APIs, which force validation to be performed, they should be protected from UB => the crate is sound

I will endeavour to highlight the non-mechanical changes in the diff, but the major changes are:

• It makes the two trait methods Follow::follow and Push::push unsafe
• Push::push is provided with the already written length, but not the already written data (to help with optimisation of loops)
• It makes the various constructors Array::new, Table::new, Vector::new, VectorIter::from_slice, VTable::init unsafe
• buffer_has_identifier and the auto-generated *_buffer_has_identifier and *_size_prefixed will panic if given too small a buffer
• Removes SafeSliceAccess for types with alignment or value constraints (e.g. bool, u32, f32, etc...)
• Table accessors now always return Vector<'a, T> instead of sometimes returning slices

[google-cla[bot]]

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.

[alamb]

For context, the relevant rustsec issue is https://rustsec.org/advisories/RUSTSEC-2021-0122.html

[dbaileychess]

@CasperN I'll let you handle this one.

[dbaileychess]

@tustvold You'll need to sign the CLA as well

[tustvold]

I think this is now ready for another review, I have added safety comments to all unsafe blocks and I'm fairly confident that they are all sound. That being said I'm fairly confident that the amount of unsafe can be reduced without impacting performance, e.g. the scalar read/write logic is already performing bounds checks at a higher level, but I would rather do that as follow up PRs as this PR is enormous as it is (although a lot of that is generated code).

[dbaileychess]

Haven't been following this closely, is everything looking good @CasperN ?

[CasperN]

@dbaileychess, its almost ready. I can push the merge button once everyone LGTMs

[CasperN]

Are we all ready to merge?

[tustvold]

I'm happy if you're happy 😄 I don't think I've missed anything...

[CasperN]

I think this is great. Thank you for this PR!

@TethysSvensson @alamb any further comments? If not, I'll submit once the branch is up to date and CI is green

[TethysSvensson]

I don't have any more comments right now. I might have time to take another look for unsoundness at a later stage.

[tustvold]

Much approval :laughing:

I'm not sure what the release schedule for this repo is, but I at least would be very interested in testing out a pre-release build of this if such a thing were possible. This would help iron out any kinks that result from these changes

[CasperN]

We need both a release of flatc (@dbaileychess) and the flatbuffers crate to get this change out there

[tustvold]

For my purposes we vendor the generated code, and so I would be happy with just a pre-release of the crate, we already have instructions for compiling flatc from source (as the versions in most distributions are incredibly old)

[dbaileychess]

Release in: https://github.com/google/flatbuffers/releases/tag/v22.9.29 Haven't pushed to crates.io since I don't have permissions yet.

[tustvold]

This has a number of substantial breaking changes, it probably needs to be a major release. Sorry i should have made that clear

[y0nil]

Release in: https://github.com/google/flatbuffers/releases/tag/v22.9.29 Haven't pushed to crates.io since I don't have permissions yet.

Hi @dbaileychess / @tustvold,

Version v22.9.29 of flatc and version <= 2.1.2 of the flatbuffers crate are incompatible. Is there an estimation for when a matching version will be available on crates.io?

[CasperN]

I want #7553 to go in before pushing, but otherwise it can go in EoWeek

[tustvold]

No rush, but just wondering where we have got to with this?

[CasperN]

Ok, #7553 did not make it, I'll push the publish button (once I get #7574 merged)

[CasperN]

released!