Skip to content

Commit eabe3b6

Browse files
authored
Bump iterator state before pushing on sexp stack (#1913)
Fixes #1911. `sexp_next_incoming()` (`src/rlang/walk.c`) receives `p_info` as 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_info` used to build `child` happen 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 690 `sexp_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.
1 parent a18c5e7 commit eabe3b6

1 file changed

Lines changed: 15 additions & 13 deletions

File tree

‎src/rlang/walk.c‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -209,19 +209,9 @@ static bool sexp_next_incoming(
209209
bool has_attrib = sexp_has_attrib(child.type, child.x);
210210
enum sexp_iterator_type it_type = sexp_iterator_type(child.type, child.x);
211211

212-
if (it_type == SEXP_ITERATOR_TYPE_atomic && !has_attrib) {
213-
child.p_state = NULL;
214-
child.dir = R_SEXP_IT_DIRECTION_leaf;
215-
} else {
216-
init_incoming_stack_info(&child, it_type, has_attrib);
217-
218-
// Push incoming node on the stack so it can be visited again,
219-
// either to descend its children or to visit it again on the
220-
// outgoing trip
221-
r_dyn_push_back(p_it->p_stack, &child);
222-
}
223-
224-
// Bump state for next iteration
212+
// Bump state for next iteration. This must be done before pushing
213+
// `child` on the stack: the push may resize the stack and `p_info`
214+
// points into the pre-resize buffer.
225215
if (state == SEXP_ITERATOR_STATE_elt) {
226216
++p_info->v_arr;
227217
if (p_info->v_arr == p_info->v_arr_end) {
@@ -239,6 +229,18 @@ static bool sexp_next_incoming(
239229
p_info->dir = R_SEXP_IT_DIRECTION_outgoing;
240230
}
241231

232+
if (it_type == SEXP_ITERATOR_TYPE_atomic && !has_attrib) {
233+
child.p_state = NULL;
234+
child.dir = R_SEXP_IT_DIRECTION_leaf;
235+
} else {
236+
init_incoming_stack_info(&child, it_type, has_attrib);
237+
238+
// Push incoming node on the stack so it can be visited again,
239+
// either to descend its children or to visit it again on the
240+
// outgoing trip
241+
r_dyn_push_back(p_it->p_stack, &child);
242+
}
243+
242244
r_ssize i = -1;
243245
if (child.v_arr) {
244246
i = child.v_arr_end - child.v_arr;

0 commit comments

Comments
 (0)