From 3f66d42a1136eec81f3ea30e0fa5bdf26d99826a Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 13 Jun 2016 23:37:19 +0200 Subject: [PATCH] Port to release/1.0.0: Fix PAL executable allocator locking 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. --- src/pal/src/include/pal/virtual.h | 2 +- src/pal/src/map/map.cpp | 2 +- src/pal/src/map/virtual.cpp | 18 ++++++------------ 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/src/pal/src/include/pal/virtual.h b/src/pal/src/include/pal/virtual.h index ac1072f6afd0..b74076783a20 100644 --- 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 3700a27eeea7..cab9c6cbfa3b 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 150160861d0d..22fea5312d0d 100644 --- a/src/pal/src/map/virtual.cpp +++ b/src/pal/src/map/virtual.cpp @@ -1197,17 +1197,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; @@ -2362,9 +2352,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; } /*++