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

CMake stub crashes when executed by VS Code #6866

Closed
petermbauer opened this issue Aug 30, 2024 · 3 comments
Closed

CMake stub crashes when executed by VS Code #6866

petermbauer opened this issue Aug 30, 2024 · 3 comments

Comments

@petermbauer
Copy link

When installing CMake via uv pip install instead of pip install, the stub created by uv crashes with a segfault when executed by the CMake Tools extension.

image

How to reproduce:

  • make sure to have VS Code with the CMake Tools extension installed and some CMakeLists.txt file in the current directory which is to be picked up by VS Code automatically.
  • run the following commands:
λ bin\uv.exe --version
uv 0.4.0 (d9bd3bc7a 2024-08-28)

λ bin\uv.exe venv myvenv
Using Python 3.10.9 interpreter at: ...
Creating virtualenv at: myvenv
Activate with: myvenv\Scripts\activate

λ bin\uv.exe pip install --python myvenv cmake==3.27.0
Resolved 1 package in 208ms
Installed 1 package in 1.97s
 + cmake==3.27.0

λ myvenv\Scripts\activate.bat

(myvenv) λ code .
  • just select "Unspecified kit" to let CMake select one or any other installed kit that works for you
  • check the CMake/Build output

When executed from the command line, cmake works as expected, same also from any terminal inside VS Code so i guess this is caused by VS Code executing the cmake.exe directly without a shell in between.

Most likely related to #6464.

Windows 10, Python 3.10

@petermbauer
Copy link
Author

petermbauer commented Aug 31, 2024

No idea why it did not work when testing with the binary from #6792 but i can confirm that it works now with uv 0.4.1 so thanks a lot for the fix!
However, the same error output as mentioned in #6699 (comment) also appears in VS Code.

[build] Failed to close child file descriptors at 1
[build] Failed to close child file descriptors at 2

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

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

@petermbauer
Copy link
Author

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

yes, i can confirm. Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants