Skip to content

Commit

Permalink
fix: adjust trampoline close_handles invalid to be safer (astral-sh#6792
Browse files Browse the repository at this point in the history
)

## Summary

Closes astral-sh#6699

On cases like the ones described in
astral-sh#6699, `lpReserved2` somehow seems
to report multiple file descriptors that were not tied to any valid
handles. The previous implementation was faulting as it would try to
dereference these invalid handles. This change moves to using `HANDLE`
directly and check if its is_invalid instead before attempting to close
them.

## Test Plan

Manually tested and verified using `busybox-w32` like described in the
issue.

---------

Co-authored-by: konstin <konstin@mailbox.org>
  • Loading branch information
samypr100 and konstin authored Aug 30, 2024
1 parent c91c99b commit 8674968
Show file tree
Hide file tree
Showing 7 changed files with 9 additions and 7 deletions.
16 changes: 9 additions & 7 deletions crates/uv-trampoline/src/bounce.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ use windows::Win32::{
TRUE,
},
System::Console::{
GetStdHandle, SetConsoleCtrlHandler, SetStdHandle, STD_ERROR_HANDLE, STD_INPUT_HANDLE,
STD_OUTPUT_HANDLE,
GetStdHandle, SetConsoleCtrlHandler, SetStdHandle, STD_INPUT_HANDLE, STD_OUTPUT_HANDLE,
},
System::Environment::GetCommandLineA,
System::JobObjects::{
Expand Down Expand Up @@ -329,7 +328,8 @@ fn spawn_child(si: &STARTUPINFOA, child_cmdline: CString) -> HANDLE {
// https://github.com/huangqinjin/ucrt/blob/10.0.19041.0/lowio/ioinit.cpp#L190-L223
fn close_handles(si: &STARTUPINFOA) {
// See distlib/PC/launcher.c::cleanup_standard_io()
for std_handle in [STD_INPUT_HANDLE, STD_OUTPUT_HANDLE, STD_ERROR_HANDLE] {
// Unlike cleanup_standard_io(), we don't close STD_ERROR_HANDLE to retain eprintln!
for std_handle in [STD_INPUT_HANDLE, STD_OUTPUT_HANDLE] {
if let Ok(handle) = unsafe { GetStdHandle(std_handle) } {
unsafe { CloseHandle(handle) }.unwrap_or_else(|_| {
eprintln!("Failed to close standard device handle {}", handle.0 as u32);
Expand All @@ -348,11 +348,13 @@ fn close_handles(si: &STARTUPINFOA) {
let handle_count = unsafe { crt_magic.read_unaligned() } as isize;
let handle_start = unsafe { crt_magic.offset(1 + handle_count) };
for i in 0..handle_count {
let handle_ptr = unsafe { handle_start.offset(i).read_unaligned() } as *const HANDLE;
let handle = HANDLE(unsafe { handle_start.offset(i).read_unaligned() as _ });
// Close all fds inherited from the parent, except for the standard I/O fds.
unsafe { CloseHandle(*handle_ptr) }.unwrap_or_else(|_| {
eprintln!("Failed to close child file descriptors at {}", i);
});
if !handle.is_invalid() {
unsafe { CloseHandle(handle) }.unwrap_or_else(|_| {
eprintln!("Failed to close child file descriptors at {}", i);
});
}
}
}

Expand Down
Binary file not shown.
Binary file modified crates/uv-trampoline/trampolines/uv-trampoline-aarch64-gui.exe
Binary file not shown.
Binary file modified crates/uv-trampoline/trampolines/uv-trampoline-i686-console.exe
Binary file not shown.
Binary file modified crates/uv-trampoline/trampolines/uv-trampoline-i686-gui.exe
Binary file not shown.
Binary file not shown.
Binary file modified crates/uv-trampoline/trampolines/uv-trampoline-x86_64-gui.exe
Binary file not shown.

0 comments on commit 8674968

Please sign in to comment.