Repository navigation
Handle vector64 of structs when loading binary schemas and in reflection Verify - #9437
Open
professor-moody wants to merge 1 commit into
Open
professor-moody wants to merge 1 commit into
professor-moody wants to merge 1 commit into
Conversation
This branch has not been deployed
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.
A schema can declare a 64-bit vector of structs, for example
big_struct_vector:[LeafStruct] (vector64)intests/64bit/test_64bit.fbs, andflatc --schema -bwrites it into the binary schema with base typereflection::Vector64and the struct's object index. Two readers of binary schemas do not handle that base type yet.Loading a binary schema (
Type::Deserializeinsrc/idl_parser.cpp).is_series(src/idl_parser.cpp:4387-4388) listsreflection::Vectorandreflection::Arraybut notreflection::Vector64, so for a vector64 of structs the object index is looked up in the enum table instead ofstructs_:Parser::Deserializereturns false. For example,flatc -b tests/64bit/test_64bit.bfbs tests/64bit/test_64bit.jsonstops with "failed to load binary schema", while the same command withtest_64bit.fbssucceeds.enum_defand a nullstruct_def. Parsing JSON for the field through the loaded schema then reachesParseTable(*val.type.struct_def, ...)(src/idl_parser.cpp:1458) with a null pointer.This change adds
reflection::Vector64tois_series, so the element type resolves to the struct as it already does forVectorandArray.Verifying a buffer with reflection (
VerifyObjectinsrc/reflection.cpp). The field switch inVerifyObject(src/reflection.cpp:176) has nocase reflection::Vector64:. Such a field reaches thedefaultcase (src/reflection.cpp:262-264), which only containsFLATBUFFERS_ASSERT(false), so in a build with NDEBUG defined the field is not checked andflatbuffers::Verifyreturns true even when the field's 64-bit offset or element count points past the end of the buffer. The new case does what generated code does for a vector64 field (VerifyOffset64(verifier, VT_BIG_STRUCT_VECTOR)andverifier.VerifyVector(big_struct_vector())attests/64bit/test_64bit_generated.h:289-290): the element type must be a scalar or a struct, the 64-bit offset is checked withVerifyOffset64(orVerifyOffset64Requiredfor a required field), and the 64-bit element count times the element size must fit in the buffer (VerifyVectorOrString<uoffset64_t>).What still works: the parser change only affects fields whose base type is
reflection::Vector64, and the verifier change only adds checks for such fields, so well-formed schemas and buffers are not newly rejected (see Validation below). Binary schemas that contain a vector64 of structs now load and parse JSON to the same bytes as the text schema. What is now rejected byflatbuffers::Verify: a buffer whose vector64 field has an offset or an element count that reaches past the end of the buffer.Tests.
BinarySchemaVector64OfStructsTestintests/test.cpprunsCheckVector64OfStructsSchemaon a table withv:[S] (vector64)andw:[ubyte] (vector64), once without an enum and once with an enum whose index equals the struct's object index:Parser::Serialize, read back withParser::Deserialize, the field'sstruct_defis checked to be the struct, and{ "v": [ { "a": 1 }, { "a": 2 } ], "w": [ 1, 2, 3 ] }parsed with the loaded schema must give the same bytes as the text schema. Without the fix this fails: the first schema does not load, and in the second the field has a nullstruct_def.flatbuffers::Verifymust accept the valid buffer (it does with and without the fix), and must reject a copy in which the vector64 offset ofvorwis moved past the end of the buffer, and a copy in which the element count ofvorwis set to0xFFFFFFFF. Without the fix all eight rejections fail in a build with NDEBUG defined.Validation at 6dda43b with clang 21.1.8, running the full
flattestsbinary with no filter in a Release build (-O3 -DNDEBUG) and in an AddressSanitizer and UndefinedBehaviorSanitizer build (-O1 -g -DNDEBUG):With the fix,
Parser::Deserializealso accepts all 5 checked-in.bfbsfiles and 78.bfbsfiles written byflatc --schema -bfrom the.fbsfiles undertests/andsamples/(before the fix, the two copies oftest_64bit.bfbsare rejected), andflatbuffers::Verifystill acceptstests/64bit/test_64bit.binagainsttest_64bit.bfbs.flatc -bwithtest_64bit.bfbsnow writes the same bytes as withtest_64bit.fbs.This touches the same files as #9430, #9432 and #9433 but in separate hunks. It applies cleanly before or after each of them, and the full test suite passes with all four changes applied together.
#9147 rewrites the
is_seriesblock inType::Deserializewithout addingreflection::Vector64, so the two do not apply on top of each other as is; whichever lands second needs the one added line carried over.