Repository navigation
Fix null dereference in JSON printer for unions without type field - #9308
Open
xhon-pelushi wants to merge 1 commit into
Open
xhon-pelushi wants to merge 1 commit into
xhon-pelushi wants to merge 1 commit into
Conversation
A corrupt buffer can hold a union value while its type field is absent. PrintOffset() only guarded this with FLATBUFFERS_ASSERT, so release builds dereferenced a null prev_val and crashed (e.g. `flatc --json` on such a binary). If an earlier unrelated field was present, prev_val still pointed at it and its value was silently used as the union type. Return an error when prev_val is null, and reset prev_val for absent fields so a union never picks up a stale type field. Fixes google#9033
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.
Fixes #9033
When
JsonPrinter::PrintOffset()(src/idl_gen_text.cpp) prints a union, it reads the discriminator throughprev_val, a pointer thatGenStruct()sets to the field printed just before the union. Corrupt buffers that have the union value but not its_typefield broke this in two ways:prev_valisnullptr. The only guard wasFLATBUFFERS_ASSERT(prev_val), which release builds compile out, so*prev_valsegfaults. This is the crash in the issue:flatc --json --raw-binaryon the 40-byte file exits with SIGSEGV on master.GenStruct()only updatedprev_valfor fields that were present. If the_typefield is absent but an earlier field was present,prev_valstill points at that earlier field, and its byte is silently read as the union type. For example, withtable Root { a: ubyte; u: MyUnion; },{a: 1, u_type: Inner, u: {x: 42}}and theu_typevtable slot zeroed, master prints{ a: 1, u: { x: 42 } }with no error, usingaas the type.The fix:
PrintOffset(), the assert is replaced withif (!prev_val) return "union type field not present";, which reports the problem through the existing error path (GenText()/GenTextFromTable()/GenTextFile()already returnconst char*errors).GenStruct(),prev_valis reset tonullptrwhen a field is absent, so a union only ever reads its own_typefield.Both parts are needed. With only the null check, the stale case in the new test still fails.
The new test is
JsonUnionMissingTypeTestintests/json_test.cpp. It builds a buffer from JSON, zeroes one vtable slot, and checksGenText(): the unmodified buffer still prints, and errors are returned whenu_typeis missing (with and without an earlier present field) and whenv_typeis missing for a vector of unions.Testing (Release build,
-DNDEBUG):flattests: all tests pass with this change.flattestssegfaults insideJsonUnionMissingTypeTest→GenText→PrintOffset.json_test.cpp:250).flatcrepro now printsUnable to generate text for corrupt (union type field not present)and exits with 1 instead of crashing. A valid buffer prints as before.