Align del behavior #6772
Align del behavior #6772
Conversation
📝 WalkthroughWalkthroughRemoved specific multiprocessing Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant DropLogic as drop_slow_inner
participant RefCount as RefCount
participant Object as Obj
participant Finalizer as __del__
DropLogic->>RefCount: inc_by(2) (temporary resurrect)
DropLogic->>Finalizer: call_slot_del(Object)
Finalizer->>Object: run __del__
Note right of Object: __del__ may create new refs (resurrect)
Finalizer-->>DropLogic: return
DropLogic->>RefCount: dec() (original)
DropLogic->>RefCount: dec() (post-finalizer)
DropLogic->>DropLogic: compare strong_count to detect resurrection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
youknowone
force-pushed
the
del
branch
3 times, most recently
from
5ae9172 to
378f737
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/common/src/refcount.rs`:
- Around line 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).
In `@crates/vm/src/object/core.rs`:
- Around line 223-227: When reattaching an object in the block that checks `if
inner.obj.is_none() { inner.obj = Some(NonNull::from(obj)); }`, also restore the
weakref invariant by incrementing `inner.ref_count` to reflect the "weakrefs + 1
if alive" ownership; i.e., when you set `inner.obj = Some(...)` increment
`inner.ref_count` (using the same semantics as `clear()` decremented it — e.g.,
checked_add or the crate's existing ref-count increment helper) so the list
won't be dropped while the object is alive and avoid overflow/underflow by
matching the existing ref-count handling used elsewhere (e.g., the `clear()` and
ref-count manipulation functions).
- Around line 843-867: The destructor path increments temporary refs via
zelf.0.ref_count.inc_by(2) then on a resurrection only performs one decrement,
leaking one temporary ref; change the logic so both temporary decs always run
(call zelf.0.ref_count.dec() twice) and use the return/result of the second dec
to determine whether references remain (i.e., whether to treat the object as
still alive) instead of skipping the second dec on resurrection; update the
block around strong_count(), slot_del(zelf, vm), and the two ref_count.dec()
calls (and the resurrection check that currently uses after_del > after_inc) so
the second dec's result drives the decision.
| #[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(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n crates/common/src/refcount.rs | head -80Repository: 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.rsRepository: RustPython/RustPython
Length of output: 227
🏁 Script executed:
# Find all call sites of inc_by
rg -n "inc_by" --type rsRepository: 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 -5Repository: 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 -nRepository: 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()")
EOFRepository: 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 5Repository: 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.
| #[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).
There was a problem hiding this comment.
@youknowone This seems cogent enough to warrant a fix.
There was a problem hiding this comment.
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!
crates/vm/src/object/core.rs
Outdated
| // Increment twice (0→2) so we can decrement twice below. | ||
| zelf.0.ref_count.inc_by(2); | ||
| let after_inc = zelf.strong_count(); // Should be 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); | ||
| } | ||
|
|
||
| let after_del = zelf.strong_count(); | ||
|
|
||
| // First decrement | ||
| zelf.0.ref_count.dec(); | ||
|
|
||
| // Check for resurrection: if ref_count increased beyond our expected 2, | ||
| // then __del__ created new references (resurrection occurred). | ||
| if after_del > after_inc { | ||
| // Resurrected - don't do second decrement, leave object alive | ||
| return false; | ||
| } | ||
|
|
||
| // No resurrection - do second decrement to get back to 0 | ||
| // This matches the double increment from inc() | ||
| zelf.0.ref_count.dec() | ||
| }); |
There was a problem hiding this comment.
Resurrection path leaks one temporary refcount.
We inc_by(2) but on resurrection only one decrement runs, leaving an extra +1 and preventing eventual drop. Remove both temporary refs regardless; the second dec() return value already indicates whether references remain.
🛠️ Suggested fix
- zelf.0.ref_count.inc_by(2);
- let after_inc = zelf.strong_count(); // Should be 2
+ 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);
}
- let after_del = zelf.strong_count();
-
- // First decrement
- zelf.0.ref_count.dec();
-
- // Check for resurrection: if ref_count increased beyond our expected 2,
- // then __del__ created new references (resurrection occurred).
- if after_del > after_inc {
- // Resurrected - don't do second decrement, leave object alive
- return false;
- }
-
- // No resurrection - do second decrement to get back to 0
- // This matches the double increment from inc()
- zelf.0.ref_count.dec()
+ // Remove the two temporary refs added above. The second
+ // decrement returns true only if no refs remain.
+ zelf.0.ref_count.dec();
+ zelf.0.ref_count.dec()📝 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.
| // Increment twice (0→2) so we can decrement twice below. | |
| zelf.0.ref_count.inc_by(2); | |
| let after_inc = zelf.strong_count(); // Should be 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); | |
| } | |
| let after_del = zelf.strong_count(); | |
| // First decrement | |
| zelf.0.ref_count.dec(); | |
| // Check for resurrection: if ref_count increased beyond our expected 2, | |
| // then __del__ created new references (resurrection occurred). | |
| if after_del > after_inc { | |
| // Resurrected - don't do second decrement, leave object alive | |
| return false; | |
| } | |
| // No resurrection - do second decrement to get back to 0 | |
| // This matches the double increment from inc() | |
| zelf.0.ref_count.dec() | |
| }); | |
| // Increment twice (0→2) so we can decrement twice below. | |
| 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); | |
| } | |
| // Remove the two temporary refs added above. The second | |
| // decrement returns true only if no refs remain. | |
| zelf.0.ref_count.dec(); | |
| zelf.0.ref_count.dec() | |
| }); |
🤖 Prompt for AI Agents
In `@crates/vm/src/object/core.rs` around lines 843 - 867, The destructor path
increments temporary refs via zelf.0.ref_count.inc_by(2) then on a resurrection
only performs one decrement, leaking one temporary ref; change the logic so both
temporary decs always run (call zelf.0.ref_count.dec() twice) and use the
return/result of the second dec to determine whether references remain (i.e.,
whether to treat the object as still alive) instead of skipping the second dec
on resurrection; update the block around strong_count(), slot_del(zelf, vm), and
the two ref_count.dec() calls (and the resurrection check that currently uses
after_del > after_inc) so the second dec's result drives the decision.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] lib: cpython/Lib/hmac.py dependencies:
dependent tests: (4 tests)
[ ] lib: cpython/Lib/multiprocessing dependencies:
dependent tests: (5 tests)
Legend:
|
fanninpm
left a comment
fanninpm
left a comment
There was a problem hiding this comment.
I do not have write access to the repository, so I cannot approve a PR to clear it for merging.
|
@fanninpm sorry, I fixed it. that was not intended. |
fanninpm
left a comment
fanninpm
left a comment
There was a problem hiding this comment.
Looks good to me, aside from the one thing the AI brought up.
Thanks! |
2 participants
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.