Fix stack-use-after-return when the depth limit refuses a hash - #4210
Open
matz wants to merge 1 commit into
Open
Fix stack-use-after-return when the depth limit refuses a hash#4210matz wants to merge 1 commit into
matz wants to merge 1 commit into
Conversation
parse_assocs stores the address of its caller's stack-local pm_static_literals_t into parser->current_hash_keys so that a hash written directly into another with ** shares its keys, and the PM_TOKEN_BRACE_LEFT case in parse_expression_prefix is the only place that takes it back out and sets it to NULL. parse_expression returns before reaching parse_expression_prefix when depth has hit PRISM_DEPTH_MAXIMUM, so on input deep enough to reach the limit inside such a hash, nothing takes the pointer. The frame holding the static literals then returns, and the next hash to be parsed adds its keys to a dead stack frame. Clear the pointer where the expression it was handed over for is refused. Co-authored-by: Claude <noreply@anthropic.com>
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.
This fixes a stack-use-after-return found while fuzzing mruby, which vendors Prism as its compiler. It is the same shape as #4155: a stack address parked in the parser and left behind on a path that never takes it back out.
Reproduction, a plain Ruby file:
Under ASan with
detect_stack_use_after_return=1:Cause
parse_assocsstores the address of its caller's stack-localpm_static_literals_tintoparser->current_hash_keys, so that a hash written directly into another one with**shares its keys for the sake of the duplicate-key warning. ThePM_TOKEN_BRACE_LEFTcase inparse_expression_prefixis the only place that takes it back out, and it sets the field toNULLas it does, exactly so that no later expression can use it.parse_expressionreturns before it reachesparse_expression_prefixwhendepthhas hitPRISM_DEPTH_MAXIMUM. On input deep enough to reach the limit inside such a hash, nothing takes the pointer.parse_argumentsthen returns, itshash_keysdies, and the next hash to be parsed adds its keys to a dead stack frame.Fix
Clear
parser->current_hash_keyswhere the expression it was handed over for is refused. The hand-over is one-shot and its consumer runs first thing inside the veryparse_expressioncall that is declining here, so a pointer still set at this point is one nothing will ever take.Reaching it at the default depth
PRISM_DEPTH_MAXIMUMis 10000 by default, and an ASan build's frames are large enough that the C stack goes first on input that deep. mruby compiles Prism withPRISM_DEPTH_MAXIMUM=256, which is how the fuzzer found it; the reproduction above is with that value. The defect itself is not conditional on the figure, only on reaching the limit inside a**{...}.Verification
mainat3ea210bwithPRISM_DEPTH_MAXIMUM=256, and in mruby's build..rbsources) are byte-identical before and after, at the default depth.{a: 1, **{a: 2}},foo(a: 1, **{a: 2})and{a: 1, **{**{a: 2}}}.MRB_GC_STRESS, ASan/UBSan, and its five CI build configurations.I could not run Prism's own Ruby test suite here (no
rake-compileravailable), so I have leaned on the AST differential above; CI will cover the rest.