Unify macOS ARM64 write protection holders with W^X ones (#54067)

* Unify macOS ARM64 write protection holders with W^X ones This change removes the original holders that were added for changing memory protection for executable code and moves the actual switching to the recently added W^X holders. The unixexports files don't support target specific symbols. So I needed to export a dummy version of the PAL_JitWriteProtect for macOS x64.
dotnet · Jun 12, 2021 · cc6d314 · cc6d314
1 parent 2a011f8
commit cc6d314
Show file tree

Hide file tree

Showing 32 changed files with 107 additions and 302 deletions.
diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp
@@ -83,7 +83,11 @@ SharedPatchBypassBuffer* DebuggerControllerPatch::GetOrCreateSharedPatchBypassBu
 
     if (m_pSharedPatchBypassBuffer == NULL)
     {
-        m_pSharedPatchBypassBuffer = new (interopsafeEXEC) SharedPatchBypassBuffer();
+        void *pSharedPatchBypassBufferRX = g_pDebugger->GetInteropSafeExecutableHeap()->Alloc(sizeof(SharedPatchBypassBuffer));
+        ExecutableWriterHolder<SharedPatchBypassBuffer> sharedPatchBypassBufferWriterHolder((SharedPatchBypassBuffer*)pSharedPatchBypassBufferRX, sizeof(SharedPatchBypassBuffer));
+        new (sharedPatchBypassBufferWriterHolder.GetRW()) SharedPatchBypassBuffer();
+        m_pSharedPatchBypassBuffer = (SharedPatchBypassBuffer*)pSharedPatchBypassBufferRX;
+
         _ASSERTE(m_pSharedPatchBypassBuffer);
         TRACE_ALLOC(m_pSharedPatchBypassBuffer);
     }
@@ -1364,9 +1368,7 @@ bool DebuggerController::ApplyPatch(DebuggerControllerPatch *patch)
 
         LPVOID baseAddress = (LPVOID)(patch->address);
 
-#if defined(HOST_OSX) && defined(HOST_ARM64)
-        auto jitWriteEnableHolder = PAL_JITWriteEnable(true);
-#else // defined(HOST_OSX) && defined(HOST_ARM64)
+#if !defined(HOST_OSX) || !defined(HOST_ARM64)
         DWORD oldProt;
 
         if (!VirtualProtect(baseAddress,
@@ -1376,7 +1378,7 @@ bool DebuggerController::ApplyPatch(DebuggerControllerPatch *patch)
             _ASSERTE(!"VirtualProtect of code page failed");
             return false;
         }
-#endif // defined(HOST_OSX) && defined(HOST_ARM64)
+#endif // !defined(HOST_OSX) || !defined(HOST_ARM64)
 
         patch->opcode = CORDbgGetInstruction(patch->address);
 
@@ -1391,7 +1393,7 @@ bool DebuggerController::ApplyPatch(DebuggerControllerPatch *patch)
             _ASSERTE(!"VirtualProtect of code page failed");
             return false;
         }
-#endif // !defined(HOST_OSX) || !defined(HOST_ARM64)
+#endif // !defined(HOST_OSX) || !defined(HOST_ARM64)        
     }
 // TODO: : determine if this is needed for AMD64
 #if defined(TARGET_X86) //REVISIT_TODO what is this?!
@@ -1408,12 +1410,14 @@ bool DebuggerController::ApplyPatch(DebuggerControllerPatch *patch)
             _ASSERTE(!"VirtualProtect of code page failed");
             return false;
         }
+
         patch->opcode =
           (unsigned int) *(unsigned short*)(patch->address+1);
 
         _ASSERTE(patch->opcode != CEE_BREAK);
 
-        *(unsigned short *) (patch->address+1) = CEE_BREAK;
+        ExecutableWriterHolder<BYTE> breakpointWriterHolder((BYTE*)patch->address, 2);
+        *(unsigned short *) (breakpointWriterHolder.GetRW()+1) = CEE_BREAK;
 
         if (!VirtualProtect((void *) patch->address, 2, oldProt, &oldProt))
         {
@@ -1460,9 +1464,7 @@ bool DebuggerController::UnapplyPatch(DebuggerControllerPatch *patch)
 
         LPVOID baseAddress = (LPVOID)(patch->address);
 
-#if defined(HOST_OSX) && defined(HOST_ARM64)
-        auto jitWriteEnableHolder = PAL_JITWriteEnable(true);
-#else // defined(HOST_OSX) && defined(HOST_ARM64)
+#if !defined(HOST_OSX) || !defined(HOST_ARM64)
         DWORD oldProt;
 
         if (!VirtualProtect(baseAddress,
@@ -1477,7 +1479,7 @@ bool DebuggerController::UnapplyPatch(DebuggerControllerPatch *patch)
             InitializePRD(&(patch->opcode));
             return false;
         }
-#endif // defined(HOST_OSX) && defined(HOST_ARM64)
+#endif // !defined(HOST_OSX) || !defined(HOST_ARM64)
 
         CORDbgSetInstruction((CORDB_ADDRESS_TYPE *)patch->address, patch->opcode);
 
@@ -1494,7 +1496,7 @@ bool DebuggerController::UnapplyPatch(DebuggerControllerPatch *patch)
             _ASSERTE(!"VirtualProtect of code page failed");
             return false;
         }
-#endif // !defined(HOST_OSX) || !defined(HOST_ARM64)
+#endif // !defined(HOST_OSX) || !defined(HOST_ARM64)        
     }
     else
     {
@@ -1519,7 +1521,8 @@ bool DebuggerController::UnapplyPatch(DebuggerControllerPatch *patch)
 #if defined(TARGET_X86)
         _ASSERTE(*(unsigned short*)(patch->address+1) == CEE_BREAK);
 
-        *(unsigned short *) (patch->address+1)
+        ExecutableWriterHolder<BYTE> breakpointWriterHolder((BYTE*)patch->address, 2);
+        *(unsigned short *) (breakpointWriterHolder.GetRW()+1)
           = (unsigned short) patch->opcode;
 #endif //this makes no sense on anything but X86
         //VERY IMPORTANT to zero out opcode, else we might mistake

diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp
@@ -67,7 +67,6 @@ bool g_EnableSIS = false;
 
 // The following instances are used for invoking overloaded new/delete
 InteropSafe interopsafe;
-InteropSafeExecutable interopsafeEXEC;
 
 #ifndef DACCESS_COMPILE
 
@@ -1316,16 +1315,15 @@ DebuggerEval::DebuggerEval(CONTEXT * pContext, DebuggerIPCE_FuncEvalInfo * pEval
 {
     WRAPPER_NO_CONTRACT;
 
-#if defined(HOST_OSX) && defined(HOST_ARM64)
-    auto jitWriteEnableHolder = PAL_JITWriteEnable(true);
-#endif // defined(HOST_OSX) && defined(HOST_ARM64)
-
     // Allocate the breakpoint instruction info in executable memory.
-    m_bpInfoSegment = new (interopsafeEXEC, nothrow) DebuggerEvalBreakpointInfoSegment(this);
+    void *bpInfoSegmentRX = g_pDebugger->GetInteropSafeExecutableHeap()->Alloc(sizeof(DebuggerEvalBreakpointInfoSegment));
+    ExecutableWriterHolder<DebuggerEvalBreakpointInfoSegment> bpInfoSegmentWriterHolder((DebuggerEvalBreakpointInfoSegment*)bpInfoSegmentRX, sizeof(DebuggerEvalBreakpointInfoSegment));
+    new (bpInfoSegmentWriterHolder.GetRW()) DebuggerEvalBreakpointInfoSegment(this);
+    m_bpInfoSegment = (DebuggerEvalBreakpointInfoSegment*)bpInfoSegmentRX;
 
     // This must be non-zero so that the saved opcode is non-zero, and on IA64 we want it to be 0x16
     // so that we can have a breakpoint instruction in any slot in the bundle.
-    m_bpInfoSegment->m_breakpointInstruction[0] = 0x16;
+    bpInfoSegmentWriterHolder.GetRW()->m_breakpointInstruction[0] = 0x16;
 #if defined(TARGET_ARM)
     USHORT *bp = (USHORT*)&m_bpInfoSegment->m_breakpointInstruction;
     *bp = CORDbg_BREAK_INSTRUCTION;

diff --git a/src/coreclr/debug/ee/debugger.h b/src/coreclr/debug/ee/debugger.h
@@ -1104,11 +1104,8 @@ struct DECLSPEC_ALIGN(4096) DebuggerHeapExecutableMemoryPage
 
     inline void SetNextPage(DebuggerHeapExecutableMemoryPage* nextPage)
     {
-#if defined(HOST_OSX) && defined(HOST_ARM64)
-        auto jitWriteEnableHolder = PAL_JITWriteEnable(true);
-#endif // defined(HOST_OSX) && defined(HOST_ARM64)
-
-        chunks[0].bookkeeping.nextPage = nextPage;
+        ExecutableWriterHolder<DebuggerHeapExecutableMemoryPage> debuggerHeapPageWriterHolder(this, sizeof(DebuggerHeapExecutableMemoryPage));
+        debuggerHeapPageWriterHolder.GetRW()->chunks[0].bookkeeping.nextPage = nextPage;
     }
 
     inline uint64_t GetPageOccupancy() const
@@ -1118,14 +1115,11 @@ struct DECLSPEC_ALIGN(4096) DebuggerHeapExecutableMemoryPage
 
     inline void SetPageOccupancy(uint64_t newOccupancy)
     {
-#if defined(HOST_OSX) && defined(HOST_ARM64)
-        auto jitWriteEnableHolder = PAL_JITWriteEnable(true);
-#endif // defined(HOST_OSX) && defined(HOST_ARM64)
-
         // Can't unset first bit of occupancy!
         ASSERT((newOccupancy & 0x8000000000000000) != 0);
 
-        chunks[0].bookkeeping.pageOccupancy = newOccupancy;
+        ExecutableWriterHolder<DebuggerHeapExecutableMemoryPage> debuggerHeapPageWriterHolder(this, sizeof(DebuggerHeapExecutableMemoryPage));
+        debuggerHeapPageWriterHolder.GetRW()->chunks[0].bookkeeping.pageOccupancy = newOccupancy;
     }
 
     inline void* GetPointerToChunk(int chunkNum) const
@@ -1135,16 +1129,14 @@ struct DECLSPEC_ALIGN(4096) DebuggerHeapExecutableMemoryPage
 
     DebuggerHeapExecutableMemoryPage()
     {
-#if defined(HOST_OSX) && defined(HOST_ARM64)
-        auto jitWriteEnableHolder = PAL_JITWriteEnable(true);
-#endif // defined(HOST_OSX) && defined(HOST_ARM64)
+        ExecutableWriterHolder<DebuggerHeapExecutableMemoryPage> debuggerHeapPageWriterHolder(this, sizeof(DebuggerHeapExecutableMemoryPage));
 
         SetPageOccupancy(0x8000000000000000); // only the first bit is set.
         for (uint8_t i = 1; i < sizeof(chunks)/sizeof(chunks[0]); i++)
         {
             ASSERT(i != 0);
-            chunks[i].data.startOfPage = this;
-            chunks[i].data.chunkNumber = i;
+            debuggerHeapPageWriterHolder.GetRW()->chunks[i].data.startOfPage = this;
+            debuggerHeapPageWriterHolder.GetRW()->chunks[i].data.chunkNumber = i;
         }
     }
 
@@ -3486,9 +3478,6 @@ class DebuggerEval
 class InteropSafe {};
 extern InteropSafe interopsafe;
 
-class InteropSafeExecutable {};
-extern InteropSafeExecutable interopsafeEXEC;
-
 #ifndef DACCESS_COMPILE
 inline void * __cdecl operator new(size_t n, const InteropSafe&)
 {
@@ -3631,62 +3620,6 @@ template<class T> void DeleteInteropSafe(T *p)
     }
 }
 
-inline void * __cdecl operator new(size_t n, const InteropSafeExecutable&)
-{
-    CONTRACTL
-    {
-        THROWS; // throw on OOM
-        GC_NOTRIGGER;
-    }
-    CONTRACTL_END;
-
-    _ASSERTE(g_pDebugger != NULL);
-    void *result = g_pDebugger->GetInteropSafeExecutableHeap()->Alloc((DWORD)n);
-    if (result == NULL) {
-        ThrowOutOfMemory();
-    }
-    return result;
-}
-
-inline void * __cdecl operator new(size_t n, const InteropSafeExecutable&, const NoThrow&) throw()
-{
-    CONTRACTL
-    {
-        NOTHROW;
-        GC_NOTRIGGER;
-    }
-    CONTRACTL_END;
-
-    _ASSERTE(g_pDebugger != NULL);
-    DebuggerHeap * pHeap = g_pDebugger->GetInteropSafeExecutableHeap_NoThrow();
-    if (pHeap == NULL)
-    {
-        return NULL;
-    }
-    void *result = pHeap->Alloc((DWORD)n);
-    return result;
-}
-
-// Note: there is no C++ syntax for manually invoking this, but if a constructor throws an exception I understand that
-// this delete operator will be invoked automatically to destroy the object.
-inline void __cdecl operator delete(void *p, const InteropSafeExecutable&)
-{
-    CONTRACTL
-    {
-        NOTHROW;
-        GC_NOTRIGGER;
-    }
-    CONTRACTL_END;
-
-    if (p != NULL)
-    {
-        _ASSERTE(g_pDebugger != NULL);
-        DebuggerHeap * pHeap = g_pDebugger->GetInteropSafeExecutableHeap_NoThrow();
-        _ASSERTE(pHeap != NULL); // should have had heap around if we're deleting
-        pHeap->Free(p);
-    }
-}
-
 //
 // Interop safe delete to match the interop safe new's above. There is no C++ syntax for actually invoking those interop
 // safe delete operators above, so we use this method to accomplish the same thing.

diff --git a/src/coreclr/debug/inc/amd64/primitives.h b/src/coreclr/debug/inc/amd64/primitives.h
@@ -12,6 +12,8 @@
 #ifndef PRIMITIVES_H_
 #define PRIMITIVES_H_
 
+#include "executableallocator.h"
+
 #ifndef CORDB_ADDRESS_TYPE
 typedef const BYTE                  CORDB_ADDRESS_TYPE;
 typedef DPTR(CORDB_ADDRESS_TYPE)    PTR_CORDB_ADDRESS_TYPE;
@@ -187,7 +189,9 @@ inline void CORDbgInsertBreakpoint(UNALIGNED CORDB_ADDRESS_TYPE *address)
 {
     LIMITED_METHOD_CONTRACT;
 
-    *((unsigned char*)address) = 0xCC;  // int 3 (single byte patch)
+    ExecutableWriterHolder<void> breakpointWriterHolder((LPVOID)address, CORDbg_BREAK_INSTRUCTION_SIZE);
+
+    *((unsigned char*)breakpointWriterHolder.GetRW()) = 0xCC; // int 3 (single byte patch)
     FlushInstructionCache(GetCurrentProcess(), address, 1);
 
 }
@@ -198,7 +202,9 @@ inline void CORDbgSetInstruction(UNALIGNED CORDB_ADDRESS_TYPE* address,
     // In a DAC build, this function assumes the input is an host address.
     LIMITED_METHOD_DAC_CONTRACT;
 
-    *((unsigned char*)address) =
+    ExecutableWriterHolder<void> instructionWriterHolder((LPVOID)address, sizeof(unsigned char));
+
+    *((unsigned char*)instructionWriterHolder.GetRW()) =
         (unsigned char) instruction;    // setting one byte is important
     FlushInstructionCache(GetCurrentProcess(), address, 1);
 

diff --git a/src/coreclr/debug/inc/arm/primitives.h b/src/coreclr/debug/inc/arm/primitives.h
@@ -12,6 +12,8 @@
 #ifndef PRIMITIVES_H_
 #define PRIMITIVES_H_
 
+#include "executableallocator.h"
+
 #ifndef THUMB_CODE
 #define THUMB_CODE 1
 #endif
@@ -159,7 +161,9 @@ inline void CORDbgSetInstruction(CORDB_ADDRESS_TYPE* address,
     // In a DAC build, this function assumes the input is an host address.
     LIMITED_METHOD_DAC_CONTRACT;
 
-    CORDB_ADDRESS ptraddr = (CORDB_ADDRESS)address;
+    ExecutableWriterHolder<void> instructionWriterHolder((LPVOID)address, sizeof(PRD_TYPE));
+
+    CORDB_ADDRESS ptraddr = (CORDB_ADDRESS)instructionWriterHolder.GetRW();
     _ASSERTE(ptraddr & THUMB_CODE);
     ptraddr &= ~THUMB_CODE;
 

diff --git a/src/coreclr/debug/inc/arm64/primitives.h b/src/coreclr/debug/inc/arm64/primitives.h
@@ -12,6 +12,8 @@
 #ifndef PRIMITIVES_H_
 #define PRIMITIVES_H_
 
+#include "executableallocator.h"
+
 typedef NEON128                     FPRegister64;
 typedef const BYTE                  CORDB_ADDRESS_TYPE;
 typedef DPTR(CORDB_ADDRESS_TYPE)    PTR_CORDB_ADDRESS_TYPE;
@@ -146,7 +148,9 @@ inline void CORDbgSetInstruction(CORDB_ADDRESS_TYPE* address,
     // In a DAC build, this function assumes the input is an host address.
     LIMITED_METHOD_DAC_CONTRACT;
 
-    ULONGLONG ptraddr = dac_cast<ULONGLONG>(address);
+    ExecutableWriterHolder<void> instructionWriterHolder((LPVOID)address, sizeof(PRD_TYPE));
+
+    ULONGLONG ptraddr = dac_cast<ULONGLONG>(instructionWriterHolder.GetRW());
     *(PRD_TYPE *)ptraddr = instruction;
     FlushInstructionCache(GetCurrentProcess(),
                           address,

diff --git a/src/coreclr/debug/inc/i386/primitives.h b/src/coreclr/debug/inc/i386/primitives.h
@@ -12,6 +12,7 @@
 #ifndef PRIMITIVES_H_
 #define PRIMITIVES_H_
 
+#include "executableallocator.h"
 
 typedef const BYTE                  CORDB_ADDRESS_TYPE;
 typedef DPTR(CORDB_ADDRESS_TYPE)    PTR_CORDB_ADDRESS_TYPE;
@@ -148,7 +149,9 @@ inline void CORDbgInsertBreakpoint(UNALIGNED CORDB_ADDRESS_TYPE *address)
 {
     LIMITED_METHOD_CONTRACT;
 
-    *((unsigned char*)address) = 0xCC; // int 3 (single byte patch)
+    ExecutableWriterHolder<void> breakpointWriterHolder((LPVOID)address, CORDbg_BREAK_INSTRUCTION_SIZE);
+
+    *((unsigned char*)breakpointWriterHolder.GetRW()) = 0xCC; // int 3 (single byte patch)
     FlushInstructionCache(GetCurrentProcess(), address, 1);
 }
 

diff --git a/src/coreclr/dlls/mscordac/mscordac_unixexports.src b/src/coreclr/dlls/mscordac/mscordac_unixexports.src
@@ -70,6 +70,7 @@ nativeStringResourceTable_mscorrc
 #PAL__open
 #PAL__pread
 #PAL__close
+#PAL_JitWriteProtect
 
 #_wcsicmp
 #_stricmp

diff --git a/src/coreclr/inc/executableallocator.h b/src/coreclr/inc/executableallocator.h
@@ -30,7 +30,13 @@ class ExecutableWriterHolder
 
     void Unmap()
     {
-        // TODO: this will be added with the double mapped allocator addition 
+        if (m_addressRX != NULL)
+        {
+            // TODO: mapping / unmapping for targets using double memory mapping  will be added with the double mapped allocator addition 
+#if defined(HOST_OSX) && defined(HOST_ARM64)
+            PAL_JitWriteProtect(false);
+#endif
+        }
     }
 
 public:
@@ -57,6 +63,9 @@ class ExecutableWriterHolder
     {
         m_addressRX = addressRX;
         m_addressRW = addressRX;
+#if defined(HOST_OSX) && defined(HOST_ARM64)
+        PAL_JitWriteProtect(true);
+#endif
     }
 
     ~ExecutableWriterHolder()

diff --git a/src/coreclr/inc/loaderheap.h b/src/coreclr/inc/loaderheap.h
@@ -554,10 +554,6 @@ class LoaderHeap : public UnlockedLoaderHeap, public ILoaderHeapBackout
     {
         WRAPPER_NO_CONTRACT;
 
-#if defined(HOST_OSX) && defined(HOST_ARM64)
-        auto jitWriteEnableHolder = PAL_JITWriteEnable(true);
-#endif // defined(HOST_OSX) && defined(HOST_ARM64)
-
         void *pResult;
         TaggedMemAllocPtr tmap;
 
@@ -634,10 +630,6 @@ class LoaderHeap : public UnlockedLoaderHeap, public ILoaderHeapBackout
     {
         WRAPPER_NO_CONTRACT;
 
-#if defined(HOST_OSX) && defined(HOST_ARM64)
-        auto jitWriteEnableHolder = PAL_JITWriteEnable(true);
-#endif // defined(HOST_OSX) && defined(HOST_ARM64)
-
         CRITSEC_Holder csh(m_CriticalSection);