⚠️ Potential issue | 🔴 Critical

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

 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();
     }
 }

‼️ 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.

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

@youknowone This seems cogent enough to warrant a fix.

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!