Fix object.getstate deadlock and panic on slotnames by luantaraschi · Pull Request #8981 · RustPython/RustPython · GitHub

This PR follows our AI policy

Summary

object_getstate_default held the __slotnames__ read lock while it looked up each slot attribute. If a getter or __getattr__ mutates the list during that lookup, which is exactly what test_descr.test_issue24097 does, it blocks on the write lock forever. On main that test hangs until the runner's 30 s timeout, so it was skipped.

A non-str entry in a cached __slotnames__ hit downcast_ref::<PyStr>().unwrap() instead:

class C:
    __slotnames__ = [1]

C().__getstate__()

thread 'main' panicked at crates/vm/src/builtins/object.rs:234:70:
called `Option::unwrap()` on a `None` value

The loop now clones each name out of the list and drops the lock before the lookup, and raises TypeError: attribute name must be string, not 'int' for a non-str name. Only AttributeError is ignored now (through get_attribute_opt): before, any exception from the lookup was swallowed, so a __getattr__ raising ValueError gave a state of None where CPython raises. The size check runs after each item, as CPython does.

Compared with CPython 3.14.7: the non-str and ValueError cases now raise the same exception and message. The mutating case raises RuntimeError like CPython does, with our existing message (CPython spells it __slotsname__). A missing attribute is still skipped.

test_issue24097 in test_descr.py is unskipped and passes. Full test_descr: OK.
4 cases added to extra_tests/snippets/builtin_object.py.
test_copy, test_copyreg, test_pickle, test_collections, test_dataclasses, test_enum, test_xml_etree, test_functools, test_deque and test_io pass as on main.
cargo test for the workspace (with the AGENTS.md exclusions), clippy -Dwarnings, cargo fmt --check and check_redundant_patches.py are clean. Linux only.

Written with Claude Code (claude-opus-5-5) as a tool. I reviewed the diff and reproduced the hang and the panic before and after the change myself.

Summary by CodeRabbit

Bug Fixes
- Improved object.__getstate__ handling of slot names: invalid entries and changes during lookup now raise clear errors, and errors reading slot values are no longer suppressed.
- Missing slots remain omitted, while available slot values are included.

object_getstate_default kept the __slotnames__ list read-locked while it looked up each slot attribute. A getter or __getattr__ that changed the list (as in test_descr.test_issue24097) then waited forever for the write lock. A non-str entry in a cached __slotnames__ hit an unwrap and panicked. Take a reference to each name and drop the lock before the lookup, raise TypeError for a non-str name, let errors other than AttributeError propagate, and check the list size after each item, as CPython does. Assisted-by: Claude Code:claude-opus-5-5

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.

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: 04adb0da-a85b-457d-b469-9ce21d55d87b

📥 Commits

Reviewing files that changed from the base of the PR and between 3e0e401 and 0026fce.

⛔ Files ignored due to path filters (1)

Lib/test/test_descr.py is excluded by !Lib/**

📒 Files selected for processing (2)

crates/vm/src/builtins/object.rs
extra_tests/snippets/builtin_object.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

object.__getstate__ now validates cached slot names, releases the list lock during attribute lookup, propagates lookup and insertion errors, and raises RuntimeError if the slot-name list changes length. Tests cover these cases and missing slots.

Changes

Object getstate slot handling

Layer / File(s)	Summary
Slot processing and validation crates/vm/src/builtins/object.rs, extra_tests/snippets/builtin_object.py	Slot processing validates each name, releases the list lock during attribute lookup, propagates lookup and insertion errors, and checks for list-length changes. Tests cover invalid names, mutations during lookup, propagated exceptions, and missing slots.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: youknowone

Merge Risk: ⚪ Minimal · up to 0026f

No merge-blocking issue is identified; merge after normal checks.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 0026f

The change removes the reported deadlock and invalid-name panic paths, but introduces another possible native panic when a concurrent thread shrinks the slot-name list before an indexed read. This requires control of the list and concurrent execution; broader service impact or privilege escalation is not established.

Retained concerns

Medium · security · inferred: The new slot-name iteration separates length validation from indexed access. A permitted concurrent thread can shrink the class's cached list after its initial length read or a previous post-lookup check, causing borrow_vec()[i] to panic before the next RuntimeError check. The base validated length and indexed under one guard, preventing this interleaving. This introduces a native failure path outside Python exception handling. Exposure requires mutable access to that list and concurrent execution within the interpreter; host-process consequences depend on embedding and panic policy.

Security review details

Security Blast Radius

inferred — The established exposure is serialization within a threaded interpreter where code can mutate the cached slot-name list. A native panic can disrupt the invoking execution context. No remote entrypoint, cross-tenant reachability, or privilege gain is established by the inspected call paths; whole-process impact remains embedding-dependent.

Security Findings and Attack Paths

inferred — A concurrent mutator can clear the cached list after the serializer reads its original length but before the serializer acquires the next read guard. Indexing the shortened slice then raises a Rust bounds panic, not a Python exception. The same race can occur between iterations. This is a newly introduced failure path supported by source analysis, not an executed exploit.

Trust Boundaries and Controls

observed — Thread creation retains its audit event, isolated-subinterpreter restriction, and shutdown check. The concern does not require bypassing these controls: it applies where concurrent execution is permitted and Python-controlled list contents reach unchecked native indexing.

Resilience and Maintainability Implications

inferred — Normal serialization errors preserve local result containment, but a Rust panic is outside the thread runner's Python-error handling. If serialization runs in such a worker, unwinding can skip its later sentinel release and thread-count cleanup. VM attachment itself has an unwind cleanup guard, limiting but not eliminating the recovery concern.

Hardening Proposals

proposed — Validate the current list length and obtain the next name under the same read guard, returning a Python error rather than indexing an invalid position. Release that guard before attribute lookup and retain the post-callback size check. This preserves reentrant callback handling while restoring atomic validation of each read.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	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 3 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: fixing the deadlock and panic related to __slotnames__ in object.__getstate__.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Fix all pre-merge checks with AI

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

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] test: cpython/Lib/test/test_descr.py (TODO: 2)
[ ] test: cpython/Lib/test/test_descrtut.py (TODO: 2)

dependencies:

dependent tests: (no tests depend on descr)

Legend:

[+] path exists in CPython
[x] up-to-date, [ ] outdated

Merging this PR will improve performance by 10.98%

⚡ 1 improved benchmark
✅ 61 untouched benchmarks
⏩ 4 skipped benchmarks1

Performance Changes

	Benchmark	BASE	HEAD	Efficiency
⚡	rustpython[loop_string.py]	936.6 µs	843.9 µs	+10.98%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.

Comparing luantaraschi:fix/object-getstate-guard (0026fce) with main (3e0e401)

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

Fix object.getstate deadlock and panic on slotnames by luantaraschi · Pull Request #8981 · RustPython/RustPython · GitHub

Repository navigation

Fix object.getstate deadlock and panic on slotnames - #8981

Fix object.getstate deadlock and panic on slotnames#8981
luantaraschi wants to merge 1 commit into
RustPython:mainfrom
luantaraschi:fix/object-getstate-guard

luantaraschi commented Oct 6, 2026 •
edited by coderabbitai Bot
Loading

Uh oh!

Uh oh!

chatgpt-codex-connector Bot commented Oct 6, 2026

Uh oh!

coderabbitai Bot commented Oct 6, 2026 •
edited
Loading

Uh oh!

Uh oh!

github-actions Bot commented Oct 6, 2026

Uh oh!

codspeed Bot commented Oct 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Fix object.__getstate__ deadlock and panic on __slotnames__ by luantaraschi · Pull Request #8981 · RustPython/RustPython · GitHub

Repository navigation

Fix object.__getstate__ deadlock and panic on __slotnames__ - #8981

Fix object.__getstate__ deadlock and panic on __slotnames__#8981luantaraschi wants to merge 1 commit intoRustPython:mainRustPython/RustPython:mainfrom luantaraschi:fix/object-getstate-guardluantaraschi/RustPython:fix/object-getstate-guardCopy head branch name to clipboard

Conversation

luantaraschi commented Oct 6, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Summary by CodeRabbit

Uh oh!

chatgpt-codex-connector Bot commented Oct 6, 2026

Uh oh!

coderabbitai Bot commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Security Architecture Review

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Oct 6, 2026

📦 Library Dependencies

Uh oh!

codspeed Bot commented Oct 6, 2026

Merging this PR will improve performance by 10.98%

Performance Changes

Footnotes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Fix object.getstate deadlock and panic on slotnames by luantaraschi · Pull Request #8981 · RustPython/RustPython · GitHub

Fix object.getstate deadlock and panic on slotnames - #8981

Fix object.getstate deadlock and panic on slotnames#8981
luantaraschi wants to merge 1 commit into
RustPython:mainfrom
luantaraschi:fix/object-getstate-guard

luantaraschi commented Oct 6, 2026 •
edited by coderabbitai Bot
Loading

coderabbitai Bot commented Oct 6, 2026 •
edited
Loading