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 sysmax_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 scenarion_large = max_refcount + 1000mask = max_refcountresult = 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]
pubfn 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]
pubfn 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.
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.
// 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.