Fix supervisor race condition on Windows trampoline bounce#18170
Fix supervisor race condition on Windows trampoline bounce#18170
Conversation
Create trampoline children with CREATE_SUSPENDED, assign them to the supervisor job object, then resume the primary thread. This removes the post-spawn assignment race for short-lived child processes. Regenerate prebuilt Windows trampoline binaries for i686, x86_64, and aarch64.
zanieb
changed the title
zanieb
changed the title
EliteTK
left a comment
EliteTK
left a comment
There was a problem hiding this comment.
I am not a fan of this code, but that's not this PR's fault.
We actually don't care if the child dies before we add it to the job object, and an easier fix here would just be to ignore that case.
But! This does fix another race, which is the one where we spawn the process and we get killed before we add the child to the job object. And that's definitely a win.
|
Actually, never mind, I can't find anything that explains what happens if we die while we have a suspended child... So this might mean we leave a suspended child lying around forever. So maybe the best solution for now is to ignore that error... At least then we can't leak a permanently suspended process. |
|
Found https://devblogs.microsoft.com/oldnewthing/20230209-00/?p=107812 . Apparently we want Some enterprises may still be running earlier versions either in locked down environments or using extended support patchsets. |
|
I wonder if we can add ourselves to the job object and then spawn the child. Normally this isn't what you want since you'd be restricting yourself somehow in some way which you don't intend, but in this case it seems like it wouldn't have any unintended impact. If we are already in a job and it's pre windows 8 then assigning ourselves to another job would fail, but we already have that problem with the existing code since the child will inherit any job we're in. |
zanieb
temporarily deployed
to
uv-test-publish
GitHub Actions
Inactive
crates/uv-trampoline/src/bounce.rs
Outdated
| /// before `AssignProcessToJobObject` is called. However, it requires Windows 10+. | ||
| /// | ||
| /// Returns `None` if the attribute list APIs fail (e.g., on older Windows versions), | ||
| /// signaling the caller to use the fallback path. |
There was a problem hiding this comment.
A nitpick: I would drop this bit after the comma. It's somewhat unbefitting a function doc comment to attempt to document the call site.
| // The first call is expected to fail with ERROR_INSUFFICIENT_BUFFER, returning | ||
| // the required size. If it fails for another reason, fall back. | ||
| if init_result.is_ok() || attr_list_size == 0 { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
If there is an error and it's not ERROR_INSUFFICIENT_BUFFER then I don't think we can assume that attr_list_size hasn't been modified to some nonsensical value. We should actually explicitly check that the error was the expected one rather than relying on attr_list_size being non-zero.
crates/uv-trampoline/src/bounce.rs
Outdated
| // Allocate the attribute list buffer. | ||
| let attr_list_buf = vec![0u8; attr_list_size]; | ||
| let attr_list = LPPROC_THREAD_ATTRIBUTE_LIST(attr_list_buf.as_ptr() as *mut _); |
There was a problem hiding this comment.
I could be wrong but if we're allocating for u8 then rust only guarantees that the resulting buffer is aligned for u8 at best? Is it safe to cast this to a type with potentially stricter alignment requirements here?
zanieb
force-pushed
the
zb/fix-bounce-race-ii
branch
3 times, most recently
from
4becdef to
ad44925
Compare
zanieb
force-pushed
the
zb/fix-bounce-race-ii
branch
from
ad44925 to
b2ac908
Compare
crates/uv-trampoline/src/bounce.rs
Outdated
| .unwrap_or_else(|_| { | ||
| error_and_exit("uv trampoline failed to create layout for attribute list"); | ||
| }); | ||
| // SAFETY: layout has non-zero size (attr_list_size comes from InitializeProcThreadAttributeList). |
There was a problem hiding this comment.
| // SAFETY: layout has non-zero size (attr_list_size comes from InitializeProcThreadAttributeList). | |
| // SAFETY: layout has non-zero size (attr_list_size comes from InitializeProcThreadAttributeList). |
crates/uv-trampoline/src/bounce.rs
Outdated
| /// before `AssignProcessToJobObject` is called. However, it requires Windows 10+. | ||
| /// | ||
| /// Returns `None` if the attribute list APIs fail (e.g., on older Windows versions). | ||
| fn spawn_child_in_job(si: &STARTUPINFOA, child_cmdline: CString, job: &Job) -> Option<HANDLE> { |
There was a problem hiding this comment.
Can we avoid variables like si? Use a clear name
There was a problem hiding this comment.
Agreed. Name originally mimicked distlib's launcher.c
crates/uv-trampoline/src/bounce.rs
Outdated
| result | ||
| } | ||
|
|
||
| /// Inner implementation of [`spawn_child_in_job`] that may fail at any point. |
There was a problem hiding this comment.
What does it mean that this may fail at any point?
Why is this a separate function at all?
crates/uv-trampoline/src/bounce.rs
Outdated
| // SAFETY: We successfully initialized the attribute list above. | ||
| unsafe { DeleteProcThreadAttributeList(attr_list) }; | ||
|
|
||
| // CreateProcessA failed — fall back to the non-attribute-list path. |
There was a problem hiding this comment.
This comment is confusing
crates/uv-trampoline/src/bounce.rs
Outdated
| /// This approach avoids race conditions where the child exits or the parent is killed | ||
| /// before `AssignProcessToJobObject` is called. However, it requires Windows 10+. | ||
| /// | ||
| /// Returns `None` if the attribute list APIs fail (e.g., on older Windows versions). |
There was a problem hiding this comment.
What is the first point at which this would fail? Won't that tell us if the attribute list API is supported and everything after that should be an error instead of None?
crates/uv-trampoline/src/bounce.rs
Outdated
| // Try the race-free path first: spawn the child directly into the job using | ||
| // PROC_THREAD_ATTRIBUTE_JOB_LIST (Windows 10+). If that's not available, | ||
| // fall back to spawning normally and assigning afterward. | ||
| let child_handle = spawn_child_in_job(&si, child_cmdline.clone(), &job) | ||
| .unwrap_or_else(|| spawn_child_and_assign_to_job(&si, child_cmdline, &job)); |
There was a problem hiding this comment.
| // Try the race-free path first: spawn the child directly into the job using | |
| // PROC_THREAD_ATTRIBUTE_JOB_LIST (Windows 10+). If that's not available, | |
| // fall back to spawning normally and assigning afterward. | |
| let child_handle = spawn_child_in_job(&si, child_cmdline.clone(), &job) | |
| .unwrap_or_else(|| spawn_child_and_assign_to_job(&si, child_cmdline, &job)); | |
| let child_handle = spawn_child_in_job(&si, child_cmdline.clone(), &job) | |
| // If the Windows 10 API is not available, fallback to the mode that can race | |
| .unwrap_or_else(|| spawn_child_and_assign_to_job(&si, child_cmdline, &job)); |
One consequence of this approach is that it moves us much further away from matching distlib's approach in launcher.c or cpython's launcher2.c. The blog post highlights a scenario which I think doesn't exactly apply since in distlib AssignProcessToJobObject is a best-effort attempt, which is why I believe distlib's approach doesn't use suspend either. The attribute list approach can also fail depending on existing nested jobs and parent state (which is a possibility with the flake). Was the goal of this PR to try to reduce the risk of failure or enforcement? I'd be curious has this flake occured before or is it after the recent trampoline changes. If the goal was to reduce risk of failure, I think a smaller simpler change as seen below be a better first step instead, e.g. - if let Err(e) = unsafe { job.assign_process(child_handle) } {
- print_job_error_and_exit(
- "uv trampoline failed to assign child process to job object",
- e,
- );
- }
+ if let Err(e) = unsafe { job.assign_process(child_handle) } {
+ warn!(
+ "uv trampoline failed to assign child process to job object\n Caused by: {} (os error {})"
+ e.message(),
+ e.code(),
+ );
+ }This is permissive, matches distlib more closely, and job assignment will still in most cases work. |
A simpler short-term fix for #18170 It's unclear if the complexity of that pull request is worth it.
|
We decided on #18291 instead for now |
3 participants
Create trampoline children withCREATE_SUSPENDED, assign them to the supervisor job object, then resume the primary thread. This avoids a post-spawn assignment race for short-lived child processes.Hopefully, this addresses a bug observed as a CI flake where
python_upgrade_transparent_from_venv_previewfailed with: