Fix crash with unmapped shuffle thunk stub #55718
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change fixes an intermittent issue that was showing up in the System.Linq.Expressions.Tests suite. When the delegate class was in a collectible ALC, some of the stubs were being used even after the underlying loader allocator was deleted, thus the memory the stubs occupied was already unmapped. Before my recent W^X change, the stubs were always being allocated from an executable allocator / executable heap in the global loader allocator due to a bug in the AssemblyLoaderAllocator::SetCollectible and AssemblyLoaderAllocator::Init ordering (the SetCollectible was being called before the Init, so the m_pStubHeap was not yet set and the ShuffleThunkCache was being passed NULL as the heap pointer. The cache handles that case as a request to allocate from global executable heap. In this fix, I've changed the AssemblyLoaderAllocator::Init to pass the SystemDomain::GetGlobalLoaderAllocator()->GetExecutableHeap() as the heap to the ShuffleThunkCache constructor. It is a workaround until the actual issue with stubs outliving the delegate classes is understood and fixed. Besides the fix, this change also fixes two unrelated issues that would only possibly cause trouble when the W^X is enabled. There were two places where memory was being reserved by the new ExecutableAllocator, but cleaned up using the ClrVirtualFree.
|
Thank you for tracking it down. |
|
The CI is acting weirdly. I cannot restart the failed leg and even closing / reopening the PR doesn't have any effect. |
|
Ah, it seems like the CI is just overloaded. Now it started to rerun the legs after an hour of inactivity. |
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change fixes an intermittent issue that was showing up in
the System.Linq.Expressions.Tests suite. When the delegate class
was in a collectible ALC, some of the stubs were being used even
after the underlying loader allocator was deleted, thus the memory
the stubs occupied was already unmapped.
Before my recent W^X change, the stubs were always being allocated
from an executable allocator / executable heap in the global loader
allocator due to a bug in the AssemblyLoaderAllocator::SetCollectible
and AssemblyLoaderAllocator::Init ordering (the SetCollectible
was being called before the Init, so the m_pStubHeap was not yet
set and the ShuffleThunkCache was being passed NULL as the heap
pointer. The cache handles that case as a request to allocate from
global executable heap.
In this fix, I've changed the AssemblyLoaderAllocator::Init to pass
the SystemDomain::GetGlobalLoaderAllocator()->GetExecutableHeap()
as the heap to the ShuffleThunkCache constructor. It is a workaround
until the actual issue with stubs outliving the delegate classes
is understood and fixed.
Besides the fix, this change also fixes two unrelated issues that
would only possibly cause trouble when the W^X is enabled. There
were two places where memory was being reserved by the new
ExecutableAllocator, but cleaned up using the ClrVirtualFree.
Close #55536