GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Align del behavior by youknowone · Pull Request #6772 · RustPython/RustPython

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 RustPython:main from youknowone:del
Jan 31, 2026

Uh oh!

There was an error while loading. Please reload this page.

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


Clear filters
Conversations
Failed to load comments.
Loading

Uh oh!

There was an error while loading. Please reload this page.

Jump to
Jump to file
Failed to load files.
Loading

Uh oh!

There was an error while loading. Please reload this page.

Diff view
Diff view

There are no files selected for viewing

5 changes: 0 additions & 5 deletions 5 .github/workflows/ci.yaml
Show comments
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 6 Lib/test/_test_multiprocessing.py
Show comments
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 17 Lib/test/test_multiprocessing_fork/test_processes.py
Show comments
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 22 Lib/test/test_multiprocessing_forkserver/test_processes.py
Show comments
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 8 Lib/test/test_multiprocessing_forkserver/test_threads.py
Show comments
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 22 Lib/test/test_multiprocessing_spawn/test_processes.py
Show comments
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 10 crates/common/src/refcount.rs
Show comments
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 thread
Comment on lines +43 to 49

@coderabbitai coderabbitai Bot Jan 31, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@fanninpm fanninpm Jan 31, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@coderabbitai coderabbitai Bot Jan 31, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
}
}

/// Returns true if successful
#[inline]
pub fn safe_inc(&self) -> bool {
Expand Down
18 changes: 16 additions & 2 deletions 18 crates/vm/src/object/core.rs
Show comments
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;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved

Uh oh!

There was an error while loading. Please reload this page.

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
Oops, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.