protect rotated ast root during injection - #1912
Merged
Merged
Conversation
After a left rotation in `maybe_rotate()`, the new expression root is only referenced from a C local: the protect stack pins the old root, which is now a child of the new one, and protection is only transitive downwards. The subsequent recursion evaluates remaining `!!` operands, so a GC triggered by user code could collect the new root and the not-yet-expanded operands, corrupting the injection result. Protect the new root across the recursion. Fixes r-lib#1910.
Member
|
Thanks! |
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
2 participants
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 #1910.
After a left rotation in
maybe_rotate()(src/internal/ast-rotate.c), the new expression root is referenced only from a C local: the protect stack pins the old root, which has just become a child of the new one, and protection is only transitive downwards. The recursion that follows evaluates remaining!!operands viar_eval(), so a GC triggered by user code collects the new root's cons cells and the not-yet-expanded operands. See the issue for a deterministic reproducible example on CRAN rlang 1.3.0 and full analysis.This PR protects the new root across the recursion. Each recursion level pins its current root, and the old root plus the unexpanded RHS chain are reachable from it. The second rotation branch is unchanged: it reattaches the pivot into the protected tree before recursing.
The regression test interpolates before calling
expect_identical(): with the injection inline in the expectation, the interpolation would be performed by the rlang imported by testthat, which inload_all()sessions can be a different (installed) rlang than the package under test.Verified: the new test fails against unfixed rlang 1.3.0 and passes with the fix; all other
test-nse-inject.Rexpectations pass; an instrumented build confirms the rotated root survives a forcedR_gc()in the previously unprotected window.