Fix PAL executable allocator locking (#5770)

We call the ExecutableAllocator from two places in PAL. One is the
VirtualAlloc when the allocation type contains MEM_RESERVE_EXECUTABLE.
The other is MAPMapPEFile.
While the former is called inside of the virtual_critsec critical section,
the latter doesn't take that critical section and so in some race cases,
we were returning the same address twice - once for VirtualAlloc and
once for MAPMapPEFile. That resulted in strange memory corruption cases.
The fix is to use the same critical section for the MAPMapPEFile too.

I am also reverting the change in the virtual commit that was made when
we have thought that the culprit might be in the mprotect.

2 files changed, 7 insertions, 13 deletions

diff --git a/src/pal/src/map/map.cpp b/src/pal/src/map/map.cpp
index 3700a27eee..cab9c6cbfa 100644
--- a/src/pal/src/map/map.cpp
+++ b/src/pal/src/map/map.cpp
@@ -2445,7 +2445,7 @@ void * MAPMapPEFile(HANDLE hFile)
     // First try to reserve virtual memory using ExecutableAllcator. This allows all PE images to be
     // near each other and close to the coreclr library which also allows the runtime to generate
     // more efficient code (by avoiding usage of jump stubs).
-    loadedBase = ReserveMemoryFromExecutableAllocator(virtualSize);
+    loadedBase = ReserveMemoryFromExecutableAllocator(pThread, virtualSize);
     if (loadedBase == NULL)
     {
         // MAC64 requires we pass MAP_SHARED (or MAP_PRIVATE) flags - otherwise, the call is failed.
diff --git a/src/pal/src/map/virtual.cpp b/src/pal/src/map/virtual.cpp
index 0d7310c9e3..50d43cf03f 100644
--- a/src/pal/src/map/virtual.cpp
+++ b/src/pal/src/map/virtual.cpp
@@ -1114,17 +1114,7 @@ static LPVOID VIRTUALCommitMemory(
         if (allocationType != MEM_COMMIT)
         {
             // Commit the pages
-            void * pRet = MAP_FAILED;
-#ifndef __APPLE__
             if (mprotect((void *) StartBoundary, MemSize, PROT_WRITE | PROT_READ) == 0)
-                pRet = (void *)StartBoundary;
-#else // __APPLE__
-            // Using mprotect above on MacOS is suspect to cause intermittent crashes
-            // https://github.com/dotnet/coreclr/issues/5672
-            pRet = mmap((void *) StartBoundary, MemSize, PROT_WRITE | PROT_READ,
-                     MAP_ANON | MAP_FIXED | MAP_PRIVATE, -1, 0);
-#endif // __APPLE__
-            if (pRet != MAP_FAILED)
             {
 #if MMAP_DOESNOT_ALLOW_REMAP
                 SIZE_T i;
@@ -1934,9 +1924,13 @@ Function :
     that is located close to the coreclr library. The memory comes from the virtual
     address range that is managed by ExecutableMemoryAllocator.
 --*/
-void* ReserveMemoryFromExecutableAllocator(SIZE_T allocationSize)
+void* ReserveMemoryFromExecutableAllocator(CPalThread* pThread, SIZE_T allocationSize)
 {
-    return g_executableMemoryAllocator.AllocateMemory(allocationSize);
+    InternalEnterCriticalSection(pThread, &virtual_critsec);
+    void* mem = g_executableMemoryAllocator.AllocateMemory(allocationSize);
+    InternalLeaveCriticalSection(pThread, &virtual_critsec);
+
+    return mem;
 }
 
 /*++