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

luantaraschi · 2026-10-06T05:22:26Z

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 to the list 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 in the returned state.

chatgpt-codex-connector · 2026-10-06T05:22:33Z

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.

coderabbitai · 2026-10-06T05:22:59Z

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: c7eea2c4-ede8-4a26-81b9-e84ad860886b

📥 Commits

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

⛔ Files ignored due to path filters: 1

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

📒 Files selected for processing: 1

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

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: youknowdot

Merge Risk: ⚪ Minimal · up to e66a9

No actionable merge-blocking issue is established. Complete the normal checks before merging.

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		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		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main changes: fixing the deadlock and panic in object.getstate when processing slotnames.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		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.}

github-actions · 2026-10-06T05:23:19Z

📦 Library Dependencies

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

:4: SyntaxWarning: invalid decimal literal
:7: SyntaxWarning: "\z" is an invalid escape sequence. Such sequences will not work in the future. Did you mean "\z"? A raw string is also an option.
:10: SyntaxWarning: "\400" is an invalid octal escape sequence. Such sequences will not work in the future. Did you mean "\400"? A raw string is also an option.
:4: SyntaxWarning: invalid decimal literal
:7: SyntaxWarning: "\z" is an invalid escape sequence. Such sequences will not work in the future. Did you mean "\z"? A raw string is also an option.
:10: SyntaxWarning: "\400" is an invalid octal escape sequence. Such sequences will not work in the future. Did you mean "\400"? A raw string is also an option.
:4: SyntaxWarning: invalid decimal literal
:7: SyntaxWarning: "\z" is an invalid escape sequence. Such sequences will not work in the future. Did you mean "\z"? A raw string is also an option.
:10: SyntaxWarning: "\400" is an invalid octal escape sequence. Such sequences will not work in the future. Did you mean "\400"? A raw string is also an option.
[ ] 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

codspeed · 2026-10-06T05:41:27Z

Merging this PR will improve performance by 10.98%

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

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

youknowone · 2026-10-08T02:24:40Z

@luantaraschi thank you for the patch! windows test hang looks like regression of this patch. could you take a look?

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

luantaraschi · 2026-10-15T08:31:11Z

I looked into it, and I don't think it's this patch. The same four vm::interpreter tests (owned_subinterpreter_cleanup_is_scoped_to_its_runtime, stop_the_world_parks_threads_of_another_interpreter, subinterpreter_new_thread_shares_sub_state, subinterpreter_runs_python_code) hang the same way in main push runs without it, e.g. 36514108240 (Windows, 09-29) and 37558600162 (Ubuntu, 10-07), plus macOS runs and other branches. They never go through __getstate__/__reduce_ex__, which is all this changes. It looks like what #9094 was going after, and I haven't seen it in CI since that merged, though that's only a day of runs.

I rebased onto main. The only conflict was in builtin_object.py, where both sides appended cases at the end, so I kept both. On Linux the rust test job passes, and vm::interpreter ran 300 times in a row without a hang, but I can't run Windows here, so the new CI run is the real check.

youknowone

Thank you for confirming! and sorry to ask you to check a fake issue.
Probably it was flaky and I was not lucky enough to pass it on second run.

luantaraschi force-pushed the fix/object-getstate-guard branch from 0026fce to e66a99c Compare October 15, 2026 08:31

youknowone approved these changes Oct 15, 2026

View reviewed changes

youknowone merged commit 8de2cbe into RustPython:main Oct 15, 2026
21 checks passed

Navigation Menu

Repository navigation

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

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

luantaraschi commented Oct 6, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

chatgpt-codex-connector Bot commented Oct 6, 2026

Uh oh!

coderabbitai Bot commented Oct 6, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 6, 2026 •

edited

Loading

Uh oh!

codspeed Bot commented Oct 6, 2026

Uh oh!

youknowone commented Oct 8, 2026

Uh oh!

luantaraschi commented Oct 15, 2026

Uh oh!

youknowone left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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!

Uh oh!

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

Uh oh!

📦 Library Dependencies

Uh oh!

codspeed Bot commented Oct 6, 2026

Merging this PR will improve performance by 10.98%

Performance Changes

Footnotes

Uh oh!

youknowone commented Oct 8, 2026

Uh oh!

luantaraschi commented Oct 15, 2026

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

luantaraschi commented Oct 6, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 6, 2026 •

edited

Loading

github-actions Bot commented Oct 6, 2026 •

edited

Loading