8000 AMSI not uninitialized when pwsh is not crossgen'd and the window hosting pwsh (directly or indirectly) gets closed (on Windows) · Issue #6862 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

AMSI not uninitialized when pwsh is not crossgen'd and the window hosting pwsh (directly or indirectly) gets closed (on Windows) #6862

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.

Sign up for GitHub

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

Jump to bottom
Closed
bergmeister opened this issue May 13, 2018 · 11 comments
Closed

AMSI not uninitialized when pwsh is not crossgen'd and the window hosting pwsh (directly or indirectly) gets closed (on Windows) #6862

bergmeister opened this issue May 13, 2018 · 11 comments
Labels
Issue-Code Cleanup the issue is for cleaning up the code with no impact on functionality WG-Engine core PowerShell engine, interpreter, and runtime

Comments

@bergmeister
Copy link
Contributor
bergmeister commented May 13, 2018 •
edited
Loading

This is either a bug or a Debug.Assert call needs to be reviewed. The VerifyAmsiUninitializeCalled method that is called here has a Debug.Assert violation in the following scenario:

  • Debug build (because Debug.Assert is only enabled in Debug)
  • The -CrossGen option on Start-PSBuild is not used
  • The ConsoleHost gets closed by the window that is hosting pwsh (can be directly or indirectly, i.e. nested) via the Windows UI (Close button on the window itself or the taskbar) but not via typing exit

Steps to reproduce

git clone https://github.com/PowerShell/PowerShell.git PowerShellReproIssue6862
cd .\PowerShellReproIssue6862
Import-Module .\build.psm1
Start-PSBootStrap
Start-PSBuild
Invoke-Item "$pwd/src\powershell-win-core\bin\Debug\netcoreapp2.1\win7-x64\publish\pwsh.exe"
# Then close the window via the `X` button or the `Close` option on the taskbar

Expected behavior

The window closes normally.

Actual behavior

Debug.Assert failure here
image

Environment data

(Latest state of master)

> $PSVersionTable
Name                           Value
----                           -----
PSVersion                      6.0.0-beta.3
PSEdition                      Core
GitCommitId                    v6.0.0-beta.3-1131-gd07a3e7c2faf3f3489ae80e17cb2f02533c392e6
OS                             Microsoft Windows 10.0.17134
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0
@bergmeister bergmeister changed the title AMSI not uninitialized when pwsh is not crossgen'd and the window hosting pwsh (directly or indirectly)t gets closed (on Windows) AMSI not uninitialized when pwsh is not crossgen'd and the window hosting pwsh (directly or indirectly) gets closed (on Windows) May 13, 2018
@iSazonov
Copy link
Collaborator

/cc @TravisEz13 @PaulHigin Please look the AMSI issue.
8000

@iSazonov
Copy link
Collaborator

/cc @SteveL-MSFT @dantraMSFT This is just the case about Assert that we discussed recently. Perhaps we should audit the code (Asserts).

@iSazonov iSazonov added WG-Engine core PowerShell engine, interpreter, and runtime Issue-Code Cleanup the issue is for cleaning up the code with no impact on functionality labels May 14, 2018
@PaulHigin
Copy link
Contributor

Thanks. I'll take a look sometime this week.

@PaulHigin
Copy link
Contributor

When PowerShell is closed via the console (clicking on window [x]) there is a race condition between PowerShell runspace clean up, the appdomain callback for the AMSI uninit assert check, and the process terminating. I don't believe this code was intended to work for this case, but instead is for the case where PowerShell exits cleanly from the command line (by typing 'exit').

I believe the assert is useful for the cleanly exiting case so it would be good to keep it. It is interesting that we are seeing the assert for the "Invoke-Item" case, when in most other cases the process terminates before the assert can be displayed.

@bergmeister
Copy link
Contributor Author

Thanks for the info. The fact that the assertion failure does not happen when using the -CrossGen switch confirms to me as well that it is a race condition as some code is faster. I guess there could also be scenarios on different hardware where this happens even when using -CrossGen. We should maybe eat more dog food that is compiled in Debug to uncover such cases.

@iSazonov iSazonov mentioned this issue Oct 1, 2018
Closed
@vexx32 vexx32 mentioned this issue Oct 11, 2018
Open
@bergmeister
Copy link
Contributor Author

Note that I still see this happening with the latest changes in master but the likelihood seems to have decreased a bit.

@iSazonov
Copy link
Collaborator

Continues to annoy.

@PaulHigin
Copy link
Contributor

I agree. I am beginning to use PSCore6 more than Windows PowerShell and it is very annoying. I think the assert should be removed. I'll check with the AMSI folks and see if it is Ok to ignore the un-initialize on process exit.

@SteveL-MSFT
Copy link
Member

We discussed this briefly this morning and agree that @PaulHigin will submit PR to remove this assert as it's no longer providing value

@SteveL-MSFT SteveL-MSFT mentioned this issue Jan 22, 2019
Closed
@PaulHigin
Copy link
Contributor

I have submitted a PR (#8713).

@bergmeister
Copy link
Contributor Author

@PaulHigin One can use closing keywords in the PR (e.g Fixes #REPLACE_WITH_ISSUE_NUMBER), then the issue gets closed automatically when the PR gets merged. It is not important in this case but useful in general where people track issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Issue-Code Cleanup the issue is for cleaning up the code with no impact on functionality WG-Engine core PowerShell engine, interpreter, and runtime
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@bergmeister @SteveL-MSFT @PaulHigin @iSazonov
0