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 when trying to load an Assembly in a hostfxr environment after calling FreeConsole() #91856

Closed
WerWolv opened this issue Sep 10, 2023 · 16 comments · Fixed by #92123
Closed
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@WerWolv
Copy link

WerWolv commented Sep 10, 2023

Description

Hi
I'm currently experiencing a crash in one of my applications which uses the hostfxr environment to load .NET Assemblies. The application is generally a GUI application, however we use the CONSOLE subsystem to have support for a command line interface. When run from the regular explorer, the application detects this and calls FreeConsole() to hide the console window.

This works fine for the most part, however we noticed that FreeConsole() apparently leaves the stdout, stderr and stdin handles in an invalid state which later on causes a crash here when the JIT tries to duplicate the stdout handle. The code there seems to do some checks already but doesn't seem to catch this problem.

Reproduction Steps

The latest nightly release of ImHex triggers this issue at launch on Windows 10 sometimes. It's not consistent but it happens often.
It can be downloaded from https://imhex.download/#nightly

Alternatively, a simple application which uses the CONSOLE subsystem, calls FreeConsole() and then loads an Assembly through the hostfxr functions should reproduce the issue on Windows 10.

Expected behavior

stdout should simply not be used in this case instead of causing a crash

Actual behavior

The call to _dup() at

int jitstdoutFd = _dup(_fileno(procstdout()));
causes a abort to happen.

Regression?

No response

Known Workarounds

Interestingly enough, this issue seems to only affect Windows 10 (and potentially even older versions of Windows). It does not happen on Windows 11.

Configuration

  • .NET 7.0.400
  • Windows 10
  • x64
  • It's specific to Windows 10 (and potentially before), not Windows 11

Other information

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 10, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 10, 2023
@jkotas jkotas added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 11, 2023
@ghost
Copy link

ghost commented Sep 11, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

Hi
I'm currently experiencing a crash in one of my applications which uses the hostfxr environment to load .NET Assemblies. The application is generally a GUI application, however we use the CONSOLE subsystem to have support for a command line interface. When run from the regular explorer, the application detects this and calls FreeConsole() to hide the console window.

This works fine for the most part, however we noticed that FreeConsole() apparently leaves the stdout, stderr and stdin handles in an invalid state which later on causes a crash here when the JIT tries to duplicate the stdout handle. The code there seems to do some checks already but doesn't seem to catch this problem.

Reproduction Steps

The latest nightly release of ImHex triggers this issue at launch on Windows 10 sometimes. It's not consistent but it happens often.
It can be downloaded from https://imhex.download/#nightly

Alternatively, a simple application which uses the CONSOLE subsystem, calls FreeConsole() and then loads an Assembly through the hostfxr functions should reproduce the issue on Windows 10.

Expected behavior

stdout should simply not be used in this case instead of causing a crash

Actual behavior

The call to _dup() at

int jitstdoutFd = _dup(_fileno(procstdout()));
causes a abort to happen.

Regression?

No response

Known Workarounds

Interestingly enough, this issue seems to only affect Windows 10 (and potentially even older versions of Windows). It does not happen on Windows 11.

Configuration

  • .NET 7.0.400
  • Windows 10
  • x64
  • It's specific to Windows 10 (and potentially before), not Windows 11

Other information

Author: WerWolv
Assignees: -
Labels:

area-CodeGen-coreclr, untriaged, needs-area-label

Milestone: -

@jkotas jkotas removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 11, 2023
@jkotas
Copy link
Member

jkotas commented Sep 11, 2023

Does the release build of the JIT need to do this I/O initialization unconditionally during startup? It is unnecessary work unless there needs to be something printed to the console.

@jakobbotsch
Copy link
Member

Can you see what the value of _fileno(procstdout()) is when the crash happens? It looks like there is already some error detection of the value above, so I suppose the value is not one of the detected cases (or there is some race condition, likely the code should reuse stdoutFd).

Does the release build of the JIT need to do this I/O initialization unconditionally during startup? It is unnecessary work unless there needs to be something printed to the console.

It would presumably be possible to refactor it to be initialized lazily, though there are a number of places we make use of jitstdout. I don't know how expensive this initialization is compared to the rest of the initialization.

@BruceForstall BruceForstall added the needs-author-action An issue or pull request that requires more info or actions from the author. label Sep 13, 2023
@ghost
Copy link

ghost commented Sep 13, 2023

This issue has been marked needs-author-action and may be missing some important information.

@BruceForstall BruceForstall removed the untriaged New issue has not been triaged by the area owner label Sep 13, 2023
@BruceForstall BruceForstall added this to the 9.0.0 milestone Sep 13, 2023
@BruceForstall
Copy link
Member

I marked it .NET 9 for now, but we can adjust that when we get a repro and/or fix.

@PerikiyoXD
Copy link

PerikiyoXD commented Sep 13, 2023

Can you see what the value of _fileno(procstdout()) is when the crash happens?

Invalid bytes. It can be any value. Actually, I can't debug it as the variables are optimized away, says VS.
Although, stdoutFd equals 1, thus entering the if ((stdoutFd != -1) && (stdoutFd != -2) && (errno != EINVAL)) condition and trying to duplicate the file descriptor (Which is INVALID).

I guessed that there's a race condition too. Must be noted that we launch plugins on paralell to the execution.

Quick summary of the situation

In this case we're under a IMAGE_SUBSYSTEM_WINDOWS_CUI executable process.

This crash is probably caused by FreeConsole.

FreeConsole call does close the standard streams.

We're getting a crash trying to load hostfxr.

hostfxr tries to duplicate a file descriptor, through a closed handle.

There are times (just like this) where a CUI/CLI application wants to detach from the console by using FreeConsole and that invalidates the streams that were used on hostfxr initialization.

Note

This is kind of a python and pythonw dilemma in the end, but definitely there should be a safe-check of the std handles when accessing and duplicating handles.

@PerikiyoXD
Copy link

PerikiyoXD commented Sep 13, 2023

It would definitely need a safe check on procstdout()

@jakobbotsch
Copy link
Member

So are you saying procstdout() is returning nullptr or some other failure value?

This code runs just once at start up, so a race condition seems unlikely if you can readily reproduce it.

@jkotas
Copy link
Member

jkotas commented Sep 13, 2023

So are you saying procstdout() is returning nullptr or some other failure value?

procstdout returns a FILE* that points to closed OS handle. Attempt to duplicate the closed OS handle fails.

@jakobbotsch
Copy link
Member

procstdout returns a FILE* that points to closed OS handle. Attempt to duplicate the closed OS handle fails.

Does it mean there is no safe way to use stdout from the VM and JIT? From MSDN on _fileno it does not sound like FreeConsole should close stdout:

If stdout or stderr is not associated with an output stream (for example, in a Windows application without a console window), the file descriptor returned is -2. In previous versions, the file descriptor returned was -1. This change allows applications to distinguish this condition from an error.

There's also this SO post that seems related, but doesn't give much insight: https://stackoverflow.com/questions/60833724/allocconsole-and-stdin-stdout

@jkotas
Copy link
Member

jkotas commented Sep 13, 2023

Does it mean there is no safe way to use stdout from the VM and JIT?

If some other component in the process closes the underlying OS handle for standard output, using stdout is not going to work. That is by design. Nothing to fix there.

This issue highlighted that the JIT initialization duplicates OS file handles that are not going to be used in 99.99% cases. I think it is something we should fix. OS handles and OS syscalls are not free.

@jakobbotsch
Copy link
Member

If some other component in the process closes the underlying OS handle for standard output, using stdout is not going to work. That is by design. Nothing to fix there.

This issue highlighted that the JIT initialization duplicates OS file handles that are not going to be used in 99.99% cases. I think it is something we should fix. OS handles and OS syscalls are not free.

I agree that we should change it to be lazily initialized. In practice it'll fix this issue too. Still, I am curious about the underlying issue here, and how we should be detecting whether or not stdout is a valid handle that can be used.
Even if we make initialization lazy the JIT is still going to crash the process if DOTNET_JitDisasm is turned on for one of these processes.

@WerWolv
Copy link
Author

WerWolv commented Sep 14, 2023

As far as I understand it, the issue is caused by FreeConsole() only closing the underlying kernel handles and not the file descriptors themselves, therefor leaving them in an undefined state. Normally this shouldn't be possible.

I found this post on StackOverflow that mostly describes the issue and how to work around it, however some of the code seems to be missing and some functions aren't available to us since we're using MinGW: https://stackoverflow.com/questions/12676312/freeconsole-behaviour-on-windows-8

@ghost ghost added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Sep 14, 2023
@jkotas
Copy link
Member

jkotas commented Sep 14, 2023

the issue is caused by FreeConsole() only closing the underlying kernel handles

I believe that this issue was fixed in more recent Windows versions.

how we should be detecting whether or not stdout is a valid handle

There is no way to reliably detect this. If the original handle was closed, the handle value could have been later reused for a different object.

@WerWolv
Copy link
Author

WerWolv commented Sep 15, 2023

I believe that this issue was fixed in more recent Windows versions.

This is absolutely possible yeah. I was never able to reporduce the issue on Windows 11 but it's pretty consistent when using a Windows 10 VM.

There is no way to reliably detect this. If the original handle was closed, the handle value could have been later reused for a different object.

I don't know how viable this is but maybe it would make sense to be able to pass in a stream that is used then when calling into the hostfxr interface. And just default to stdout if none is passed in.

@jakobbotsch
Copy link
Member

jakobbotsch commented Sep 15, 2023

I don't know how viable this is but maybe it would make sense to be able to pass in a stream that is used then when calling into the hostfxr interface. And just default to stdout if none is passed in.

The optimization @jkotas suggested above will in practice fix the issue: we can avoid any I/O operations until the JIT actually needs the handle. The JIT does not need to do anything with stdout in normal scenarios -- only when certain environment variables are set.

jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Sep 15, 2023
Avoid duplicating a handle and doing several I/O operations on the
startup path. Fixes dotnet#91856 as a side effect.
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 15, 2023
@jakobbotsch jakobbotsch self-assigned this Sep 15, 2023
@jakobbotsch jakobbotsch removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Sep 15, 2023
jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Sep 18, 2023
Avoid duplicating a handle and doing several I/O operations on the
startup path. Fixes dotnet#91856 as a side effect.
carlossanlop pushed a commit that referenced this issue Sep 18, 2023
* JIT: Initialize jitstdout lazily

Avoid duplicating a handle and doing several I/O operations on the
startup path. Fixes #91856 as a side effect.

* Fix jitShutdown

* Add volatile

* CSE

* Ensure jitstdout() is not used eagerly

* Move comment, fix printf -> jitprintf

* Clean up
jakobbotsch added a commit that referenced this issue Sep 19, 2023
Avoid duplicating a handle and doing several I/O operations on the
startup path. Fixes #91856 as a side effect.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 19, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants