Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fill freed loader heap chunk with non-zero value #12731

Merged
merged 12 commits into from
Jul 31, 2017
Merged

Fill freed loader heap chunk with non-zero value #12731

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3384270
Add FEATURE_LOADER_HEAP_GUARD feature
parjong Jul 11, 2017
48e667b
Invoke memset only for reclaimed regions
parjong Jul 17, 2017
87b2287
Enable FEATURE_LOADER_HEAP_GUARD by default
parjong Jul 18, 2017
66939d1
Insert trap inside UMEntryThunk::Terminate
parjong Jul 19, 2017
0c13959
Make all exectuable heaps not to zero-initialize itself
parjong Jul 19, 2017
7c63a69
Add comment
parjong Jul 19, 2017
cd6d5ec
Revert unnecessary changes
parjong Jul 19, 2017
63dcb89
Add and use 'Poison' method to insert a trap
parjong Jul 19, 2017
03e2ed1
Do NOT invoke FlushInstructionCache
parjong Jul 20, 2017
4029d1c
Update comment
parjong Jul 20, 2017
c986ba4
Add comment on ARM Poisoning instruction
parjong Jul 31, 2017
9c97b74
Use X86_INSTR_INT3 instead of 0xCC
parjong Jul 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 13 additions & 10 deletions src/inc/loaderheap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ class UnlockedLoaderHeap

size_t * m_pPrivatePerfCounter_LoaderBytes;

DWORD m_flProtect;
DWORD m_Options;

LoaderHeapFreeBlock *m_pFirstFreeBlock;

Expand Down Expand Up @@ -288,7 +288,8 @@ class UnlockedLoaderHeap
SIZE_T dwReservedRegionSize,
size_t *pPrivatePerfCounter_LoaderBytes = NULL,
RangeList *pRangeList = NULL,
BOOL fMakeExecutable = FALSE);
BOOL fMakeExecutable = FALSE,
BOOL fZeroInit = TRUE);

~UnlockedLoaderHeap();
#endif
Expand Down Expand Up @@ -398,10 +399,8 @@ class UnlockedLoaderHeap
return m_dwTotalAlloc;
}

BOOL IsExecutable()
{
return (PAGE_EXECUTE_READWRITE == m_flProtect);
}
BOOL IsExecutable();
BOOL IsZeroInit();


public:
Expand Down Expand Up @@ -447,14 +446,16 @@ class LoaderHeap : public UnlockedLoaderHeap, public ILoaderHeapBackout
DWORD dwCommitBlockSize,
size_t *pPrivatePerfCounter_LoaderBytes = NULL,
RangeList *pRangeList = NULL,
BOOL fMakeExecutable = FALSE
BOOL fMakeExecutable = FALSE,
BOOL fZeroInit = TRUE
)
: UnlockedLoaderHeap(dwReserveBlockSize,
dwCommitBlockSize,
NULL, 0,
pPrivatePerfCounter_LoaderBytes,
pRangeList,
fMakeExecutable)
fMakeExecutable,
fZeroInit)
{
WRAPPER_NO_CONTRACT;
m_CriticalSection = NULL;
Expand All @@ -469,15 +470,17 @@ class LoaderHeap : public UnlockedLoaderHeap, public ILoaderHeapBackout
SIZE_T dwReservedRegionSize,
size_t *pPrivatePerfCounter_LoaderBytes = NULL,
RangeList *pRangeList = NULL,
BOOL fMakeExecutable = FALSE
BOOL fMakeExecutable = FALSE,
BOOL fZeroInit = TRUE
)
: UnlockedLoaderHeap(dwReserveBlockSize,
dwCommitBlockSize,
dwReservedRegionAddress,
dwReservedRegionSize,
pPrivatePerfCounter_LoaderBytes,
pRangeList,
fMakeExecutable)
fMakeExecutable,
fZeroInit)
{
WRAPPER_NO_CONTRACT;
m_CriticalSection = NULL;
Expand Down
35 changes: 26 additions & 9 deletions src/utilcode/loaderheap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
#define DONOT_DEFINE_ETW_CALLBACK
#include "eventtracebase.h"

#define LHF_EXECUTABLE 0x1
#define LHF_ZEROINIT 0x2

#ifndef DACCESS_COMPILE

INDEBUG(DWORD UnlockedLoaderHeap::s_dwNumInstancesOfLoaderHeaps = 0;)
Expand Down Expand Up @@ -903,7 +906,8 @@ UnlockedLoaderHeap::UnlockedLoaderHeap(DWORD dwReserveBlockSize,
SIZE_T dwReservedRegionSize,
size_t *pPrivatePerfCounter_LoaderBytes,
RangeList *pRangeList,
BOOL fMakeExecutable)
BOOL fMakeExecutable,
BOOL fZeroInit)
{
CONTRACTL
{
Expand Down Expand Up @@ -939,10 +943,11 @@ UnlockedLoaderHeap::UnlockedLoaderHeap(DWORD dwReserveBlockSize,

m_pPrivatePerfCounter_LoaderBytes = pPrivatePerfCounter_LoaderBytes;

m_Options = 0;
if (fMakeExecutable)
m_flProtect = PAGE_EXECUTE_READWRITE;
else
m_flProtect = PAGE_READWRITE;
m_Options |= LHF_EXECUTABLE;
if (fZeroInit)
m_Options |= LHF_ZEROINIT;

m_pFirstFreeBlock = NULL;

Expand Down Expand Up @@ -1133,7 +1138,7 @@ BOOL UnlockedLoaderHeap::UnlockedReservePages(size_t dwSizeToCommit)
}

// Commit first set of pages, since it will contain the LoaderHeapBlock
void *pTemp = ClrVirtualAlloc(pData, dwSizeToCommit, MEM_COMMIT, m_flProtect);
void *pTemp = ClrVirtualAlloc(pData, dwSizeToCommit, MEM_COMMIT, (m_Options & LHF_EXECUTABLE) ? PAGE_EXECUTE_READWRITE : PAGE_READWRITE);
if (pTemp == NULL)
{
//_ASSERTE(!"Unable to ClrVirtualAlloc commit in a loaderheap");
Expand Down Expand Up @@ -1225,7 +1230,7 @@ BOOL UnlockedLoaderHeap::GetMoreCommittedPages(size_t dwMinSize)
dwSizeToCommit = ALIGN_UP(dwSizeToCommit, GetOsPageSize());

// Yes, so commit the desired number of reserved pages
void *pData = ClrVirtualAlloc(m_pPtrToEndOfCommittedRegion, dwSizeToCommit, MEM_COMMIT, m_flProtect);
void *pData = ClrVirtualAlloc(m_pPtrToEndOfCommittedRegion, dwSizeToCommit, MEM_COMMIT, (m_Options & LHF_EXECUTABLE) ? PAGE_EXECUTE_READWRITE : PAGE_READWRITE);
if (pData == NULL)
return FALSE;

Expand Down Expand Up @@ -1351,7 +1356,7 @@ void *UnlockedLoaderHeap::UnlockedAllocMem_NoThrow(size_t dwSize
// Don't fill the memory we allocated - it is assumed to be zeroed - fill the memory after it
memset(pAllocatedBytes + dwRequestedSize, 0xEE, LOADER_HEAP_DEBUG_BOUNDARY);
#endif
if (dwRequestedSize > 0)
if ((dwRequestedSize > 0) && (m_Options & LHF_ZEROINIT))
{
_ASSERTE_MSG(pAllocatedBytes[0] == 0 && memcmp(pAllocatedBytes, pAllocatedBytes + 1, dwRequestedSize - 1) == 0,
"LoaderHeap must return zero-initialized memory");
Expand Down Expand Up @@ -1529,7 +1534,8 @@ void UnlockedLoaderHeap::UnlockedBackoutMem(void *pMem,
{
// Cool. This was the last block allocated. We can just undo the allocation instead
// of going to the freelist.
memset(pMem, 0, dwSize); // Must zero init this memory as AllocMem expect it
if (m_Options & LHF_ZEROINIT)
memset(pMem, 0x00, dwSize); // Fill freed region with 0
m_pAllocPtr = (BYTE*)pMem;
}
else
Expand Down Expand Up @@ -1639,14 +1645,15 @@ void *UnlockedLoaderHeap::UnlockedAllocAlignedMem_NoThrow(size_t dwRequestedSiz


((BYTE*&)pResult) += extra;

#ifdef _DEBUG
BYTE *pAllocatedBytes = (BYTE *)pResult;
#if LOADER_HEAP_DEBUG_BOUNDARY > 0
// Don't fill the entire memory - we assume it is all zeroed -just the memory after our alloc
memset(pAllocatedBytes + dwRequestedSize, 0xee, LOADER_HEAP_DEBUG_BOUNDARY);
#endif

if (dwRequestedSize != 0)
if ((dwRequestedSize != 0) && (m_Options & LHF_ZEROINIT))
{
_ASSERTE_MSG(pAllocatedBytes[0] == 0 && memcmp(pAllocatedBytes, pAllocatedBytes + 1, dwRequestedSize - 1) == 0,
"LoaderHeap must return zero-initialized memory");
Expand Down Expand Up @@ -1766,6 +1773,16 @@ void *UnlockedLoaderHeap::UnlockedAllocMemForCode_NoThrow(size_t dwHeaderSize, s

#endif // #ifndef DACCESS_COMPILE

BOOL UnlockedLoaderHeap::IsExecutable()
{
return (m_Options & LHF_EXECUTABLE);
}

BOOL UnlockedLoaderHeap::IsZeroInit()
{
return (m_Options & LHF_ZEROINIT);
}

#ifdef DACCESS_COMPILE

void UnlockedLoaderHeap::EnumMemoryRegions(CLRDataEnumMemoryFlags flags)
Expand Down
22 changes: 13 additions & 9 deletions src/vm/dllimportcallback.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1111,28 +1111,32 @@ UMEntryThunk* UMEntryThunk::CreateUMEntryThunk()

UMEntryThunk * p;

#ifdef FEATURE_WINDOWSPHONE
// On the phone, use loader heap to save memory commit of regular executable heap
p = (UMEntryThunk *)(void *)SystemDomain::GetGlobalLoaderAllocator()->GetExecutableHeap()->AllocMem(S_SIZE_T(sizeof(UMEntryThunk)));
#else
p = new (executable) UMEntryThunk;
memset (p, 0, sizeof(*p));
#endif

RETURN p;
}

#if defined(_TARGET_X86_) || defined(_TARGET_AMD64_)
#define REDZONE_VALUE 0xcc
Copy link
Member

Choose a reason for hiding this comment

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

Red zone is area under stack pointer: https://en.m.wikipedia.org/wiki/Red_zone_(computing)

Could you please find better name for it?

Copy link
Member

Choose a reason for hiding this comment

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

It may be best to move this into a platform specific method on UMEntryThunkCode . memset may not be always appropriate to neutralize the entrypoint.

Copy link
Author

Choose a reason for hiding this comment

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

How about poison pill?

Copy link
Author

Choose a reason for hiding this comment

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

We may introduce Spoil method as you suggested, which allows us to insert a breakpoint to the exact entry point (instead of filling the whole region).

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a plan. Poison or Neutralize may be a better name than Spoil.

#elif defined(_TARGET_ARM_)
#define REDZONE_VALUE 0xBE
#else
#define REDZONE_VALUE 0x00
#endif

void UMEntryThunk::Terminate()
{
WRAPPER_NO_CONTRACT;

#ifdef FEATURE_WINDOWSPHONE
_ASSERTE(!SystemDomain::GetGlobalLoaderAllocator()->GetExecutableHeap()->IsZeroInit());
FillMemory(&m_code, sizeof(m_code), REDZONE_VALUE);

SystemDomain::GetGlobalLoaderAllocator()->GetExecutableHeap()->BackoutMem(this, sizeof(UMEntryThunk));
#else
DeleteExecutable(this);
#endif
}

#undef REDZONE_VALUE

VOID UMEntryThunk::FreeUMEntryThunk(UMEntryThunk* p)
{
CONTRACTL
Expand Down
4 changes: 0 additions & 4 deletions src/vm/dllimportcallback.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,10 +326,6 @@ class UMEntryThunk
{
DestroyLongWeakHandle(GetObjectHandle());
}

#ifdef _DEBUG
FillMemory(this, sizeof(*this), 0xcc);
#endif
}

void Terminate();
Expand Down
3 changes: 2 additions & 1 deletion src/vm/loaderallocator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1005,7 +1005,8 @@ void LoaderAllocator::Init(BaseDomain *pDomain, BYTE *pExecutableHeapMemory)
dwExecutableHeapReserveSize,
LOADERHEAP_PROFILE_COUNTER,
NULL,
TRUE /* Make heap executable */);
TRUE /* Make heap executable */,
FALSE /* Do NOT zero-initialize */);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe useful to describe why - that it is needed by the UMEntryThunkCode::Poison.

initReservedMem += dwExecutableHeapReserveSize;
}

Expand Down