[Rust] Verify the size prefix before skipping it - #9205
Open
Nexory wants to merge 1 commit into
Open
Conversation
size_prefixed_root verifies the root that the size prefix points at, but never reads the prefix itself: SkipSizePrefix::run_verifier advances four bytes and hands over. So a buffer whose prefix promises more bytes than the buffer holds verifies, and the caller gets an accessor for it. The three other verifiers in this repository read the prefix. C++ does so in VerifySizePrefixedBuffer, C# in Verifier.VerifyBuffer (the class in net/FlatBuffers/FlatBufferVerify.cs), and Swift since google#9196, where the review asked for the same convention and linked verifier.h L256. The check mirrors the C++ one, which is an upper bound rather than an equality, so a prefix smaller than the slice keeps working: a stream of size prefixed buffers passed as one slice, or a larger read buffer, still verifies. One behaviour does shift. get_uoffset counts the four prefix bytes towards apparent_size, which nothing read before, so the counter sits four higher: the smallest max_apparent_size that accepts a given 148 byte buffer moves from 251 to 255. The default is 1 << 31. Delegating to range_in_buffer instead would move it to 399, because that books the whole promised size on top of the payload the verifier walks anyway. The bounds check is written out rather than delegated to range_in_buffer, because that would add the whole promised size to apparent_size and fail a large but legitimate prefix with ApparentSizeTooLarge. reflection shares the entry point through size_prefixed_root_as_schema. flatc --size-prefixed writes schemas that way and reads them back through VerifySizePrefixedSchemaBuffer, so the C++ side of that round trip checks the prefix today and the Rust side does not. The test sits in tests/rust_usage_test because that is what RustTest.sh runs; the crate's own unit tests are not part of it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
size_prefixed_rootverifies the root that the size prefix points at, but neverreads the prefix itself.
SkipSizePrefix::run_verifieradvances four bytes andhands over:
So a buffer whose prefix promises more bytes than the buffer holds verifies, and
the caller gets an accessor for it. The three other verifiers in this repository
read the prefix: C++ in
VerifySizePrefixedBuffer(
include/flatbuffers/verifier.h:256), C# inVerifier.VerifyBuffer(net/FlatBuffers/FlatBufferVerify.cs:805), and Swiftsince #9196, where the review asked for it in these words: "it would be better if the
swift implementation also follows a similar convention. Where we only skip after
we have verified the prefixed value too", linking
verifier.h#L256. This patchdoes the same for Rust.
The doc comment on
size_prefixed_rootalready says verification "may not bemaximally performant or catch every error (though that is the goal)". This is
one of those errors, and the parenthesis names catching them as the aim.
What it looks like
One schema,
table Doc { text:string; }withroot_type Doc, built withFinishSizePrefixedand holding "hello": 36 bytes, correct prefix 32. Only thefirst four bytes differ between the files below, the remaining 32 are identical:
VerifySizePrefixedDocBufferSome("hello")Some("hello")The third column is measured before the patch and the fourth after it, on the
same five files. Rust then agrees with C++ on all five.
The fix
The check mirrors the C++ one, which is an upper bound rather than an equality.
That matters, so I measured what stays accepted. Building one buffer and running
it through
size_prefixed_rootbefore and after the patch:len - 4, correctlen, four too manyA prefix smaller than the slice keeps working, which is what a caller passing a
stream or a larger read buffer relies on. C# requires exact equality here and
would reject those; this patch follows C++.
Two details a reviewer will probably ask about, both measured on the same 148
byte buffer by binary searching the smallest
max_apparent_sizeit stillverifies under:
max_apparent_sizev.range_in_buffer(start, size)insteadThe bounds check is written out rather than delegated to
range_in_bufferbecause that call books the whole promised
sizeagainstapparent_size, whichis the 399 above: the payload would be counted twice. What remains is the four
bytes
get_uoffsetbooks for the prefix itself, which nothing read before, sothe counter sits four higher. At the default of
1 << 31that only matters fora buffer whose apparent size comes within four bytes of the limit, but it is a
change, not nothing.
reflectionshares the entry point throughsize_prefixed_root_as_schema, whichforwards to
size_prefixed_root.flatc --size-prefixedwrites schemas that way(
FinishSizePrefixedinidl_parser.cpp) and reads them back throughVerifySizePrefixedSchemaBuffer, so the C++ side of that round trip checks theprefix today and the Rust side does not. On a 244 byte
.bfbswritten byflatc --size-prefixed, the unmodified file is accepted before and after thispatch, while the same file with the prefix set to 1000000 goes from accepted to
rejected.
Test
The test sits in
tests/rust_usage_test, next to the existingtest_size_prefixed_buffer, because that is whatRustTest.shruns; thelibrary's own unit tests are not part of it. Without the fix it reports
while
test_size_prefixed_bufferkeeps passing, so the two together show thecheck is narrow.
bash RustTest.shpasses: serde tests, no_std compilation,rust_usage_testinboth configurations, both heap allocation checks, and the clippy step: 634
passing test results across 29 binaries, which is 317 tests run twice because
the suite covers both configurations. Nothing fails. Two steps in that script
are conditional and did not run here: the
outdirtests need a builtflatcnext to the checkout, and the miri run needs
RUST_NIGHTLY=1. Running clippydirectly on
rust/flatbuffersbefore and after the patch gives the same set ofmessages, with one pre-existing warning shifted by eleven lines.