[C#] Verify the file identifier instead of skipping the check - #9211
Open
Nexory wants to merge 1 commit into
Open
[C#] Verify the file identifier instead of skipping the check#9211Nexory wants to merge 1 commit into
Nexory wants to merge 1 commit into
Conversation
CheckBufferFromStart() gated the identifier comparison on
identifier.Length == 0, so the comparison could only run when the caller
passed an empty identifier. VerifyBuffer() rejects an empty identifier
earlier with ArgumentException, so the branch was unreachable: a buffer
carrying a wrong identifier, or none at all, verified as valid.
The contract documented in the same file states the opposite: "When empty
identifier is provided the identifier validation is skipped." The C++
verifier (verifier.h) and Swift (Root.swift) both check. flatc emits
VerifyBuffer("", ...) for schemas that declare no file_identifier, which
today throws instead of skipping.
Flip the condition to identifier.Length != 0 and cover matching,
mismatched, missing and empty identifiers with tests.
TestVerifyingUnions finished its buffer with fbb.Finish(root), omitting
the "MONS" identifier, while asserting that Monster.VerifyMonster()
returns true. That assertion held only because the check never ran, so it
now uses Monster.FinishMonsterBuffer().
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
1 participant
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.
Verifier.VerifyBuffer(identifier, ...)never compares the file identifier, andfor a schema without a
file_identifierit throws instead of verifying.CheckBufferFromStartgates the comparison onidentifier.Length == 0.VerifyBufferrejects an empty identifier earlier withArgumentException, sothe guarded branch is unreachable and both cases below fall through to "valid".
The contract documented further down in the same file says the opposite:
"identifier - the expected identifier of buffer data. When empty identifier is
provided the identifier validation is skipped."
C++ (
verifier.h) and Swift (Root.swift) both check.Measured on master 5761d6e, before the change:
VerifyBuffer("")on a valid bufferArgumentException: file identifier must be length4The last row is what generated code hits: flatc emits
VerifyBuffer("", ...)forschemas that declare no
file_identifier, for exampletests/type_field_collsion/Collision.csandgoldens/csharp/flatbuffers/goldens/Universe.cs. That is issue #8002, which thestale bot closed without an answer.
The fix flips the condition to
identifier.Length != 0.One existing test changes with it.
TestVerifyingUnionsfinished its buffer withfbb.Finish(monster_outer.Value), leaving out the "MONS" identifier, and thenasserted that
Monster.VerifyMonster(...)returns true. That assertion held onlybecause the check never ran, so it now uses
Monster.FinishMonsterBuffer.This is a behavior change worth calling out: code that verifies a buffer against
an identifier the buffer does not carry now gets false where it got true before.
That is the documented contract, but it can surface in existing callers.
Tests: 4 cases covering a matching, mismatched, missing and empty identifier.
With the fix the suite reports 160 tests run, 0 failed; without it 3 of the 4
fail. On master without the new file it reports 156 tests run, 0 failed, so
nothing else changes. I ran the build and test matrix from the workflow (default,
UnsafeByteBuffer, EnableSpanT plus UnsafeByteBuffer) for net8.0 and net6.0.
I left Rust alone: its verifier takes no identifier parameter at all, which is a
separate gap rather than the same bug.