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

Crash on windows when calling out to juliaup #51461

Closed
GunnarFarneback opened this issue Sep 26, 2023 · 3 comments · Fixed by libuv/libuv#4152
Closed

Crash on windows when calling out to juliaup #51461

GunnarFarneback opened this issue Sep 26, 2023 · 3 comments · Fixed by libuv/libuv#4152
Labels
domain:cmd Relates to calling of external programs system:windows Affects only Windows

Comments

@GunnarFarneback
Copy link
Contributor

GunnarFarneback commented Sep 26, 2023

I'm not sure whether to blame this crash on julia or juliaup or a combination, but let's start here.
This seems likely to be a Windows bug with programs installed from Windows store but it would be good if Julia can work around it.

Reproducer

This assumes juliaup, from Windows store, and git (not from Windows store) are installed. As discussed later neither juliaup nor git are critical.

  1. Start a Cmd window.
  2. In the Cmd, start julia.
  3. In julia, run
run(`juliaup --version`)
run(`git --version`)

Crash

julia> run(`juliaup --version`)
Juliaup 1.8.16
Process(`juliaup --version`, ProcessExited(0))

julia> run(`git --version`)
AssignProcessToJobObject: (87) The parameter is incorrect.

Please submit a bug report with steps to reproduce this fault, and any error messages that follow (in their entirety). Thanks.
Exception: EXCEPTION_BREAKPOINT at 0x7ff8adf3b502 -- DebugBreak at C:\WINDOWS\System32\KERNELBASE.dll (unknown line)
in expression starting at REPL[2]:1
DebugBreak at C:\WINDOWS\System32\KERNELBASE.dll (unknown line)
Allocations: 2998 (Pool: 2986; Big: 12); GC: 0

C:\Users\GunnarF>git version 2.14.3.windows.1

C:\Users\GunnarF>

Analysis

  1. Replacing the call to git with e.g. cmd /c dir or ping also crashes.
  2. Replacing juliaup with another program installed from Windows store, e.g. winget, also crashes.
  3. If juliaup is installed with a non-store installer, it doesn't crash.
  4. Different versions of Julia all crash. Tested with Julia 1.6 and 1.9 installed from juliaup and 1.9 from a binary download.
  5. Replacing Cmd with Powershell also crashes.
  6. Replacing Cmd with Git Bash or starting Julia directly from Windows do not crash.
  7. Running this via a bat file does crash (this is the case that really matters to me).
  8. Calling juliaup multiple times, followed by git, crashes, but not until calling git.
  9. Effective workaround: This sequence does not crash:
run(`git --version`)
run(`juliaup --version`)
run(`git --version`)
  1. Calling out from a Python started in a Cmd does not crash:
import subprocess
subprocess.run(["juliaup", "--version"])
subprocess.run(["git", "--version"])
@mgkuhn mgkuhn added system:windows Affects only Windows domain:cmd Relates to calling of external programs labels Sep 26, 2023
@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 27, 2023

Seems to be a problem other people have reported, but not gotten any response on such as microsoft/pylance-release#1118. The first object here is a Windows Store app, and it seems like that corrupts the Job object in the kernel in some way so that it fails on future uses.

@GunnarFarneback
Copy link
Contributor Author

Good point. I've tried with a non-store install of juliaup and then it doesn't crash while it does crash if I replace juliaup with another store app (winget --version). I'll update the description to include this.

Anyhow, even if this is a Windows bug, it seems likely that people will run into it again from time to time, so it would be good to work around the bug. Somehow Python avoids the crash but maybe subprocess.run isn't quite as equivalent as it naively looks.

vtjnash added a commit to vtjnash/libuv that referenced this issue Sep 27, 2023
Make sure this handle is functional. The Windows kernel seems to have a bug that if the first use of AssignProcessToJobObject is for a Windows Store program, subsequent attempts to use the handle with fail with INVALID_PARAMETER (87). This is possilby because all uses of the handle must be for the same Terminal Services session. We can ensure it is tied to our current session now by adding ourself to it. We could remove ourself afterwards, but there doesn't seem to be a reason to.

Secondly, we start the process suspended so that we can make sure we added it
to the job control object before it does anything itself (such as launch more
jobs or exit).

Fixes: JuliaLang/julia#51461
@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 27, 2023

Most programs don't set up job control to match the unix behavior. The reason might be this restriction in the documentation:

Terminal Services: All processes within a job must run within the same session as the job.

But anyways, despite probably being a kernel bug of some sort, we can work around it: libuv/libuv#4152

vtjnash added a commit to libuv/libuv that referenced this issue Oct 2, 2023
Make sure this handle is functional. The Windows kernel seems to have a
bug that if the first use of AssignProcessToJobObject is for a Windows
Store program, subsequent attempts to use the handle with fail with
INVALID_PARAMETER (87). This is possilby because all uses of the handle
must be for the same Terminal Services session. We can ensure it is
tied to our current session now by adding ourself to it. We could
remove ourself afterwards, but there doesn't seem to be a reason to.

Secondly, we start the process suspended so that we can make sure we
added it to the job control object before it does anything itself (such
as launch more jobs or exit).

Fixes: JuliaLang/julia#51461
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:cmd Relates to calling of external programs system:windows Affects only Windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants