Unregister the event handler from 'AppDomain.CurrentDomain.ProcessExit' when uninitializing AMSI#10626
Conversation
|
I tried to add a test, but I cannot find a way to get the |
|
I can get the invocation method list in .NET, but not in .NET Core. The following works in Windows PowerShell $f = [System.AppDomain].GetFields([System.Reflection.BindingFlags]::Instance -bor [System.Reflection.BindingFlags]::NonPublic -bor [System.Reflection.BindingFlags]::GetField) | ? name -eq '_processExit'
$v = $f.GetValue([System.AppDomain]::CurrentDomain)
$v.GetInvocationList()
Method Target
------ ------
Void DisposeOnShutdown(System.Object, System.EventArgs)
Void CurrentDomain_ProcessExit(System.Object, System.EventArgs)
Void SingletonDomainUnload(System.Object, System.EventArgs) {System.EventHandler}So at least I can add a test when porting this change to Windows PowerShell. |
|
Hello @daxian-dbw! Because this pull request has the Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 24 hours, a condition that will be fulfilled in about 1 hour 59 minutes. No worries though, I will be back when the time is right! 😉 p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
@daxian-dbw I got it ("static" is key): $f = [System.AppContext].GetFields([System.Reflection.BindingFlags]::Instance -bor [System.Reflection.BindingFlags]::NonPublic -bor [System.Reflection.BindingFlags]::GetProperty -bor [System.Reflection.BindingFlags]::Static) | ? name -eq 'ProcessExit'
$v = $f.GetValue([System.AppDomain]::CurrentDomain)
# or $f.GetValue([System.AppContext]::ProcessExit)
$v.GetInvocationList() |
|
@iSazonov This is very strange, and feels to me may randomly stop working in future .NET Core 😄 |
|
@daxian-dbw AppContext is public API and the property is static - it is not likely that this public API will be changed. |
|
Ahh, I thought it was
This is wrong, should pass in |
|
Hmm, I cannot write a Pester test for this, because the leak won't happen as long as a Runspace still opens. Need to do it in xUnit. |
I don't understand your thought. It seem works well in the example I posted. |
|
It's a static field, so should pass |
|
Sorry, if I noisy but I always wonder how in-depth you look. |
|
I was not clear in my above statement. Today, you can pass anything to It may be because today the .net implementation ignore the argument when the field is static, but this could change in future so that exception would be thrown if the passed in argument is not null. |
|
Thanks for clarify! |
|
🎉 Handy links: |
Add a test for the even handler leaking fix addressed in PowerShell#10626 PowerShell#10626 The test is added to verify that PowerShell unregister the 'AppDomain.ProcessExit' event handler when a Runspace closes.
PR Summary
Unregister the event handler from
AppDomain.CurrentDomain.ProcessExitwhen uninitializing AMSI.The event handler was registered when opening a Runspace, but not unregistered when closing.
In the hosting scenario, every time a Runspace is opened/closed, an instance of the
EventHandlerwill be left behind linked to theAppDomain.CurrentDomain. In a dump file provided by a partner team, there are 588017 suchEventHandlerinstances leaked.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.