GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Replace unsafe pointer cast with PyMutex in PyCode by youknowone · Pull Request #7055 · RustPython/RustPython 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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
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).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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.
66eb96f to
5103e88
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
The reason will be displayed to describe this comment to others. Learn more.
🏁 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 -A5
Repository: 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 -100
Repository: 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 -A8
Repository: 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:
AtomicPtr or atomic reference) for signal-handler reads, ortry_lock() with a fallback (e.g., print "<unknown>" if the lock is contended).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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@youknowone doesn't Since the lifetime is What the update code does is actually an atomic compare and exchange, I think. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@jaens Thanks! That sounds right |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.