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 1 commit
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
Prev Previous commit
Next Next commit
refcount inc_by for atomicity
  • Loading branch information
@youknowone
youknowone committed Jan 31, 2026
commit 896c0e1797e8a13046641929f3cb202f6ecf610c
9 changes: 9 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,15 @@ impl RefCount {
}
}

#[inline]
pub fn inc_by(&self, n: usize) {
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
3 changes: 1 addition & 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 @@ -841,8 +841,7 @@ impl PyObject {
) -> Result<(), ()> {
let ret = crate::vm::thread::with_vm(zelf, |vm| {
// Increment twice (0→2) so we can decrement twice below.
zelf.0.ref_count.inc();
zelf.0.ref_count.inc();
zelf.0.ref_count.inc_by(2);
let after_inc = zelf.strong_count(); // Should be 2

if let Err(e) = slot_del(zelf, vm) {
Expand Down