bump iterator state before pushing on sexp stack - #1913
Merged
Conversation
`sexp_next_incoming()` received `p_info` as a raw pointer into the traversal stack's buffer. Pushing the child could resize the stack, reallocating the buffer, after which the parent's state bump and incoming/outgoing flip were written into the dead pre-resize buffer. When the parent resurfaced, the stale copied state made the iterator revisit the same edge, duplicating traversal of entire subtrees once depth exceeded the initial stack capacity of 256. Bump the state before the push so the updated state is what gets copied on resize. Fixes r-lib#1911.
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 #1911.
sexp_next_incoming()(src/rlang/walk.c) receivesp_infoas a raw pointer into the traversal stack's dyn-array buffer. The child push could resize the stack — reallocating the buffer and dropping the old one's protection — after which the parent's state bump and incoming→outgoing flip were written through the stale pointer into the dead buffer. The live (copied) entry kept its pre-bump state, so the iterator revisited the same edge and re-traversed entire subtrees once depth exceeded the initial stack capacity of 256. It was also a latent use-after-free write, benign today only because no allocation happens between the resize and the writes.This PR moves the state bump before the push. All reads of
p_infoused to buildchildhappen earlier, so the reorder is behavior-preserving apart from the fix.Since this code is only compiled under
RLANG_USE_PRIVATE_ACCESSORS(and currently doesn't build on R >= 4.5, see the issue), there's no regression test in the default configuration. Verified in a flagged dev build with accessor stubs: before the fix, a depth-300 nested list yields 690sexp_iterate()callback visits with 44 nodes visited twice as incoming; after the fix, exactly 601 visits (2n + 1) with no duplicates, and the depth-250 control is unchanged.