Skip to content

Commit b279716

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 6a40f0f commit b279716

File tree

3 files changed

+55
-19
lines changed

3 files changed

+55
-19
lines changed

‎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
@@ -281,6 +281,16 @@ impl GcState {
281281
count.fetch_sub(1, Ordering::SeqCst);
282282
obj_ref.clear_gc_tracked();
283283
obj_ref.set_gc_generation(GC_UNTRACKED);
284+
} else {
285+
// Object claims to be in this generation but wasn't found in the list.
286+
// This indicates a bug: the object was already removed from the list
287+
// without updating gc_generation, or was never inserted.
288+
eprintln!(
289+
"GC WARNING: untrack_object failed to remove obj={obj:p} from gen={obj_gen}, \
290+
tracked={}, gc_gen={}",
291+
obj_ref.is_gc_tracked(),
292+
obj_ref.gc_generation()
293+
);
284294
}
285295
return;
286296
}
@@ -472,10 +482,10 @@ impl GcState {
472482
);
473483
}
474484

475-
// Create strong references to reachable objects while read locks are
476-
// still held. After dropping gen_locks, other threads can untrack+free
477-
// objects, making the raw pointers in `reachable` dangling.
478-
// Strong refs keep objects alive for promote_survivors.
485+
// Create strong references while read locks are still held.
486+
// After dropping gen_locks, other threads can untrack+free objects,
487+
// making the raw pointers in `reachable`/`unreachable` dangling.
488+
// Strong refs keep objects alive for later phases.
479489
let survivor_refs: Vec<PyObjectRef> = reachable
480490
.iter()
481491
.filter_map(|ptr| {
@@ -488,6 +498,18 @@ impl GcState {
488498
})
489499
.collect();
490500

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

502524
// Step 6: Finalize unreachable objects and handle resurrection
503525

504-
// 6a: Get references to all unreachable objects
505-
let unreachable_refs: Vec<crate::PyObjectRef> = unreachable
506-
.iter()
507-
.filter_map(|ptr| {
508-
let obj = unsafe { ptr.0.as_ref() };
509-
if obj.strong_count() > 0 {
510-
Some(obj.to_owned())
511-
} else {
512-
None
513-
}
514-
})
515-
.collect();
516-
517526
if unreachable_refs.is_empty() {
518527
self.promote_survivors(generation, &survivor_refs);
519528
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)