FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix store subscr list finalizer deadlock by teddygood · Pull Request #8956 · RustPython/RustPython · GitHub

  1. RustPython
  2. RustPython

Repository navigation

Fix store subscr list finalizer deadlock - #8956

Merged
youknowone merged 2 commits into
RustPython:mainfrom
teddygood:fix-store-subscr-list-finalizer-deadlock
Oct 4, 2026
ConversationCommits (2)ChecksFiles changed
Merged

Fix store subscr list finalizer deadlock#8956
youknowone merged 2 commits into
RustPython:mainfrom
teddygood:fix-store-subscr-list-finalizer-deadlock

Conversation

teddygood commented Oct 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #8955
Follow-up to #8894.

One of checkbox below must be checked.

  • I did not use AI to write the code of this patch.
  • This PR follows our AI policy

Summary

STORE_SUBSCR_LIST_INT assigned the new item while holding the list's write guard, so the replaced element was dropped under the lock. If that element's __del__ read the same list, the interpreter deadlocked as soon as the store was specialized.

The handler now takes the old element out with core::mem::replace, releases the guard and then drops it. This is the rule #8894 applied to list.__setitem__, and the order CPython uses for this instruction (UNLOCK_OBJECT(list); // unlock before decrefs!).

AI assistance

Written with Claude (Opus 5.5), reviewed by a human before submission.

chatgpt-codex-connector Bot commented Oct 4, 2026

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

github-actions Bot added the z-ca-2026 Tag to track Contribution Academy 2026 label Oct 4, 2026

coderabbitai Bot commented Oct 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info ⚙️ Run configuration
  • Configuration used: Repository: RustPython/RustPython/.coderabbit.yml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6a46ef14-fb61-4204-8579-d7fde20b76f7
📥 Commits

Reviewing files that changed from the base of the PR and between f39b054 and c633b28.

📒 Files selected for processing (2)
  • crates/vm/src/frame.rs
  • extra_tests/snippets/vm_specialization.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Walkthrough

The specialized integer-index list store now releases its mutable list borrow before dropping the replaced value. A regression test warms the specialization and checks that each finalizer can inspect the updated list.

Changes

Specialized list-store finalizer behavior

Layer / File(s) Summary
Release the list borrow before dropping the replaced value
crates/vm/src/frame.rs, extra_tests/snippets/vm_specialization.py		 The fast path saves the replaced element, releases the mutable list borrow, and then drops the element. The new test warms STORE_SUBSCR_LIST_INT and checks that finalizers observe a list of length 1 with the replaced item no longer in the slot.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: youknowone

Merge Risk: ⚪ Minimal · up to c633b

The change addresses the reported finalizer deadlock, and no actionable merge-blocking issue is established by the supplied evidence.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to c633b

The change removes a finalizer-triggered deadlock while preserving existing input checks and access rights. No new material security risk was identified in the reviewed change.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected availability behavior is within an interpreter executing eligible Python list assignments whose displaced objects have finalizers. The inspected change does not broaden eligible inputs or grant finalizers additional authority; embedding and multi-tenant exposure are not established by this evidence.

Security Findings and Attack Paths

  • inferred — The finalizer-triggered deadlock mechanism predates this PR: destruction under the list guard can reenter code that accesses the same list. Moving destruction after guard release removes that lock-reentry mechanism rather than introducing a new attack path.

Trust Boundaries and Controls

  • observed — Python-supplied objects and indices still pass the existing type and bounds controls before guarded replacement. Unsupported cases still delegate to the generic operation; the changed branch adds no privileged operation or new callback source.
🚥 Pre-merge checks | ✅ 4 | ❓ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ❓ Inconclusive Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #8955 requires specialized list item assignment to avoid deadlock when the replaced item’s finalizer reads the list. In frame.rs, STORE_SUBSCR_LIST_INT replaces the item with `core::mem::rep…
Out of Scope Changes check ✅ Passed The frame.rs change fixes issue #8955. The vm_specialization.py test covers the same finalizer and lock behavior. No unrelated changes appear in the reviewed diff.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the fix for a deadlock caused by a list element finalizer during specialized store-subscript handling.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files. (1 skipped: 1 too large.)

✨ Finishing Touches 🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

codspeed Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 62 untouched benchmarks
⏩ 4 skipped benchmarks1

Comparing teddygood:fix-store-subscr-list-finalizer-deadlock (c633b28) with main (f39b054)

Footnotes

  1. 4 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

youknowone approved these changes Oct 4, 2026
View reviewed changes

youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

👍 Thanks!

youknowone merged commit 7536730 into RustPython:main Oct 4, 2026
22 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

youknowone youknowone approved these changes

Assignees

teddygood

Labels

z-ca-2026 Tag to track Contribution Academy 2026

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Specialized list item assignment deadlocks when the replaced element's finalizer reads the list

2 participants

Back | FazBrowse Home | New Git URL