Fix shm directory creation for named mutexes

- Creating a directory involves mkdir and chmod. Due to this, there could be a race where one process running as one user creates the directory but does not update permissions for all uses to have access, and at that point another process tries to use the directory.
- Fixed by creating a temp directory using mkdtemp(), chmod to set appropriate permissions for all users, and rename the directory. This is only done when a global lock is not held during the directory creation operation (applies only to /tmp/.dotnet and /tmp/.dotnet/shm).
- Tested manually by adding sleeps at the race points with a second process running as a different user, to make sure it works in any order.

diff --git a/src/pal/src/include/pal/sharedmemory.h b/src/pal/src/include/pal/sharedmemory.h
index 046eb63..475cd70 100644 (file)
--- a/src/pal/src/include/pal/sharedmemory.h
+++ b/src/pal/src/include/pal/sharedmemory.h
@@ -38,6 +38,8 @@ static_assert_no_msg(_countof(SHARED_MEMORY_LOCK_FILES_DIRECTORY_PATH) >= _count
 #define SHARED_MEMORY_SESSION_DIRECTORY_NAME_PREFIX "session"
 static_assert_no_msg(_countof(SHARED_MEMORY_SESSION_DIRECTORY_NAME_PREFIX) >= _countof(SHARED_MEMORY_GLOBAL_DIRECTORY_NAME));
 
+#define SHARED_MEMORY_UNIQUE_TEMP_NAME_TEMPLATE "/tmp/.coreclr.XXXXXX"
+
 #define SHARED_MEMORY_MAX_SESSION_ID_CHAR_COUNT (10)
 
 #define SHARED_MEMORY_MAX_FILE_PATH_CHAR_COUNT \
@@ -103,8 +105,9 @@ public:
 
     template<SIZE_T DestinationByteCount, SIZE_T SourceByteCount> static SIZE_T CopyString(char (&destination)[DestinationByteCount], SIZE_T destinationStartOffset, const char (&source)[SourceByteCount]);
     template<SIZE_T DestinationByteCount> static SIZE_T CopyString(char (&destination)[DestinationByteCount], SIZE_T destinationStartOffset, LPCSTR source, SIZE_T sourceCharCount);
+    template<SIZE_T DestinationByteCount> static SIZE_T AppendUInt32String(char (&destination)[DestinationByteCount], SIZE_T destinationStartOffset, UINT32 value);
 
-    static bool EnsureDirectoryExists(const char *path, bool createIfNotExist = true);
+    static bool EnsureDirectoryExists(const char *path, bool isGlobalLockAcquired, bool createIfNotExist = true);
 private:
     static int Open(LPCSTR path, int flags, mode_t mode = static_cast<mode_t>(0));
 public:

diff --git a/src/pal/src/include/pal/sharedmemory.inl b/src/pal/src/include/pal/sharedmemory.inl
index 240da95..69b8704 100644 (file)
--- a/src/pal/src/include/pal/sharedmemory.inl
+++ b/src/pal/src/include/pal/sharedmemory.inl
@@ -9,9 +9,11 @@
 
 #include "dbgmsg.h"
 
+#include <string.h>
+
 template<SIZE_T DestinationByteCount, SIZE_T SourceByteCount>
 SIZE_T SharedMemoryHelpers::CopyString(
-    char(&destination)[DestinationByteCount],
+    char (&destination)[DestinationByteCount],
     SIZE_T destinationStartOffset,
     const char(&source)[SourceByteCount])
 {
@@ -20,16 +22,32 @@ SIZE_T SharedMemoryHelpers::CopyString(
 
 template<SIZE_T DestinationByteCount>
 SIZE_T SharedMemoryHelpers::CopyString(
-    char(&destination)[DestinationByteCount],
+    char (&destination)[DestinationByteCount],
     SIZE_T destinationStartOffset,
     LPCSTR source,
     SIZE_T sourceCharCount)
 {
-    _ASSERTE(destinationStartOffset <= DestinationByteCount);
+    _ASSERTE(destinationStartOffset < DestinationByteCount);
     _ASSERTE(sourceCharCount < DestinationByteCount - destinationStartOffset);
+    _ASSERTE(strlen(source) == sourceCharCount);
 
     memcpy_s(&destination[destinationStartOffset], DestinationByteCount - destinationStartOffset, source, sourceCharCount + 1);
     return destinationStartOffset + sourceCharCount;
 }
 
+template<SIZE_T DestinationByteCount>
+SIZE_T SharedMemoryHelpers::AppendUInt32String(
+    char (&destination)[DestinationByteCount],
+    SIZE_T destinationStartOffset,
+    UINT32 value)
+{
+    _ASSERTE(destination != nullptr);
+    _ASSERTE(destinationStartOffset < DestinationByteCount);
+
+    int valueCharCount =
+        sprintf_s(&destination[destinationStartOffset], DestinationByteCount - destinationStartOffset, "%u", value);
+    _ASSERTE(valueCharCount > 0);
+    return destinationStartOffset + valueCharCount;
+}
+
 #endif // !_PAL_SHARED_MEMORY_INL_

diff --git a/src/pal/src/sharedmemory/sharedmemory.cpp b/src/pal/src/sharedmemory/sharedmemory.cpp
index 2c5ef9b..6e740ba 100644 (file)
--- a/src/pal/src/sharedmemory/sharedmemory.cpp
+++ b/src/pal/src/sharedmemory/sharedmemory.cpp
@@ -16,6 +16,8 @@
 #include <sys/types.h>
 
 #include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
 
@@ -90,8 +92,11 @@ SIZE_T SharedMemoryHelpers::AlignUp(SIZE_T value, SIZE_T alignment)
     return AlignDown(value + (alignment - 1), alignment);
 }
 
-bool SharedMemoryHelpers::EnsureDirectoryExists(const char *path, bool createIfNotExist)
+bool SharedMemoryHelpers::EnsureDirectoryExists(const char *path, bool isGlobalLockAcquired, bool createIfNotExist)
 {
+    _ASSERTE(path != nullptr);
+    _ASSERTE(SharedMemoryManager::IsCreationDeletionProcessLockAcquired());
+
     // Check if the path already exists
     struct stat statInfo;
     int statResult = stat(path, &statInfo);
@@ -102,27 +107,49 @@ bool SharedMemoryHelpers::EnsureDirectoryExists(const char *path, bool createIfN
             return false;
         }
 
-        // The path does not exist, create the directory
-        if (mkdir(path, PermissionsMask_AllUsers_ReadWriteExecute) == 0)
+        // The path does not exist, create the directory. The permissions mask passed to mkdir() is filtered by the process'
+        // permissions umask, so mkdir() may not set all of the requested permissions. We need to use chmod() to set the proper
+        // permissions. That creates a race when there is no global lock acquired when creating the directory. Another user's
+        // process may create the directory and this user's process may try to use it before the other process sets the full
+        // permissions. In that case, create a temporary directory first, set the permissions, and rename it to the actual
+        // directory name.
+
+        if (isGlobalLockAcquired)
         {
-            // The permissions mask passed to mkdir() is filtered by the process' permissions umask, so mkdir() may not set all
-            // of the requested permissions. Use chmod() to set the proper permissions.
-            if (chmod(path, PermissionsMask_AllUsers_ReadWriteExecute) == 0)
+            if (mkdir(path, PermissionsMask_AllUsers_ReadWriteExecute) != 0)
             {
-                return true;
+                throw SharedMemoryException(static_cast<DWORD>(SharedMemoryError::IO));
             }
-            rmdir(path);
+            if (chmod(path, PermissionsMask_AllUsers_ReadWriteExecute) != 0)
+            {
+                rmdir(path);
+                throw SharedMemoryException(static_cast<DWORD>(SharedMemoryError::IO));
+            }
+            return true;
+        }
+
+        char tempPath[] = SHARED_MEMORY_UNIQUE_TEMP_NAME_TEMPLATE;
+        if (mkdtemp(tempPath) == nullptr)
+        {
             throw SharedMemoryException(static_cast<DWORD>(SharedMemoryError::IO));
         }
-        if (errno != EEXIST)
+        if (chmod(tempPath, PermissionsMask_AllUsers_ReadWriteExecute) != 0)
         {
+            rmdir(tempPath);
             throw SharedMemoryException(static_cast<DWORD>(SharedMemoryError::IO));
         }
+        if (rename(tempPath, path) == 0)
+        {
+            return true;
+        }
 
+        // Another process may have beaten us to it. Delete the temp directory and continue to check the requested directory to
+        // see if it meets our needs.
+        rmdir(tempPath);
         statResult = stat(path, &statInfo);
     }
 
-    // The path exists, check that it's a directory
+    // If the path exists, check that it's a directory
     if (statResult != 0 || !(statInfo.st_mode & S_IFDIR))
     {
         throw SharedMemoryException(static_cast<DWORD>(SharedMemoryError::IO));
@@ -425,14 +452,7 @@ SIZE_T SharedMemoryId::AppendSessionDirectoryName(
     if (IsSessionScope())
     {
         pathCharCount = SharedMemoryHelpers::CopyString(path, pathCharCount, SHARED_MEMORY_SESSION_DIRECTORY_NAME_PREFIX);
-        int sessionIdCharCount =
-            sprintf_s(
-                &path[pathCharCount],
-                _countof(path) - pathCharCount,
-                "%u",
-                GetCurrentSessionId());
-        _ASSERTE(sessionIdCharCount > 0);
-        pathCharCount += sessionIdCharCount;
+        pathCharCount = SharedMemoryHelpers::AppendUInt32String(path, pathCharCount, GetCurrentSessionId());
     }
     else
     {
@@ -581,7 +601,7 @@ SharedMemoryProcessDataHeader *SharedMemoryProcessDataHeader::CreateOrOpen(
     SIZE_T filePathCharCount = SharedMemoryHelpers::CopyString(filePath, 0, SHARED_MEMORY_SHARED_MEMORY_DIRECTORY_PATH);
     filePath[filePathCharCount++] = '/';
     filePathCharCount = id.AppendSessionDirectoryName(filePath, filePathCharCount);
-    if (!SharedMemoryHelpers::EnsureDirectoryExists(filePath, createIfNotExist))
+    if (!SharedMemoryHelpers::EnsureDirectoryExists(filePath, true /* isGlobalLockAcquired */, createIfNotExist))
     {
         _ASSERTE(!createIfNotExist);
         return nullptr;
@@ -1009,12 +1029,19 @@ void SharedMemoryManager::AcquireCreationDeletionFileLock()
 
     if (s_creationDeletionLockFileDescriptor == -1)
     {
-        if (!SharedMemoryHelpers::EnsureDirectoryExists(SHARED_MEMORY_TEMP_DIRECTORY_PATH, false /* createIfNotExist */))
+        if (!SharedMemoryHelpers::EnsureDirectoryExists(
+                SHARED_MEMORY_TEMP_DIRECTORY_PATH,
+                false /* isGlobalLockAcquired */,
+                false /* createIfNotExist */))
         {
             throw SharedMemoryException(static_cast<DWORD>(SharedMemoryError::IO));
         }
-        SharedMemoryHelpers::EnsureDirectoryExists(SHARED_MEMORY_RUNTIME_TEMP_DIRECTORY_PATH);
-        SharedMemoryHelpers::EnsureDirectoryExists(SHARED_MEMORY_SHARED_MEMORY_DIRECTORY_PATH);
+        SharedMemoryHelpers::EnsureDirectoryExists(
+            SHARED_MEMORY_RUNTIME_TEMP_DIRECTORY_PATH,
+            false /* isGlobalLockAcquired */);
+        SharedMemoryHelpers::EnsureDirectoryExists(
+            SHARED_MEMORY_SHARED_MEMORY_DIRECTORY_PATH,
+            false /* isGlobalLockAcquired */);
         s_creationDeletionLockFileDescriptor = SharedMemoryHelpers::OpenDirectory(SHARED_MEMORY_SHARED_MEMORY_DIRECTORY_PATH);
         if (s_creationDeletionLockFileDescriptor == -1)
         {

diff --git a/src/pal/src/synchobj/mutex.cpp b/src/pal/src/synchobj/mutex.cpp
index 1977bbf..c2a0be0 100644 (file)
--- a/src/pal/src/synchobj/mutex.cpp
+++ b/src/pal/src/synchobj/mutex.cpp
@@ -1146,7 +1146,7 @@ SharedMemoryProcessDataHeader *NamedMutexProcessData::CreateOrOpen(
             SharedMemoryHelpers::CopyString(lockFilePath, 0, SHARED_MEMORY_LOCK_FILES_DIRECTORY_PATH);
         if (created)
         {
-            SharedMemoryHelpers::EnsureDirectoryExists(lockFilePath);
+            SharedMemoryHelpers::EnsureDirectoryExists(lockFilePath, true /* isGlobalLockAcquired */);
         }
 
         // Create the session directory
@@ -1155,7 +1155,7 @@ SharedMemoryProcessDataHeader *NamedMutexProcessData::CreateOrOpen(
         lockFilePathCharCount = id->AppendSessionDirectoryName(lockFilePath, lockFilePathCharCount);
         if (created)
         {
-            SharedMemoryHelpers::EnsureDirectoryExists(lockFilePath);
+            SharedMemoryHelpers::EnsureDirectoryExists(lockFilePath, true /* isGlobalLockAcquired */);
             autoCleanup.m_lockFilePath = lockFilePath;
             autoCleanup.m_sessionDirectoryPathCharCount = lockFilePathCharCount;
         }