Skip to content

Fix edge case of code heap initial request#71213

Merged
janvorli merged 1 commit into
dotnet:mainfrom
janvorli:fix-codeheap-initial-request-edge-case
Jun 23, 2022
Merged

Fix edge case of code heap initial request#71213
janvorli merged 1 commit into
dotnet:mainfrom
janvorli:fix-codeheap-initial-request-edge-case

Conversation

@janvorli

Copy link
Copy Markdown
Member

When the requested size passed to the LoaderCodeHeap::CreateCodeHeap is less
than 16 bytes smaller than the COLLECTIBLE_CODEHEAP_SIZE, we incorrectly
try to use the initially preallocated block. The problem is that besides the
requested size, we also allocate JUMP_ALLOCATE_SIZE sized block. In the
edge case mentioned above, it doesn't fit and we assert in debug builds
of the runtime.
In release build, it would throw OOM in such case.

This issue was found by the Fuzzlyn tool.

Close #71200

When the requested size passed to the LoaderCodeHeap::CreateCodeHeap is less
than 16 bytes smaller than the COLLECTIBLE_CODEHEAP_SIZE, we incorrectly
try to use the initially preallocated block. The problem is that besides the
requested size, we also allocate JUMP_ALLOCATE_SIZE sized block. In the
edge case mentioned above, it doesn't fit and we assert in debug builds
of the runtime.
In release build, it would throw OOM in such case.
@janvorli janvorli added this to the 7.0.0 milestone Jun 23, 2022
@janvorli
janvorli requested a review from jkotas June 23, 2022 16:13
@janvorli janvorli self-assigned this Jun 23, 2022
@janvorli

Copy link
Copy Markdown
Member Author

@jkotas most of the changes are just because I've made the AllocMem_TotalSize a member of the UnlockedLoaderHeap. It was kind of weird to not to be that way.


pBaseAddr = (BYTE *)pInfo->m_pAllocator->GetCodeHeapInitialBlock(loAddr, hiAddr, (DWORD)initialRequestSize, &dwSizeAcquiredFromInitialBlock);
size_t allocationSize = pCodeHeap->m_LoaderHeap.AllocMem_TotalSize(initialRequestSize);
#if defined(TARGET_AMD64) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use #ifdef TARGET_64BIT instead (as done below on line 2600)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can do that. I've just copied the same ifdef that we have below where we allocate the personality routine. But there are many more places in the code where we have this same compound #if. So maybe it would be worth a separate cleanup instead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw even the CLRPersonalityRoutine declaration is under this compound ifdef

@am11 am11 Jun 23, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Some of the usage of HOST_64BIT in VM can also be reviewed as part of that work, because it looks like the correct macro for them (in most places) is TARGET_64BIT (but it will require an in-depth analysis: #2256 (comment)).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created issue #71220 for that.

@janvorli
janvorli merged commit 8c05969 into dotnet:main Jun 23, 2022
@janvorli
janvorli deleted the fix-codeheap-initial-request-edge-case branch June 23, 2022 21:19
@ghost ghost locked as resolved and limited conversation to collaborators Jul 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Assert failure: heapSize >= initialRequestSize with collectible assemblies on linux-x64

3 participants