fix: stack overflow in FlexBuffers ToString via unbounded mutual recursion#9078
Open
SongTonyLi wants to merge 2 commits intogoogle:masterfrom
Open
fix: stack overflow in FlexBuffers ToString via unbounded mutual recursion#9078SongTonyLi wants to merge 2 commits intogoogle:masterfrom
SongTonyLi wants to merge 2 commits intogoogle:masterfrom
Conversation
…rsion ToString and AppendToString<Vector> recurse into each other with no depth limit. A crafted 31-byte FlexBuffer with self-referential vectors passes VerifyBuffer and then blows the stack (~247 frames). Fix: cap recursion at depth 64 using the existing cur_indent parameter, emitting "..." when the limit is reached. Fixes google#9074
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.
Summary
Fixes #9074.
Reference::ToStringandAppendToString<Vector>call each other back and forth with no depth check. A 31-byte crafted FlexBuffer that contains a self-referential vector passesVerifyBufferfine, but thenToStringrecurses about 247 times and blows the stack.Root cause
The PoC encodes a vector at offset 28 whose single element points right back to offset 28 (i.e., it references itself). The verifier doesn't catch this because its reuse-tracker has already marked offset 28 as verified on the first pass, so when it sees offset 28 again it just short-circuits and says "already checked, we're good."
Once
ToStringgets called on this, it enters an infinite loop:Reference::ToStringsees a vector, callsAppendToString<Vector>AppendToString<Vector>iterates the child, callschild.ToString(...)AppendToString<Vector>I put some trace probes into the verifier to see what was happening:
VerifyRefentered atdata_off=28withoff=0,indirect_off=28,type=10(FBT_VECTOR) -- the self-referenceVerifyVectorran atp_off=28the first time (full shape check), then entered again at the samep_off=28and exited immediately via the reuse-tracker short-circuitVerifyBufferreturnedok=1-- accepted the cyclic bufferThen ASan caught the stack overflow: ~247 alternating
ToString/AppendToString<Vector>frames before it hit the guard page.Fix
ToStringalready has acur_indentparameter that gets incremented on every recursive call into a nested container. I added akToStringMaxDepth = 64constant and a check right before the map/vector/typed-vector branches: ifcur_indent >= 64, just emit"..."and stop recursing. No new function parameters needed sincecur_indentwas already being threaded through.Why this approach
I considered putting the depth guard inside
AppendToString<Vector>instead, but that only covers vector-type recursion. The map branch inToStringalso recurses (throughvals[i].ToString(..., cur_indent + 1, ...)), so a guard there would still leave map recursion unbounded. Putting the check inToStringitself covers all container types in one place.Verification
ERROR: AddressSanitizer: stack-overflowwith ~247 alternatingToString/AppendToString<Vector>framesToStringproduces truncated output (259 chars with"..."at the depth limit), exits cleanly with no crashRegression testing