Skip to content

Commit

Permalink
fix: adjust close_handles pointer offsets to match distlib cleanup_fds (
Browse files Browse the repository at this point in the history
#6955)

## Summary

Resolves issues mentioned in comments
* #6699 (comment)
* #6866 (comment)

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](https://github.com/njsmith/posy/blob/dda22e6f90f5fefa339b869dd2bbe107f5b48448/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](https://github.com/pypa/distlib/blob/888c48b56886b03398646be1217508830427bd75/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](https://github.com/pypa/distlib/blob/888c48b56886b03398646be1217508830427bd75/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 <[email protected]>
  • Loading branch information
samypr100 and konstin authored Sep 4, 2024
1 parent c9787f9 commit 66699de
Show file tree
Hide file tree
Showing 7 changed files with 14 additions and 8 deletions.
22 changes: 14 additions & 8 deletions crates/uv-trampoline/src/bounce.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
}
}

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 66699de

Please sign in to comment.