Replace unsafe pointer cast with PyMutex in PyCode#7055
Replace unsafe pointer cast with PyMutex in PyCode#7055youknowone merged 1 commit intoRustPython:mainfrom
Conversation
📝 WalkthroughWalkthroughEncapsulate PyCode.source_path behind a PyMutex with getter/setter and replace direct field accesses across the VM to use the new methods; PyCode construction updated to use PyCode::new. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/vm/src/builtins/code.rs`:
- Around line 337-351: Replace direct reads of the CodeObject field
"frame.code.source_path" with the PyCode accessor "frame.code.source_path()" so
the mutex-protected value from PyCode::source_path is used; search for
occurrences of "frame.code.source_path" in faulthandler.rs and update them to
call the accessor (leveraging the existing PyCode::source_path and
set_source_path methods).
Add `source_path: PyMutex<&'static PyStrInterned>` field to `PyCode` for interior mutability, replacing the UB-inducing `#[allow(invalid_reference_casting)]` + `write_volatile` pattern in `update_code_filenames`. Update all read sites across the codebase to use the new `source_path()` accessor method.
youknowone
force-pushed
the
pycode-path
branch
from
66eb96f to
5103e88
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/stdlib/src/faulthandler.rs`:
- Around line 254-256: dump_frame_from_raw is called from the signal handler
faulthandler_fatal_error but calls frame.code.source_path() which acquires a
PyMutex and can deadlock; make this signal-safe by avoiding any mutex
acquisition: either add a signal-safe cached string pointer on PyCode (e.g., an
atomic/AtomicPtr holding a nul-terminated C string updated outside signal
handlers and read here) and read that in dump_frame_from_raw, or replace the
call to source_path() with a non-blocking attempt (call try_lock() on the
PyCode/source_path mutex and if it fails print a fallback like "<unknown>" or
the raw pointer/address), updating references to Frame/ PyCode/ source_path
accordingly so dump_frame_from_raw never blocks in a signal context.
| fn dump_frame_from_raw(fd: i32, frame: &Frame) { | ||
| let filename = frame.code.source_path.as_str(); | ||
| let filename = frame.code.source_path().as_str(); | ||
| let funcname = frame.code.obj_name.as_str(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find PyMutex definition
echo "=== PyMutex Definition ==="
rg -n 'type PyMutex|struct PyMutex' --type rust -A3 -B1
echo ""
echo "=== source_path method signature and impl ==="
# Search for source_path method definition
rg -n 'fn source_path' --type rust -A8
echo ""
echo "=== Check signal handler context ==="
# Find faulthandler_fatal_error to confirm it's a signal handler
rg -n 'faulthandler_fatal_error|extern "C"' crates/stdlib/src/faulthandler.rs -B2 -A5Repository: RustPython/RustPython
Length of output: 3633
🏁 Script executed:
#!/bin/bash
# Let's check the specific file to understand the context better
echo "=== Checking faulthandler.rs structure ==="
cat -n crates/stdlib/src/faulthandler.rs | head -300 | tail -100Repository: RustPython/RustPython
Length of output: 4032
🏁 Script executed:
#!/bin/bash
# Check what the Frame structure looks like and where source_path comes from
echo "=== Frame structure and code field ==="
rg -n 'struct Frame|pub struct Frame' --type rust -A10
echo ""
echo "=== PyCode structure related to source_path ==="
rg -n 'pub struct PyCode|source_path' --type rust -B2 -A8Repository: RustPython/RustPython
Length of output: 48965
source_path() acquires a mutex inside a signal handler — potential deadlock.
dump_frame_from_raw (line 254) is reachable from faulthandler_fatal_error, which is a signal handler. Calling source_path() at line 255 locks a PyMutex. If the signal fires while the same PyCode's source_path mutex is held elsewhere, this causes a deadlock.
The comment on line 252 explicitly documents this function as "(signal-safe)", which is now violated. Consider either:
- Storing a separate signal-safe copy (e.g.,
AtomicPtroratomicreference) for signal-handler reads, or - Using
try_lock()with a fallback (e.g., print"<unknown>"if the lock is contended).
🤖 Prompt for AI Agents
In `@crates/stdlib/src/faulthandler.rs` around lines 254 - 256,
dump_frame_from_raw is called from the signal handler faulthandler_fatal_error
but calls frame.code.source_path() which acquires a PyMutex and can deadlock;
make this signal-safe by avoiding any mutex acquisition: either add a
signal-safe cached string pointer on PyCode (e.g., an atomic/AtomicPtr holding a
nul-terminated C string updated outside signal handlers and read here) and read
that in dump_frame_from_raw, or replace the call to source_path() with a
non-blocking attempt (call try_lock() on the PyCode/source_path mutex and if it
fails print a fallback like "<unknown>" or the raw pointer/address), updating
references to Frame/ PyCode/ source_path accordingly so dump_frame_from_raw
never blocks in a signal context.
|
@youknowone doesn't Since the lifetime is What the update code does is actually an atomic compare and exchange, I think. |
|
@jaens Thanks! That sounds right |
2 participants
Add
source_path: PyMutex<&'static PyStrInterned>field toPyCodefor interior mutability, replacing the UB-inducing#[allow(invalid_reference_casting)]+write_volatilepattern inupdate_code_filenames. Update all 12 read sites across the codebase to use the newsource_path()method.Summary by CodeRabbit