Skip to content

Align del behavior #6772

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 4 commits into from
Jan 31, 2026
Merged

Align del behavior #6772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
10a4c44
slot_del
youknowone Jan 30, 2026
896c0e1
refcount inc_by for atomicity
youknowone Jan 30, 2026
98458e5
temp patch multithreading
youknowone Jan 31, 2026
f310395
apply review
youknowone Jan 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,7 @@ env:
test_yield_from
ENV_POLLUTING_TESTS_COMMON: >-
ENV_POLLUTING_TESTS_LINUX: >-
test.test_multiprocessing_fork.test_processes
test.test_multiprocessing_forkserver.test_processes
test.test_multiprocessing_spawn.test_processes
ENV_POLLUTING_TESTS_MACOS: >-
test.test_multiprocessing_forkserver.test_processes
test.test_multiprocessing_spawn.test_processes
ENV_POLLUTING_TESTS_WINDOWS: >-
# Python version targeted by the CI.
PYTHON_VERSION: "3.14.2"
Expand Down
6 changes: 4 additions & 2 deletions Lib/test/_test_multiprocessing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6592,7 +6592,8 @@ def tearDownClass(cls):
# cycles. Trigger a garbage collection to break these cycles.
test.support.gc_collect()

processes = set(multiprocessing.process._dangling) - set(cls.dangling[0])
# TODO: RUSTPYTHON: Filter out stopped processes since gc.collect() is a no-op
processes = {p for p in multiprocessing.process._dangling if p.is_alive()} - {p for p in cls.dangling[0] if p.is_alive()}
if processes:
test.support.environment_altered = True
support.print_warning(f'Dangling processes: {processes}')
Expand Down Expand Up @@ -6789,7 +6790,8 @@ def tearDownModule():

multiprocessing.set_start_method(old_start_method[0], force=True)
# pause a bit so we don't get warning about dangling threads/processes
processes = set(multiprocessing.process._dangling) - set(dangling[0])
# TODO: RUSTPYTHON: Filter out stopped processes since gc.collect() is a no-op
processes = {p for p in multiprocessing.process._dangling if p.is_alive()} - {p for p in dangling[0] if p.is_alive()}
if processes:
need_sleep = True
test.support.environment_altered = True
Expand Down
17 changes: 0 additions & 17 deletions Lib/test/test_multiprocessing_fork/test_processes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,23 +22,6 @@ def test_args_argument(self): super().test_args_argument() # TODO: RUSTPYTHON
@unittest.skipIf(sys.platform == 'linux', 'TODO: RUSTPYTHON flaky timeout')
def test_process(self): super().test_process() # TODO: RUSTPYTHON

class WithProcessesTestPool(WithProcessesTestPool): # TODO: RUSTPYTHON
@unittest.skipIf( # TODO: RUSTPYTHON
sys.platform == 'linux' and 'RUSTPYTHON_SKIP_ENV_POLLUTERS' in os.environ, # TODO: RUSTPYTHON
'TODO: RUSTPYTHON environment pollution when running rustpython -m test --fail-env-changed due to unknown reason'
) # TODO: RUSTPYTHON
def test_async_timeout(self): super().test_async_timeout() # TODO: RUSTPYTHON
@unittest.skipIf( # TODO: RUSTPYTHON
sys.platform == 'linux' and 'RUSTPYTHON_SKIP_ENV_POLLUTERS' in os.environ, # TODO: RUSTPYTHON
'TODO: RUSTPYTHON environment pollution when running rustpython -m test --fail-env-changed due to unknown reason'
) # TODO: RUSTPYTHON
def test_terminate(self): super().test_terminate() # TODO: RUSTPYTHON
@unittest.skipIf( # TODO: RUSTPYTHON
sys.platform == 'linux' and 'RUSTPYTHON_SKIP_ENV_POLLUTERS' in os.environ, # TODO: RUSTPYTHON
'TODO: RUSTPYTHON environment pollution when running rustpython -m test --fail-env-changed due to unknown reason'
) # TODO: RUSTPYTHON
def test_traceback(self): super().test_traceback() # TODO: RUSTPYTHON

class WithProcessesTestPoolWorkerLifetime(WithProcessesTestPoolWorkerLifetime): # TODO: RUSTPYTHON
@unittest.skipIf(sys.platform == 'linux', 'TODO: RUSTPYTHON flaky timeout')
def test_pool_worker_lifetime(self): super().test_pool_worker_lifetime() # TODO: RUSTPYTHON
Expand Down
22 changes: 0 additions & 22 deletions Lib/test/test_multiprocessing_forkserver/test_processes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,33 +11,11 @@ def test_notify(self): super().test_notify()
def test_notify_n(self): super().test_notify_n()

class WithProcessesTestLock(WithProcessesTestLock): # TODO: RUSTPYTHON
@unittest.skipIf( # TODO: RUSTPYTHON
'RUSTPYTHON_SKIP_ENV_POLLUTERS' in os.environ, # TODO: RUSTPYTHON
'TODO: RUSTPYTHON environment pollution when running rustpython -m test --fail-env-changed due to unknown reason'
) # TODO: RUSTPYTHON
def test_repr_lock(self): super().test_repr_lock() # TODO: RUSTPYTHON
@unittest.skipIf( # TODO: RUSTPYTHON
sys.platform == 'linux', # TODO: RUSTPYTHON
'TODO: RUSTPYTHON flaky BrokenPipeError, flaky ConnectionRefusedError, flaky ConnectionResetError, flaky EOFError'
) # TODO: RUSTPYTHON
def test_repr_rlock(self): super().test_repr_rlock() # TODO: RUSTPYTHON

class WithProcessesTestPool(WithProcessesTestPool): # TODO: RUSTPYTHON
@unittest.skipIf( # TODO: RUSTPYTHON
'RUSTPYTHON_SKIP_ENV_POLLUTERS' in os.environ, # TODO: RUSTPYTHON
'TODO: RUSTPYTHON environment pollution when running rustpython -m test --fail-env-changed due to unknown reason'
) # TODO: RUSTPYTHON
def test_async_timeout(self): super().test_async_timeout() # TODO: RUSTPYTHON
@unittest.skipIf( # TODO: RUSTPYTHON
'RUSTPYTHON_SKIP_ENV_POLLUTERS' in os.environ, # TODO: RUSTPYTHON
'TODO: RUSTPYTHON environment pollution when running rustpython -m test --fail-env-changed due to unknown reason'
) # TODO: RUSTPYTHON
def test_terminate(self): super().test_terminate() # TODO: RUSTPYTHON
@unittest.skipIf( # TODO: RUSTPYTHON
'RUSTPYTHON_SKIP_ENV_POLLUTERS' in os.environ, # TODO: RUSTPYTHON
'TODO: RUSTPYTHON environment pollution when running rustpython -m test --fail-env-changed due to unknown reason'
) # TODO: RUSTPYTHON
def test_traceback(self): super().test_traceback() # TODO: RUSTPYTHON

if __name__ == '__main__':
unittest.main()
8 changes: 0 additions & 8 deletions Lib/test/test_multiprocessing_forkserver/test_threads.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,5 @@

install_tests_in_module_dict(globals(), 'forkserver', only_type="threads")

import os # TODO: RUSTPYTHON
class WithThreadsTestPool(WithThreadsTestPool): # TODO: RUSTPYTHON
@unittest.skipIf( # TODO: RUSTPYTHON
'RUSTPYTHON_SKIP_ENV_POLLUTERS' in os.environ, # TODO: RUSTPYTHON
'TODO: RUSTPYTHON environment pollution when running rustpython -m test --fail-env-changed due to unknown reason'
) # TODO: RUSTPYTHON
def test_terminate(self): super().test_terminate() # TODO: RUSTPYTHON

if __name__ == '__main__':
unittest.main()
22 changes: 0 additions & 22 deletions Lib/test/test_multiprocessing_spawn/test_processes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,33 +9,11 @@ class WithProcessesTestCondition(WithProcessesTestCondition): # TODO: RUSTPYTHO
def test_notify(self): super().test_notify()

class WithProcessesTestLock(WithProcessesTestLock): # TODO: RUSTPYTHON
@unittest.skipIf( # TODO: RUSTPYTHON
sys.platform in ('darwin', 'linux') and 'RUSTPYTHON_SKIP_ENV_POLLUTERS' in os.environ, # TODO: RUSTPYTHON
'TODO: RUSTPYTHON environment pollution when running rustpython -m test --fail-env-changed due to unknown reason'
) # TODO: RUSTPYTHON
def test_repr_lock(self): super().test_repr_lock() # TODO: RUSTPYTHON
@unittest.skipIf( # TODO: RUSTPYTHON
sys.platform == 'linux', # TODO: RUSTPYTHON
'TODO: RUSTPYTHON flaky BrokenPipeError, flaky ConnectionRefusedError, flaky ConnectionResetError, flaky EOFError'
) # TODO: RUSTPYTHON
def test_repr_rlock(self): super().test_repr_rlock() # TODO: RUSTPYTHON

class WithProcessesTestPool(WithProcessesTestPool): # TODO: RUSTPYTHON
@unittest.skipIf( # TODO: RUSTPYTHON
sys.platform in ('darwin', 'linux') and 'RUSTPYTHON_SKIP_ENV_POLLUTERS' in os.environ, # TODO: RUSTPYTHON
'TODO: RUSTPYTHON environment pollution when running rustpython -m test --fail-env-changed due to unknown reason'
) # TODO: RUSTPYTHON
def test_async_timeout(self): super().test_async_timeout() # TODO: RUSTPYTHON
@unittest.skipIf( # TODO: RUSTPYTHON
sys.platform in ('darwin', 'linux') and 'RUSTPYTHON_SKIP_ENV_POLLUTERS' in os.environ, # TODO: RUSTPYTHON
'TODO: RUSTPYTHON environment pollution when running rustpython -m test --fail-env-changed due to unknown reason'
) # TODO: RUSTPYTHON
def test_terminate(self): super().test_terminate() # TODO: RUSTPYTHON
@unittest.skipIf( # TODO: RUSTPYTHON
sys.platform in ('darwin', 'linux') and 'RUSTPYTHON_SKIP_ENV_POLLUTERS' in os.environ, # TODO: RUSTPYTHON
'TODO: RUSTPYTHON environment pollution when running rustpython -m test --fail-env-changed due to unknown reason'
) # TODO: RUSTPYTHON
def test_traceback(self): super().test_traceback() # TODO: RUSTPYTHON

if __name__ == '__main__':
unittest.main()
10 changes: 10 additions & 0 deletions crates/common/src/refcount.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,16 @@ impl RefCount {
}
}

#[inline]
pub fn inc_by(&self, n: usize) {
debug_assert!(n <= Self::MASK);
let old_size = self.strong.fetch_add(n, Relaxed);

if old_size & Self::MASK > Self::MASK - n {
std::process::abort();
Comment on lines 43 to 49
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 31, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

cat -n crates/common/src/refcount.rs | head -80

Repository: RustPython/RustPython

Length of output: 2550

🏁 Script executed:

# Search for MASK definition and inc_by usage
rg -n "MASK|inc_by" crates/common/src/refcount.rs

Repository: RustPython/RustPython

Length of output: 227

🏁 Script executed:

# Find all call sites of inc_by
rg -n "inc_by" --type rs

Repository: RustPython/RustPython

Length of output: 91

🏁 Script executed:

# Find all call sites of inc_by across the codebase
rg -n "inc_by\s*\(" --type-list | head -5

Repository: RustPython/RustPython

Length of output: 454

🏁 Script executed:

# Find all call sites of inc_by
rg -n "\.inc_by\s*\("

Repository: RustPython/RustPython

Length of output: 198

🏁 Script executed:

# Check the specific call site context
sed -n '840,850p' crates/vm/src/object/core.rs | cat -n

Repository: RustPython/RustPython

Length of output: 714

🏁 Script executed:

# Verify the math: what is MASK / MAX_REFCOUNT value
python3 << 'EOF'
import sys
max_refcount = (2**63 - 1) if sys.maxsize == (2**63 - 1) else (2**31 - 1)
print(f"isize::MAX on this system: {max_refcount}")
print(f"Self::MASK = {max_refcount}")

# Test the underflow scenario
n_large = max_refcount + 1000
mask = max_refcount
result = mask - n_large if n_large <= mask else (mask.bit_length(), "would underflow")
print(f"\nIf n = {n_large} (exceeds MASK):")
print(f"  Self::MASK - n underflows in release mode to: {(1 << 64) - (n_large - mask)}")
print(f"  The check would compare against this huge number and likely skip abort()")
EOF

Repository: RustPython/RustPython

Length of output: 332

🏁 Script executed:

# Check if there are any guards or validation of the n parameter elsewhere
rg -n "inc_by" -B 5 -A 5

Repository: RustPython/RustPython

Length of output: 1825

Add bounds check in inc_by to prevent overflow detection bypass.

The overflow check if old_size & Self::MASK > Self::MASK - n fails when n > MASK because the subtraction underflows (wraps to a large number in release mode). This bypasses the abort() and allows refcount to wrap, potentially causing use-after-free. Although current call sites use hardcoded small values (1 and 2), the public API accepts arbitrary usize and should validate the input.

🛠️ Suggested fix 
 pub fn inc_by(&self, n: usize) {
+    if n > Self::MASK {
+        std::process::abort();
+    }
     let old_size = self.strong.fetch_add(n, Relaxed);
-
     if old_size & Self::MASK > Self::MASK - n {
         std::process::abort();
     }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#[inline]
pub fn inc_by(&self, n: usize) {
let old_size = self.strong.fetch_add(n, Relaxed);
if old_size & Self::MASK == Self::MASK {
if old_size & Self::MASK > Self::MASK - n {
std::process::abort();
#[inline]
pub fn inc_by(&self, n: usize) {
if n > Self::MASK {
std::process::abort();
}
let old_size = self.strong.fetch_add(n, Relaxed);
if old_size & Self::MASK > Self::MASK - n {
std::process::abort();
}
}
🤖 Prompt for AI Agents 
In `@crates/common/src/refcount.rs` around lines 39 - 44, The inc_by method must
validate its n argument before using it to detect overflow; add an explicit
bounds check at the top of pub fn inc_by(&self, n: usize) so that if n == 0 it
returns early and if n > Self::MASK it aborts (to avoid subtraction underflow
when computing Self::MASK - n). After that, keep the existing fetch_add/overflow
check (using self.strong.fetch_add) unchanged; this ensures the existing
overflow detection logic remains correct for valid n values (<= MASK).

Copy link
Contributor

@fanninpm fanninpm Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@youknowone This seems cogent enough to warrant a fix.

Copy link
Contributor

@coderabbitai coderabbitai bot Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

}
}

/// Returns true if successful
#[inline]
pub fn safe_inc(&self) -> bool {
Expand Down
18 changes: 16 additions & 2 deletions crates/vm/src/object/core.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,12 @@ impl WeakRefList {
}))
});
let mut inner = unsafe { inner_ptr.as_ref().lock() };
// If obj was cleared but object is still alive (e.g., new weakref
// created during __del__), restore the obj pointer
if inner.obj.is_none() {
inner.obj = Some(NonNull::from(obj));
inner.ref_count += 1;
}
if is_generic && let Some(generic_weakref) = inner.generic_weakref {
let generic_weakref = unsafe { generic_weakref.as_ref() };
if generic_weakref.0.ref_count.get() != 0 {
Expand Down Expand Up @@ -835,15 +841,23 @@ impl PyObject {
slot_del: fn(&PyObject, &VirtualMachine) -> PyResult<()>,
) -> Result<(), ()> {
let ret = crate::vm::thread::with_vm(zelf, |vm| {
zelf.0.ref_count.inc();
// Temporarily resurrect (0→2) so ref_count stays positive
// during __del__, preventing safe_inc from seeing 0.
zelf.0.ref_count.inc_by(2);

if let Err(e) = slot_del(zelf, vm) {
let del_method = zelf.get_class_attr(identifier!(vm, __del__)).unwrap();
vm.run_unraisable(e, None, del_method);
}

// Undo the temporary resurrection. Always remove both
// temporary refs; the second dec returns true only when
// ref_count drops to 0 (no resurrection).
zelf.0.ref_count.dec();
zelf.0.ref_count.dec()
});
match ret {
// the decref right above set ref_count back to 0
// the decref set ref_count back to 0
Some(true) => Ok(()),
// we've been resurrected by __del__
Some(false) => Err(()),
Expand Down
Loading