mirror of https://github.com/astral-sh/uv
fix: adjust close_handles pointer offsets to match distlib cleanup_fds (#6955)
## Summary Resolves issues mentioned in comments * https://github.com/astral-sh/uv/issues/6699#issuecomment-2322515962 * https://github.com/astral-sh/uv/issues/6866#issuecomment-2322785906 Further investigation on the comments revealed that the pointer arithmethic being performed in `let handle_start = unsafe { crt_magic.offset(1 + handle_count) };` from [posy trampoline](dda22e6f90/src/trampolines/windows-trampolines/posy-trampoline/src/bounce.rs (L146)) had some slight errors. Since `crt_magic` was a `*const u32`, doing an offset by `1 + handle_count` would offset by too much, with some possible out of bounds reads or attempts to call CloseHandle on garbage. We needed to offset differently since we want to offset by `handle_count` bytes after the initial offset as seen in [launcher.c](888c48b568/PC/launcher.c (L578)). Similarly, we needed to skip the first 3 handles, otherwise we'd still be attempting to close standard I/O handles of the parent (in this case the shell from `busybox.exe sh -l`). I also added a few extra checks available from `launcher.c` which checks if the handle value is `-2` just to match the distlib implementation more closely and minimize differences. ## Test Plan Manually compiled distlib's launcher with additional logging and replaced `Lib/site-packages/pip/_vendor/distlib/t64.exe` with the compiled one to log pointers. As a result, I was able to verify the retrieved handle memory addresses in this function actually match in both uv and distlib's implementation from within busybox.exe nested shell where this behavior can be observed and manually tested. I was also able to confirm this fixes the issues mentioned in the comments, at least with busybox's shell, but I assume this would fix the case with cmake. ## Open areas `launcher.c` also [checks the size](888c48b568/PC/launcher.c (L573-L576)) of `cbReserved2` before retrieving `handle_start` which this function currently doesn't do. If we wanted to, we could add the additional check here as well, but I wasn't fully sure why it wasn't added in the first place. Thoughts? ```rust // Verify the buffer is large enough if si.cbReserved2 < (size_of::<u32>() as isize + handle_count + size_of::<HANDLE>() as isize * handle_count) as u16 { return; } ``` --------- Co-authored-by: konstin <konstin@mailbox.org>
This commit is contained in:
parent
c9787f9fd8
commit
66699def2e
|
|
@ -344,17 +344,23 @@ fn close_handles(si: &STARTUPINFOA) {
|
|||
if si.cbReserved2 == 0 || si.lpReserved2.is_null() {
|
||||
return;
|
||||
}
|
||||
|
||||
let crt_magic = si.lpReserved2 as *const u32;
|
||||
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 = HANDLE(unsafe { handle_start.offset(i).read_unaligned() as _ });
|
||||
// Close all fds inherited from the parent, except for the standard I/O fds.
|
||||
if !handle.is_invalid() {
|
||||
unsafe { CloseHandle(handle) }.unwrap_or_else(|_| {
|
||||
eprintln!("Failed to close child file descriptors at {}", i);
|
||||
});
|
||||
let handle_start =
|
||||
unsafe { (crt_magic.offset(1) as *const u8).offset(handle_count) as *const HANDLE };
|
||||
|
||||
// Close all fds inherited from the parent, except for the standard I/O fds (skip first 3).
|
||||
for i in 3..handle_count {
|
||||
let handle = unsafe { handle_start.offset(i).read_unaligned() };
|
||||
// Ignore invalid handles, as that means this fd was not inherited.
|
||||
// -2 is a special value (https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/get-osfhandle)
|
||||
if handle.is_invalid() || handle.0 == -2 as _ {
|
||||
continue;
|
||||
}
|
||||
unsafe { CloseHandle(handle) }.unwrap_or_else(|_| {
|
||||
eprintln!("Failed to close child file descriptors at {}", i);
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Loading…
Reference in New Issue