Skip to content

Commit 19e1ab3

Browse files
youknowoneyouknowone
committed
Fix race condition: create unreachable_refs under read locks
Move unreachable_refs creation before drop(gen_locks) so that raw pointer dereferences and refcount increments happen while generation list read locks are held. Previously, after dropping read locks, other threads could untrack and free objects, causing use-after-free when creating strong references from the raw GcPtr pointers. Also add linked list integrity assertions and diagnostic warnings for debugging GC list corruption.
1 parent d3016dc commit 19e1ab3

3 files changed

Lines changed: 55 additions & 19 deletions

File tree

‎crates/common/src/linked_list.rs‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,15 @@ impl<L: Link> LinkedList<L, L::Target> {
157157
let ptr = L::as_raw(&val);
158158
assert_ne!(self.head, Some(ptr));
159159
unsafe {
160+
// Verify the node is not already in a list (pointers must be clean)
161+
assert!(
162+
L::pointers(ptr).as_ref().get_prev().is_none(),
163+
"push_front: node already has prev pointer (double-insert?)"
164+
);
165+
assert!(
166+
L::pointers(ptr).as_ref().get_next().is_none(),
167+
"push_front: node already has next pointer (double-insert?)"
168+
);
160169
L::pointers(ptr).as_mut().set_next(self.head);
161170
L::pointers(ptr).as_mut().set_prev(None);
162171

@@ -226,7 +235,11 @@ impl<L: Link> LinkedList<L, L::Target> {
226235
pub unsafe fn remove(&mut self, node: NonNull<L::Target>) -> Option<L::Handle> {
227236
unsafe {
228237
if let Some(prev) = L::pointers(node).as_ref().get_prev() {
229-
debug_assert_eq!(L::pointers(prev).as_ref().get_next(), Some(node));
238+
assert_eq!(
239+
L::pointers(prev).as_ref().get_next(),
240+
Some(node),
241+
"linked list corruption: prev->next != node (prev={prev:p}, node={node:p})"
242+
);
230243
L::pointers(prev)
231244
.as_mut()
232245
.set_next(L::pointers(node).as_ref().get_next());
@@ -239,7 +252,11 @@ impl<L: Link> LinkedList<L, L::Target> {
239252
}
240253

241254
if let Some(next) = L::pointers(node).as_ref().get_next() {
242-
debug_assert_eq!(L::pointers(next).as_ref().get_prev(), Some(node));
255+
assert_eq!(
256+
L::pointers(next).as_ref().get_prev(),
257+
Some(node),
258+
"linked list corruption: next->prev != node (next={next:p}, node={node:p})"
259+
);
243260
L::pointers(next)
244261
.as_mut()
245262
.set_prev(L::pointers(node).as_ref().get_prev());

‎crates/vm/src/gc_state.rs‎

Lines changed: 26 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,16 @@ impl GcState {
279279
count.fetch_sub(1, Ordering::SeqCst);
280280
obj_ref.clear_gc_tracked();
281281
obj_ref.set_gc_generation(GC_UNTRACKED);
282+
} else {
283+
// Object claims to be in this generation but wasn't found in the list.
284+
// This indicates a bug: the object was already removed from the list
285+
// without updating gc_generation, or was never inserted.
286+
eprintln!(
287+
"GC WARNING: untrack_object failed to remove obj={obj:p} from gen={obj_gen}, \
288+
tracked={}, gc_gen={}",
289+
obj_ref.is_gc_tracked(),
290+
obj_ref.gc_generation()
291+
);
282292
}
283293
return;
284294
}
@@ -470,10 +480,10 @@ impl GcState {
470480
);
471481
}
472482

473-
// Create strong references to reachable objects while read locks are
474-
// still held. After dropping gen_locks, other threads can untrack+free
475-
// objects, making the raw pointers in `reachable` dangling.
476-
// Strong refs keep objects alive for promote_survivors.
483+
// Create strong references while read locks are still held.
484+
// After dropping gen_locks, other threads can untrack+free objects,
485+
// making the raw pointers in `reachable`/`unreachable` dangling.
486+
// Strong refs keep objects alive for later phases.
477487
let survivor_refs: Vec<PyObjectRef> = reachable
478488
.iter()
479489
.filter_map(|ptr| {
@@ -486,6 +496,18 @@ impl GcState {
486496
})
487497
.collect();
488498

499+
let unreachable_refs: Vec<crate::PyObjectRef> = unreachable
500+
.iter()
501+
.filter_map(|ptr| {
502+
let obj = unsafe { ptr.0.as_ref() };
503+
if obj.strong_count() > 0 {
504+
Some(obj.to_owned())
505+
} else {
506+
None
507+
}
508+
})
509+
.collect();
510+
489511
if unreachable.is_empty() {
490512
drop(gen_locks);
491513
self.promote_survivors(generation, &survivor_refs);
@@ -499,19 +521,6 @@ impl GcState {
499521

500522
// Step 6: Finalize unreachable objects and handle resurrection
501523

502-
// 6a: Get references to all unreachable objects
503-
let unreachable_refs: Vec<crate::PyObjectRef> = unreachable
504-
.iter()
505-
.filter_map(|ptr| {
506-
let obj = unsafe { ptr.0.as_ref() };
507-
if obj.strong_count() > 0 {
508-
Some(obj.to_owned())
509-
} else {
510-
None
511-
}
512-
})
513-
.collect();
514-
515524
if unreachable_refs.is_empty() {
516525
self.promote_survivors(generation, &survivor_refs);
517526
self.generations[0].count.store(0, Ordering::SeqCst);

‎crates/vm/src/object/core.rs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,16 @@ pub(super) unsafe fn default_dealloc<T: PyPayload>(obj: *mut PyObject) {
176176
unsafe {
177177
crate::gc_state::gc_state().untrack_object(ptr);
178178
}
179+
// Verify untrack cleared the tracked flag and generation
180+
debug_assert!(
181+
!obj_ref.is_gc_tracked(),
182+
"object still tracked after untrack_object"
183+
);
184+
debug_assert_eq!(
185+
obj_ref.gc_generation(),
186+
crate::object::GC_UNTRACKED,
187+
"gc_generation not reset after untrack_object"
188+
);
179189
}
180190

181191
// Extract child references before deallocation to break circular refs (tp_clear)

0 commit comments

Comments
 (0)