Fix interleaved heap RW commit failure handling #73566
Conversation
We were not checking the result of the ExecutableAllocator::Commit call for mapping the RW page in the case of interleaved heaps. I've seen a failure in the CI that seems to be caused by this - when we succeeded committing the RX page, but failed to commit the related RW page, we have then crashed when trying to initialize the precode stubs data. This change adds the check to fix the problem. This causes the LoaderHeap allocation to fail as expected in such case.
| @@ -1304,14 +1304,17 @@ BOOL UnlockedLoaderHeap::GetMoreCommittedPages(size_t dwMinSize) | |||
| void *pData = ExecutableAllocator::Instance()->Commit(m_pPtrToEndOfCommittedRegion, dwSizeToCommitPart, IsExecutable()); | |||
| if (pData == NULL) | |||
| { | |||
| _ASSERTE(!"Unable to commit a loaderheap page"); | |||
There was a problem hiding this comment.
I have removed this assert since I've noticed the same CI test that uncovered the issue has failed due to this assert in another run. We should just return FALSE and let the caller handle the memory allocation failure.
There was a problem hiding this comment.
I've seen this assert occasionally on macOS-arm64 Fuzzlyn runs but have not had time to create a repro case. When would we expect this to occur? In low memory situations or for other reasons too?
There was a problem hiding this comment.
In low memory situation only.
|
The only failure in the CI tests is the known GetGCMemoryInfo failure on arm32. |
believe a change which might have caused this failure was reverted which should have avoided the failure. Perhaps your CI didnt pickup that change? |
I think it didn't. I would have to close and reopen the PR to pick the latest changes in main. Since it was the only issue in the CI and it was known, I would just merge this. I was just waiting for an approval. |
ghost
locked as resolved and limited conversation to collaborators
We were not checking the result of the
ExecutableAllocator::Commitcallfor mapping the RW page in the case of interleaved heaps. I've seen a
failure in the CI that seems to be caused by this - when we succeeded
committing the RX page, but failed to commit the related RW page, we
have then crashed when trying to initialize the precode stubs data.
This change adds the check to fix the problem. This causes the
LoaderHeapallocation to fail as expected in such case.Close #71624