fix: validate offset before forming a pointer in ResizeContext::ResizeTable - #9201
Open
prasanna8585 wants to merge 1 commit into
Open
fix: validate offset before forming a pointer in ResizeContext::ResizeTable#9201prasanna8585 wants to merge 1 commit into
prasanna8585 wants to merge 1 commit into
Conversation
…eTable
SetString and ResizeAnyVector construct a ResizeContext, which walks
the ENTIRE object graph from the root table (not just the field being
resized) via ResizeTable, adjusting offsets for the insertion point.
For each object-typed field encountered along the way, it read a
stored uoffset_t and formed a pointer from it with no validation:
auto ref = offsetloc + ReadScalar<uoffset_t>(offsetloc);
A malformed or corrupted offset in ANY such field -- not necessarily
the field the caller intended to touch -- produces a wild pointer
that is then dereferenced in the recursive ResizeTable calls below,
causing a crash (confirmed via a real ASan SEGV in
ResizeContext::ResizeTable).
Re-running Verifier immediately before the call does catch this
specific corruption, but ResizeTable itself has no independent bounds
checking of its own -- the same gap AddFlatBuffer had for its root
offset before that was fixed directly in the function (commit
21b0332, "Add bounds check for root offset in AddFlatBuffer"), rather
than treated as solely the caller's responsibility to have verified.
This applies the same style of fix here: validate the raw offset
value against the buffer's bounds using integer arithmetic (checked
via subtraction to avoid overflow in the addition itself) before
using it to form any pointer, and skip the field rather than crash if
it's out of range.
Adds ResizeTableMalformedOffsetTest, mirroring the existing
ForAllFieldsReverseTest's structure, which corrupts a sibling
object-typed field's stored offset and confirms that resizing an
unrelated string field no longer crashes.
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.
SetStringandResizeAnyVectorconstruct aResizeContext, which walks the ENTIRE object graph from the root table (not just the field being resized) viaResizeTable, adjusting offsets for the insertion point. For each object-typed field encountered along the way, it read a storeduoffset_tand formed a pointer from it with no validation:A malformed or corrupted offset in ANY such field — not necessarily the field the caller intended to touch — produces a wild pointer that is then dereferenced in the recursive
ResizeTablecalls below, causing a crash. Confirmed via a real ASan SEGV inResizeContext::ResizeTable, reached fromSetString→ResizeContext::ResizeContext→ResizeTable→ResizeTable(recursive) → wild read.Re-running
Verifierimmediately before the call does catch this specific corruption, butResizeTableitself has no independent bounds checking of its own — the same gapAddFlatBufferhad for its root offset before that was fixed directly in the function (commit 21b0332, "Add bounds check for root offset in AddFlatBuffer"), rather than treated as solely the caller's responsibility to have verified beforehand. This applies the same style of fix here: validate the raw offset value against the buffer's bounds using integer arithmetic (checked via subtraction to avoid overflow in the addition itself) before using it to form any pointer, and skip the field rather than crash if it's out of range.Adds
ResizeTableMalformedOffsetTest, mirroring the existingForAllFieldsReverseTest's structure: it corrupts a sibling object-typed field's stored offset and confirms that resizing an unrelated string field no longer crashes.Verified: full existing test suite passes under ASan (
ALL TESTS PASSED, no sanitizer errors) with the fix applied, including the new regression test.