Skip to content

More fork safety #7380

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 9 commits into from
Mar 8, 2026
Merged

More fork safety #7380

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6217ad6
apply more allow_threads
youknowone Mar 7, 2026
6b6138b
Simplify STW thread state transitions
youknowone Mar 7, 2026
396fec9
Representable for ThreadHandle
youknowone Mar 7, 2026
9fe70f2
Set ThreadHandle state to Running in parent thread after spawn
youknowone Mar 7, 2026
e37b15f
Set ThreadHandle state to Running in parent thread after spawn
youknowone Mar 7, 2026
cfd7f01
Add debug_assert for thread state in start_the_world
youknowone Mar 8, 2026
1b97419
Unskip now-passing test_get_event_loop_thread and test_start_new_thre…
youknowone Mar 7, 2026
1ba2821
Wrap IO locks and file ops in allow_threads
youknowone Mar 8, 2026
ca41577
Use std::sync for thread start/ready events
youknowone Mar 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add debug_assert for thread state in start_the_world
  • Loading branch information
@youknowone
youknowone committed Mar 8, 2026
commit cfd7f011aacfd7b0dcf9f14e4668285bc07699fe
4 changes: 2 additions & 2 deletions crates/vm/src/stdlib/posix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -935,14 +935,14 @@ pub mod module {
Ok(n) => n,
Err(_) => return 0,
};
let line = match std::str::from_utf8(&buf[..n]) {
let line = match core::str::from_utf8(&buf[..n]) {
Ok(s) => s,
Err(_) => return 0,
};
if let Some(field) = line.split_whitespace().nth(19) {
return field.parse::<isize>().unwrap_or(0);
}
return 0;
0
}
Comment on lines +926 to +946
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 8, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Linux: /proc/self/stat parsing is fragile when comm contains spaces.

The comm field (field 2 in /proc/self/stat) is enclosed in parentheses and can contain spaces, parentheses, or newlines. Using split_whitespace().nth(19) will yield incorrect results if the process name contains spaces.

For example, a process named "my app" produces:

123 (my app) S 456 ...

split_whitespace() splits this into ["123", "(my", "app)", "S", ...], making nth(19) point to the wrong field.

CPython handles this by finding the last ) to locate the end of the comm field, then counting fields from there.

🛡️ Suggested fix to handle comm field correctly 
         #[cfg(target_os = "linux")]
         {
             use std::io::Read as _;
             let mut file = match std::fs::File::open("/proc/self/stat") {
                 Ok(f) => f,
                 Err(_) => return 0,
             };
             let mut buf = [0u8; 160];
             let n = match file.read(&mut buf) {
                 Ok(n) => n,
                 Err(_) => return 0,
             };
             let line = match core::str::from_utf8(&buf[..n]) {
                 Ok(s) => s,
                 Err(_) => return 0,
             };
-            if let Some(field) = line.split_whitespace().nth(19) {
-                return field.parse::<isize>().unwrap_or(0);
+            // Find end of comm field (last ')') to handle spaces in process name
+            let after_comm = match line.rfind(')') {
+                Some(idx) => &line[idx + 1..],
+                None => return 0,
+            };
+            // Field 20 (num_threads) is the 17th field after comm (fields 3-19 are 17 fields)
+            if let Some(field) = after_comm.split_whitespace().nth(17) {
+                return field.parse::<isize>().unwrap_or(0);
             }
             0
         }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/stdlib/posix.rs` around lines 926 - 946, The current parsing of
/proc/self/stat is fragile because split_whitespace() will break when the comm
field (inside parentheses) contains spaces; instead, after reading the file and
converting to string, find the index of the last ')' in the line, take the
substring after that character, then split_whitespace() on that remainder and
parse the desired field (the same field you previously accessed with nth(19))
from the resultant iterator (preserve the existing return behavior using
parse::<isize>().unwrap_or(0)); replace the direct
line.split_whitespace().nth(19) usage with this "find last ')', take substring
after it, then split" approach to reliably locate the field even when comm
contains spaces.

#[cfg(not(any(target_os = "macos", target_os = "linux")))]
{
Expand Down
7 changes: 6 additions & 1 deletion crates/vm/src/vm/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,12 @@ impl StopTheWorldState {
continue;
}
slot.stop_requested.store(false, Ordering::Release);
if slot.state.load(Ordering::Relaxed) == THREAD_SUSPENDED {
let state = slot.state.load(Ordering::Relaxed);
debug_assert!(
state == THREAD_SUSPENDED,
"non-requester thread not suspended at start-the-world: id={id} state={state}"
);
if state == THREAD_SUSPENDED {
slot.state.store(THREAD_DETACHED, Ordering::Release);
slot.thread.unpark();
}
Expand Down