Skip to content
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.

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

Stubs on Windows exit abnormally, includes succinct repro with busybox, related to some wider issue #6699

Closed
doctorpangloss opened this issue Aug 27, 2024 · 12 comments · Fixed by #6792
Labels
windows Specific to the Windows platform

Comments

@doctorpangloss
Copy link

related: rmyorston/busybox-w32#451
related: #6464

WindowsTerminal_qBCDluBxeQ

wget https://frippery.org/files/busybox/prerelease/busybox_pre64.exe
busybox_pre64.exe sh -X
mkdir uv-test
cd uv-test
uv venv --seed
source .venv/scripts/activate
# uses stub
pip install insightface

Observe pip exits abnormally under this shell. In Powershell it works fine.

uv 0.3.4 (39f3cd2 2024-08-26)
Windows 2022

All stubs, not just pip, are cursed this way. The stubs created by native pip do not have any issues.

@doctorpangloss doctorpangloss changed the title Stubs on Windows exit abnormally when run under busybox, indicating wider issue Stubs on Windows exit abnormally, includes succinct repro with busybox, related to some wider issue Aug 27, 2024
@rmyorston
Copy link

rmyorston commented Aug 28, 2024

I can see the problem, but don't have much idea why it occurs.

What I've done:

  • Install Python on Windows using an installer from python.org.
  • From PowerShell run pip install uv.
  • From a busybox-w32 shell do the whole mkdir uv-test; ... ; pip install insightface thing. As promised, it doesn't work.

The question is why not. I was suspicious of the changes the busybox-w32 shell makes to environment variables when it reads them in as shell variables. By default it converts the variable names to uppercase and replaces backslashes with forward slashes in the value. (There are a couple of exceptions to the latter, it seemed possible more might be needed.)

I made a custom shell so those changes could be avoided. It remains necessary to change Path to PATH, or the shell won't find any external commands. pip install insightface still fails with a segfault. I do note, though, that if the failing process is stopped with an error dialog the installation seems to carry on in the background until the dialog is closed.

@zanieb
Copy link
Member

zanieb commented Aug 28, 2024

Thanks for the report. I presume this is a bug in our "trampoline" that's used for executables on Windows. I don't see an error message in the video, what's the failure mode exactly?

@zanieb zanieb added the windows Specific to the Windows platform label Aug 28, 2024
@samypr100
Copy link
Collaborator

By default it converts the variable names to uppercase and replaces backslashes with forward slashes in the value.

Does this behave as if it was a MinGW based environment? There's been a number of MinGW related issues, especially when it comes to paths. e.g. #3573

@rmyorston
Copy link

@zanieb In my experience the failure mode is a 0xc0000005 error, which I (possibly wrongly) equate to a segfault.

@rmyorston
Copy link

@samypr100 The issue you reference seems to be somewhat confused about what MinGW means. (And I'm not sure I can be lucid enough to clear up the confusion.)

MinGW (and these days that generally means MinGW-w64) is a set of headers and libraries which make it possible to build native Windows applications.

There are various ways that can be done:

  • I cross-compile on Linux.
  • There's the Windows-native w64devkit toolchain.
  • MSYS2 is an alternative toolchain for Windows.

busybox-w32 is built using MinGW, so it's a native Windows binary which deals in native Windows paths.

This can be opposed to binaries built using the Cygwin DLL, such as those that come with MSYS2: these see fake POSIX paths.

When the referenced issue talks of 'mingw python' it seems to mean the Python that's shipped with MSYS2. This is built using the Cygwin DLL and understands non-Windows paths. I wouldn't call that a 'MinGW Python'. Furthermore, I wouldn't be at all surprised to hear there have been issues with it's expectations regarding paths.

@samypr100
Copy link
Collaborator

@rmyorston Thanks for the explanation. I've made an adjustment in #6792 that would fix this issue, or at least not fault. I haven't digged into the busybox impl, but lpReserved2 reports multiple file descriptors which are invalid handles which led to the issue manifesting. For now, I've switched the implementation to ignore any handle that seems to be invalid.

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) };

@rmyorston
Copy link

@samypr100 The fix sounds promising, thanks.

The shell in busybox-w32 uses spawnve() from the Microsoft C runtime to invoke external commands. What happens after that is up to Microsoft.

It'll be interesting to know if the fix also resolves #6464.

konstin added a commit that referenced this issue Aug 30, 2024
## Summary

Closes #6699

On cases like the ones described in
#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 <[email protected]>
@doctorpangloss
Copy link
Author

Is it expected to see

Failed to close child file descriptors at 1

when invoking the stub from within sh onto stderr? otherwise things appear to be behaving normally

konstin added a commit that referenced this issue Sep 4, 2024
#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]>
@samypr100
Copy link
Collaborator

@doctorpangloss Would you be able to confirm 0.4.5 fixes the fd issue for you?

@doctorpangloss
Copy link
Author

doctorpangloss commented Sep 9, 2024

$ pip install -U uv
Collecting uv
  Obtaining dependency information for uv from https://files.pythonhosted.org/packages/06/f2/0278155d4b2726765bcad89e5bc6020a1afb52d2bfe8cf1602758e44a93f/uv-0.4.8-py3-none-win_amd64.whl.metadata
  Downloading uv-0.4.8-py3-none-win_amd64.whl.metadata (11 kB)
Downloading uv-0.4.8-py3-none-win_amd64.whl (12.7 MB)
   ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 12.7/12.7 MB 17.2 MB/s eta 0:00:00
Installing collected packages: uv
Successfully installed uv-0.4.8
WARNING: There was an error checking the latest version of pip.
(.venv) ~/Documents/appmana $ cd appmana-comfyui/src/
(.venv) ~/Documents/appmana/appmana-comfyui/src $ uv pip install --no-deps -e .
Resolved 1 package in 18ms
Installed 1 package in 14ms
 + comfyui==0.0.1 (from file:///C:/Users/bberman/Documents/appmana/appmana-comfyui/src)
(.venv) ~/Documents/appmana/appmana-comfyui/src $ comfyui -w ../../appmana-artworkspace/src/workdir/
Failed to close child file descriptors at 1

no, unfortunately the stubs still show the issue, they do work though

@samypr100
Copy link
Collaborator

samypr100 commented Sep 10, 2024

Could you provide steps on how to reproduce this particular case?

@doctorpangloss
Copy link
Author

doctorpangloss commented Sep 10, 2024

nervermind it's resolved! 0.4.8 has fixed it

@github-staff github-staff deleted a comment from YeGop0218 Oct 28, 2024
@github-staff github-staff deleted a comment from YeGop0218 Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
windows Specific to the Windows platform
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants