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.

diff --git a/src/pal/src/include/pal/virtual.h b/src/pal/src/include/pal/virtual.h
index ac1072f6afd07224ab5d21331f99e64a57d88772..b74076783a20ded969f82f6e3d4e9d63b7f95625 100644 (file)
--- a/src/pal/src/include/pal/virtual.h
+++ b/src/pal/src/include/pal/virtual.h
@@ -209,7 +209,7 @@ 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(CorUnix::CPalThread* pthrCurrent, SIZE_T allocationSize);
 
 #endif /* _PAL_VIRTUAL_H_ */
 

diff --git a/src/pal/src/map/map.cpp b/src/pal/src/map/map.cpp
index 3700a27eeea7a738183b40ab8269b66923ab6704..cab9c6cbfa3bf8b21d9f51ccdf140a6d644ba7e5 100644 (file)
--- 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 0d7310c9e3d04fd4a686c5df4cb7f89ca1bfb005..50d43cf03fb1ee168393955cb4c0dc4dd3224da8 100644 (file)
--- 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;
 }
 
 /*++