Repository navigation
fix: prevent stale scope slot reference in variable declarations - #263
Open
minh-anm-nguyen wants to merge 1 commit into
Open
minh-anm-nguyen wants to merge 1 commit into
minh-anm-nguyen wants to merge 1 commit into
Conversation
ASTNodeVariableDecl::execute() bound a reference to the scope's last pattern and used it again after creating the declared type's patterns. Creating those patterns appends to the very same vector, which grows and relocates its storage, leaving the earlier reference pointing at memory the vector no longer owns. Remember the element's index instead and re-acquire the slot immediately before each later use. A copied shared_ptr is not an alternative here: Evaluator::setVariable() assigns through its reference parameter, so a copy would stop updating the scope slot for reference variables. Add a regression test covering the pattern-local declaration path that reaches this code. The reproducer fails on 19 of 20 unrepaired runs, so the test repeats the evaluation to keep a single clean run from hiding a regression. Pattern-local declarations produce no entries in getPatterns(), so it asserts emptiness as a structural guard against the source being changed to a placed declaration.
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 the crash reported in WerWolv/ImHex#2043.
ASTNodeVariableDecl::execute()binds a reference to the scope's last pattern:and uses it again about thirty lines further down, after
evaluatedType->createPatterns()has run.Scope::scopeis astd::vector<std::shared_ptr<Pattern>>*, and creating the declared type's patterns appends to that same vector. The vector grows, relocates its storage, and the earlier reference is left pointing at memory the vector no longer owns.Reproducer, a section placed inside a struct:
Under a debugger the vector's data pointer moves while the reference stays behind:
Run through
plclithis terminates on 19 of 20 runs. The runs that survive do so only because the previous bytes happen to still be readable.The fix records the element's index and re-acquires the slot immediately before each later use. Copying the
shared_ptrinstead is not an option:Evaluator::setVariable(std::shared_ptr<Pattern>&, ...)assigns through its reference parameter, so a copy would stop updating the scope slot and would silently change the behaviour of reference variables.The regression test covers the same path without needing an include path, by calling
builtin::std::mem::create_sectiondirectly. Because a pattern-local declaration produces no entries ingetPatterns(), there is no resulting value to assert on, so the test asserts emptiness as a structural guard: that fails if the reproducer is ever changed to a placed declaration, which takes the other branch and would stop covering this defect. The evaluation is repeated so that one lucky run cannot hide a regression.