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

* Add FEATURE_LOADER_HEAP_GUARD feature

* Invoke memset only for reclaimed regions

* Enable FEATURE_LOADER_HEAP_GUARD by default

* Insert trap inside UMEntryThunk::Terminate

* Make all exectuable heaps not to zero-initialize itself

Use fZeroInit (instead of fMakeRelazed)

* Add comment

* Revert unnecessary changes

* Add and use 'Poison' method to insert a trap

* Do NOT invoke FlushInstructionCache

* Update comment

* Add comment on ARM Poisoning instruction

* Use X86_INSTR_INT3 instead of 0xCC

diff --git a/src/inc/loaderheap.h b/src/inc/loaderheap.h
index 7d4c48f..4333505 100644 (file)
--- a/src/inc/loaderheap.h
+++ b/src/inc/loaderheap.h
@@ -217,7 +217,7 @@ private:
 
     size_t *             m_pPrivatePerfCounter_LoaderBytes;
 
-    DWORD                m_flProtect;
+    DWORD                m_Options;
 
     LoaderHeapFreeBlock *m_pFirstFreeBlock;
 
@@ -288,7 +288,8 @@ protected:
                        SIZE_T dwReservedRegionSize,
                        size_t *pPrivatePerfCounter_LoaderBytes = NULL,
                        RangeList *pRangeList = NULL,
-                       BOOL fMakeExecutable = FALSE);
+                       BOOL fMakeExecutable = FALSE,
+                       BOOL fZeroInit = TRUE);
 
     ~UnlockedLoaderHeap();
 #endif
@@ -398,10 +399,8 @@ public:
         return m_dwTotalAlloc;
     }
 
-    BOOL IsExecutable()
-    {
-        return (PAGE_EXECUTE_READWRITE == m_flProtect);
-    }
+    BOOL IsExecutable();
+    BOOL IsZeroInit();
 
 
 public:
@@ -447,14 +446,16 @@ public:
                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;
@@ -469,7 +470,8 @@ public:
                SIZE_T dwReservedRegionSize,
                size_t *pPrivatePerfCounter_LoaderBytes = NULL,
                RangeList *pRangeList = NULL,
-               BOOL fMakeExecutable = FALSE
+               BOOL fMakeExecutable = FALSE,
+               BOOL fZeroInit = TRUE
                )
       : UnlockedLoaderHeap(dwReserveBlockSize,
                            dwCommitBlockSize,
@@ -477,7 +479,8 @@ public:
                            dwReservedRegionSize,
                            pPrivatePerfCounter_LoaderBytes,
                            pRangeList,
-                           fMakeExecutable)
+                           fMakeExecutable,
+                           fZeroInit)
     {
         WRAPPER_NO_CONTRACT;
         m_CriticalSection = NULL;

diff --git a/src/utilcode/loaderheap.cpp b/src/utilcode/loaderheap.cpp
index 3f1063c..2603a8e 100644 (file)
--- a/src/utilcode/loaderheap.cpp
+++ b/src/utilcode/loaderheap.cpp
@@ -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;)
@@ -903,7 +906,8 @@ UnlockedLoaderHeap::UnlockedLoaderHeap(DWORD dwReserveBlockSize,
                                        SIZE_T dwReservedRegionSize, 
                                        size_t *pPrivatePerfCounter_LoaderBytes,
                                        RangeList *pRangeList,
-                                       BOOL fMakeExecutable)
+                                       BOOL fMakeExecutable,
+                                       BOOL fZeroInit)
 {
     CONTRACTL
     {
@@ -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;
 
@@ -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");
@@ -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;
 
@@ -1351,7 +1356,7 @@ again:
             // 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");
@@ -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
@@ -1639,6 +1645,7 @@ void *UnlockedLoaderHeap::UnlockedAllocAlignedMem_NoThrow(size_t  dwRequestedSiz
 
     
     ((BYTE*&)pResult) += extra;
+
 #ifdef _DEBUG
      BYTE *pAllocatedBytes = (BYTE *)pResult;
 #if LOADER_HEAP_DEBUG_BOUNDARY > 0
@@ -1646,7 +1653,7 @@ void *UnlockedLoaderHeap::UnlockedAllocAlignedMem_NoThrow(size_t  dwRequestedSiz
     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");
@@ -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)

diff --git a/src/vm/amd64/cgenamd64.cpp b/src/vm/amd64/cgenamd64.cpp
index 497abcd..20dca22 100644 (file)
--- a/src/vm/amd64/cgenamd64.cpp
+++ b/src/vm/amd64/cgenamd64.cpp
@@ -670,6 +670,19 @@ void UMEntryThunkCode::Encode(BYTE* pTargetCode, void* pvSecretParam)
     _ASSERTE(DbgIsExecutable(&m_movR10[0], &m_jmpRAX[3]-&m_movR10[0]));
 }
 
+void UMEntryThunkCode::Poison()
+{
+    CONTRACTL
+    {
+        NOTHROW;
+        GC_NOTRIGGER;
+        MODE_ANY;
+    }
+    CONTRACTL_END;
+
+    m_movR10[0] = X86_INSTR_INT3;
+}
+
 UMEntryThunk* UMEntryThunk::Decode(LPVOID pCallback)
 {
     LIMITED_METHOD_CONTRACT;

diff --git a/src/vm/amd64/cgencpu.h b/src/vm/amd64/cgencpu.h
index 64a6501..b74e3ca 100644 (file)
--- a/src/vm/amd64/cgencpu.h
+++ b/src/vm/amd64/cgencpu.h
@@ -472,6 +472,7 @@ struct DECLSPEC_ALIGN(8) UMEntryThunkCode
     BYTE            m_padding2[5];
 
     void Encode(BYTE* pTargetCode, void* pvSecretParam);
+    void Poison();
 
     LPCBYTE GetEntryPoint() const
     {

diff --git a/src/vm/arm/cgencpu.h b/src/vm/arm/cgencpu.h
index 181d5f1..6f128f6 100644 (file)
--- a/src/vm/arm/cgencpu.h
+++ b/src/vm/arm/cgencpu.h
@@ -988,6 +988,7 @@ struct DECLSPEC_ALIGN(4) UMEntryThunkCode
     TADDR       m_pvSecretParam;
 
     void Encode(BYTE* pTargetCode, void* pvSecretParam);
+    void Poison();
 
     LPCBYTE GetEntryPoint() const
     {

diff --git a/src/vm/arm/stubs.cpp b/src/vm/arm/stubs.cpp
index 2e8bb19..39b5937 100644 (file)
--- a/src/vm/arm/stubs.cpp
+++ b/src/vm/arm/stubs.cpp
@@ -2522,6 +2522,12 @@ void UMEntryThunkCode::Encode(BYTE* pTargetCode, void* pvSecretParam)
     FlushInstructionCache(GetCurrentProcess(),&m_code,sizeof(m_code));
 }
 
+void UMEntryThunkCode::Poison()
+{
+    // Insert 'bkpt 0x00be' at the entry point
+    m_code[0] = 0xbebe;
+}
+
 ///////////////////////////// UNIMPLEMENTED //////////////////////////////////
 
 #ifndef DACCESS_COMPILE

diff --git a/src/vm/arm64/cgencpu.h b/src/vm/arm64/cgencpu.h
index d8bbcf7..5c522c5 100644 (file)
--- a/src/vm/arm64/cgencpu.h
+++ b/src/vm/arm64/cgencpu.h
@@ -481,6 +481,7 @@ struct DECLSPEC_ALIGN(16) UMEntryThunkCode
     TADDR       m_pvSecretParam;
 
     void Encode(BYTE* pTargetCode, void* pvSecretParam);
+    void Poison();
 
     LPCBYTE GetEntryPoint() const
     {

diff --git a/src/vm/arm64/stubs.cpp b/src/vm/arm64/stubs.cpp
index 40d2749..df2124d 100644 (file)
--- a/src/vm/arm64/stubs.cpp
+++ b/src/vm/arm64/stubs.cpp
@@ -1244,6 +1244,10 @@ void UMEntryThunkCode::Encode(BYTE* pTargetCode, void* pvSecretParam)
     FlushInstructionCache(GetCurrentProcess(),&m_code,sizeof(m_code));
 }
 
+void UMEntryThunkCode::Poison()
+{
+
+}
 
 #ifdef PROFILING_SUPPORTED
 #include "proftoeeinterfaceimpl.h"

diff --git a/src/vm/dllimportcallback.cpp b/src/vm/dllimportcallback.cpp
index 90c01a4..8684c12 100644 (file)
--- a/src/vm/dllimportcallback.cpp
+++ b/src/vm/dllimportcallback.cpp
@@ -1111,13 +1111,8 @@ 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;
 }
@@ -1126,11 +1121,10 @@ void UMEntryThunk::Terminate()
 {
     WRAPPER_NO_CONTRACT;
 
-#ifdef FEATURE_WINDOWSPHONE
+    _ASSERTE(!SystemDomain::GetGlobalLoaderAllocator()->GetExecutableHeap()->IsZeroInit());
+    m_code.Poison();
+
     SystemDomain::GetGlobalLoaderAllocator()->GetExecutableHeap()->BackoutMem(this, sizeof(UMEntryThunk));
-#else
-    DeleteExecutable(this);
-#endif
 }
 
 VOID UMEntryThunk::FreeUMEntryThunk(UMEntryThunk* p)

diff --git a/src/vm/dllimportcallback.h b/src/vm/dllimportcallback.h
index af2a0b1..e79c5f0 100644 (file)
--- a/src/vm/dllimportcallback.h
+++ b/src/vm/dllimportcallback.h
@@ -326,10 +326,6 @@ public:
         {
             DestroyLongWeakHandle(GetObjectHandle());
         }
-
-#ifdef _DEBUG
-        FillMemory(this, sizeof(*this), 0xcc);
-#endif
     }
 
     void Terminate();

diff --git a/src/vm/i386/cgencpu.h b/src/vm/i386/cgencpu.h
index ff76d99..e4a623b 100644 (file)
--- a/src/vm/i386/cgencpu.h
+++ b/src/vm/i386/cgencpu.h
@@ -504,6 +504,7 @@ struct DECLSPEC_ALIGN(4) UMEntryThunkCode
     const BYTE *    m_execstub; // pointer to destination code  // make sure the backpatched portion is dword aligned.
 
     void Encode(BYTE* pTargetCode, void* pvSecretParam);
+    void Poison();
 
     LPCBYTE GetEntryPoint() const
     {

diff --git a/src/vm/i386/cgenx86.cpp b/src/vm/i386/cgenx86.cpp
index c75490b..e315ffb 100644 (file)
--- a/src/vm/i386/cgenx86.cpp
+++ b/src/vm/i386/cgenx86.cpp
@@ -1588,6 +1588,13 @@ void UMEntryThunkCode::Encode(BYTE* pTargetCode, void* pvSecretParam)
     FlushInstructionCache(GetCurrentProcess(),GetEntryPoint(),sizeof(UMEntryThunkCode));
 }
 
+void UMEntryThunkCode::Poison()
+{
+    LIMITED_METHOD_CONTRACT;
+
+    m_movEAX = X86_INSTR_INT3;
+}
+
 UMEntryThunk* UMEntryThunk::Decode(LPVOID pCallback)
 {
     LIMITED_METHOD_CONTRACT;

diff --git a/src/vm/loaderallocator.cpp b/src/vm/loaderallocator.cpp
index 1a05bf2..ff54277 100644 (file)
--- a/src/vm/loaderallocator.cpp
+++ b/src/vm/loaderallocator.cpp
@@ -1005,7 +1005,9 @@ void LoaderAllocator::Init(BaseDomain *pDomain, BYTE *pExecutableHeapMemory)
                                                                       dwExecutableHeapReserveSize,
                                                                       LOADERHEAP_PROFILE_COUNTER,
                                                                       NULL,
-                                                                      TRUE /* Make heap executable */);
+                                                                      TRUE /* Make heap executable */,
+                                                                      FALSE /* Disable zero-initialization (needed by UMEntryThunkCode::Poison) */
+                                                                      );
         initReservedMem += dwExecutableHeapReserveSize;
     }