Skip to content

Commit 25692b4

Browse files
committed
Fix UAF in visit_generation
Callback can free an object resulting in UAF if we have relied on the object for visiting the next item in the chain. Fix is to use `finalize_garbage` approach of walking the object gc generation list directly maintaing and moving it into a tempoary list before calling callback
1 parent 036d309 commit 25692b4

3 files changed

Lines changed: 56 additions & 8 deletions

File tree

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Fix a crash in :c:func:`PyUnstable_GC_VisitObjects` on the default build
2+
when the callback releases the last reference to the object it is visiting.

‎Modules/_testcapi/gc.c‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,42 @@ test_gc_visit_objects_exit_early(PyObject *Py_UNUSED(self),
271271
Py_RETURN_NONE;
272272
}
273273

274+
#ifndef Py_GIL_DISABLED
275+
static int
276+
gc_visit_callback_drop_current(PyObject *obj, void *arg)
277+
{
278+
struct gc_visit_state_basic *state = (struct gc_visit_state_basic *)arg;
279+
if (obj == state->target) {
280+
state->found = 1;
281+
Py_CLEAR(state->target);
282+
}
283+
return 1;
284+
}
285+
286+
static PyObject *
287+
test_gc_visit_objects_drop_current(PyObject *Py_UNUSED(self),
288+
PyObject *Py_UNUSED(ignored))
289+
{
290+
struct gc_visit_state_basic state;
291+
292+
state.target = PySet_New(NULL);
293+
if (state.target == NULL) {
294+
return NULL;
295+
}
296+
state.found = 0;
297+
298+
PyUnstable_GC_VisitObjects(gc_visit_callback_drop_current, &state);
299+
if (!state.found) {
300+
Py_DECREF(state.target);
301+
PyErr_SetString(
302+
PyExc_AssertionError,
303+
"test_gc_visit_objects_drop_current: Didn't find live set");
304+
return NULL;
305+
}
306+
Py_RETURN_NONE;
307+
}
308+
#endif
309+
274310
typedef struct {
275311
PyObject_HEAD
276312
} ObjExtraData;
@@ -369,6 +405,9 @@ static PyMethodDef test_methods[] = {
369405
{"test_gc_visit_objects_basic", test_gc_visit_objects_basic, METH_NOARGS, NULL},
370406
{"test_gc_visit_objects_frozen", test_gc_visit_objects_frozen, METH_NOARGS, NULL},
371407
{"test_gc_visit_objects_exit_early", test_gc_visit_objects_exit_early, METH_NOARGS, NULL},
408+
#ifndef Py_GIL_DISABLED
409+
{"test_gc_visit_objects_drop_current", test_gc_visit_objects_drop_current, METH_NOARGS, NULL},
410+
#endif
372411
{"without_gc", without_gc, METH_O, NULL},
373412
{"with_tp_del", with_tp_del, METH_VARARGS, NULL},
374413
{NULL}

‎Python/gc.c‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2164,18 +2164,25 @@ PyObject_GC_IsFinalized(PyObject *obj)
21642164
static int
21652165
visit_generation(gcvisitobjects_t callback, void *arg, struct gc_generation *gen)
21662166
{
2167-
PyGC_Head *gc_list, *gc;
2168-
gc_list = &gen->head;
2169-
for (gc = GC_NEXT(gc_list); gc != gc_list; gc = GC_NEXT(gc)) {
2167+
PyGC_Head *gc_list = &gen->head;
2168+
PyGC_Head seen;
2169+
int res = 1;
2170+
2171+
/* The callback may free or untrack the object it is given, so always
2172+
* take the first object and move it to `seen` before calling it. */
2173+
gc_list_init(&seen);
2174+
while (res && !gc_list_is_empty(gc_list)) {
2175+
PyGC_Head *gc = GC_NEXT(gc_list);
21702176
PyObject *op = FROM_GC(gc);
2177+
gc_list_move(gc, &seen);
21712178
Py_INCREF(op);
2172-
int res = callback(op, arg);
2179+
res = callback(op, arg);
21732180
Py_DECREF(op);
2174-
if (!res) {
2175-
return -1;
2176-
}
21772181
}
2178-
return 0;
2182+
/* Put the visited objects back in front of the unvisited ones. */
2183+
gc_list_merge(gc_list, &seen);
2184+
gc_list_merge(&seen, gc_list);
2185+
return res ? 0 : -1;
21792186
}
21802187

21812188
void

0 commit comments

Comments
 (0)