From 1c4e4c11e4ad514a6a1198ce494873365af4eace Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Fri, 11 Aug 2023 14:34:40 -0700 Subject: [PATCH] Fix session-local named mutex compat issue (#90342) - A previous change that was serviced back changed session-local named mutexes to be user-specific by restricting the permissions of the session directories and files under them, and adding the sticky bit to some directoires. A compat issue arose from that change, as the session directories have the session ID in their name and session IDs can be reused between different users. The current plan that we have discussed is to revert the change and service back the revert, which also restores the intended behavior, and offer user-specific mutexes as a new feature in a future .NET that would satisfy some user scenarios in a better way. - This PR reverts the previous change (first commit) and restores one change from the previous change (second commit) to improve backward compatibility due to differences in permissions for session directories before and after the change - Fixes https://github.com/dotnet/runtime/issues/80619 --- .../pal/src/include/pal/sharedmemory.h | 6 +-- .../pal/src/sharedmemory/sharedmemory.cpp | 41 ++++++------------- src/coreclr/pal/src/synchobj/mutex.cpp | 6 +-- 3 files changed, 17 insertions(+), 36 deletions(-) diff --git a/src/coreclr/pal/src/include/pal/sharedmemory.h b/src/coreclr/pal/src/include/pal/sharedmemory.h index cbdd816dcf2..88834b93d06 100644 --- a/src/coreclr/pal/src/include/pal/sharedmemory.h +++ b/src/coreclr/pal/src/include/pal/sharedmemory.h @@ -88,11 +88,9 @@ public: class SharedMemoryHelpers { private: - static const mode_t PermissionsMask_CurrentUser_ReadWrite; static const mode_t PermissionsMask_CurrentUser_ReadWriteExecute; static const mode_t PermissionsMask_AllUsers_ReadWrite; static const mode_t PermissionsMask_AllUsers_ReadWriteExecute; - public: static const UINT32 InvalidProcessId; static const SIZE_T InvalidThreadId; @@ -108,12 +106,12 @@ public: static void BuildSharedFilesPath(PathCharString& destination, const char *suffix, int suffixByteCount); static bool AppendUInt32String(PathCharString& destination, UINT32 value); - static bool EnsureDirectoryExists(const char *path, bool isGlobalLockAcquired, bool hasCurrentUserAccessOnly, bool setStickyFlag, bool createIfNotExist = true, bool isSystemDirectory = false); + static bool EnsureDirectoryExists(const char *path, bool isGlobalLockAcquired, bool createIfNotExist = true, bool isSystemDirectory = false); private: static int Open(LPCSTR path, int flags, mode_t mode = static_cast(0)); public: static int OpenDirectory(LPCSTR path); - static int CreateOrOpenFile(LPCSTR path, bool createIfNotExist = true, bool isSessionScope = true, bool *createdRef = nullptr); + static int CreateOrOpenFile(LPCSTR path, bool createIfNotExist = true, bool *createdRef = nullptr); static void CloseFile(int fileDescriptor); static SIZE_T GetFileSize(int fileDescriptor); diff --git a/src/coreclr/pal/src/sharedmemory/sharedmemory.cpp b/src/coreclr/pal/src/sharedmemory/sharedmemory.cpp index 12a3892c931..a2342f23efa 100644 --- a/src/coreclr/pal/src/sharedmemory/sharedmemory.cpp +++ b/src/coreclr/pal/src/sharedmemory/sharedmemory.cpp @@ -62,7 +62,6 @@ DWORD SharedMemoryException::GetErrorCode() const //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// // SharedMemoryHelpers -const mode_t SharedMemoryHelpers::PermissionsMask_CurrentUser_ReadWrite = S_IRUSR | S_IWUSR; const mode_t SharedMemoryHelpers::PermissionsMask_CurrentUser_ReadWriteExecute = S_IRUSR | S_IWUSR | S_IXUSR; const mode_t SharedMemoryHelpers::PermissionsMask_AllUsers_ReadWrite = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH; @@ -97,8 +96,6 @@ SIZE_T SharedMemoryHelpers::AlignUp(SIZE_T value, SIZE_T alignment) bool SharedMemoryHelpers::EnsureDirectoryExists( const char *path, bool isGlobalLockAcquired, - bool hasCurrentUserAccessOnly, - bool setStickyFlag, bool createIfNotExist, bool isSystemDirectory) { @@ -106,13 +103,6 @@ bool SharedMemoryHelpers::EnsureDirectoryExists( _ASSERTE(!(isSystemDirectory && createIfNotExist)); // should not create or change permissions on system directories _ASSERTE(SharedMemoryManager::IsCreationDeletionProcessLockAcquired()); _ASSERTE(!isGlobalLockAcquired || SharedMemoryManager::IsCreationDeletionFileLockAcquired()); - _ASSERTE(!(setStickyFlag && hasCurrentUserAccessOnly)); // Sticky bit doesn't make sense with current user access only - - mode_t mode = hasCurrentUserAccessOnly ? PermissionsMask_CurrentUser_ReadWriteExecute : PermissionsMask_AllUsers_ReadWriteExecute; - if (setStickyFlag) - { - mode |= S_ISVTX; - } // Check if the path already exists struct stat statInfo; @@ -133,11 +123,11 @@ bool SharedMemoryHelpers::EnsureDirectoryExists( if (isGlobalLockAcquired) { - if (mkdir(path, mode) != 0) + if (mkdir(path, PermissionsMask_AllUsers_ReadWriteExecute) != 0) { throw SharedMemoryException(static_cast(SharedMemoryError::IO)); } - if (chmod(path, mode) != 0) + if (chmod(path, PermissionsMask_AllUsers_ReadWriteExecute) != 0) { rmdir(path); throw SharedMemoryException(static_cast(SharedMemoryError::IO)); @@ -152,7 +142,7 @@ bool SharedMemoryHelpers::EnsureDirectoryExists( { throw SharedMemoryException(static_cast(SharedMemoryError::IO)); } - if (chmod(tempPath, mode) != 0) + if (chmod(tempPath, PermissionsMask_AllUsers_ReadWriteExecute) != 0) { rmdir(tempPath); throw SharedMemoryException(static_cast(SharedMemoryError::IO)); @@ -192,11 +182,11 @@ bool SharedMemoryHelpers::EnsureDirectoryExists( // For non-system directories (such as gSharedFilesPath/SHARED_MEMORY_RUNTIME_TEMP_DIRECTORY_NAME), // require sufficient permissions for all users and try to update them if requested to create the directory, so that // shared memory files may be shared by all processes on the system. - if ((statInfo.st_mode & mode) == mode) + if ((statInfo.st_mode & PermissionsMask_AllUsers_ReadWriteExecute) == PermissionsMask_AllUsers_ReadWriteExecute) { return true; } - if (!createIfNotExist || chmod(path, mode) != 0) + if (!createIfNotExist || chmod(path, PermissionsMask_AllUsers_ReadWriteExecute) != 0) { // We were not asked to create the path or we weren't able to set the new permissions. // As a last resort, check that at least the current user has full access. @@ -253,7 +243,7 @@ int SharedMemoryHelpers::OpenDirectory(LPCSTR path) return fileDescriptor; } -int SharedMemoryHelpers::CreateOrOpenFile(LPCSTR path, bool createIfNotExist, bool isSessionScope, bool *createdRef) +int SharedMemoryHelpers::CreateOrOpenFile(LPCSTR path, bool createIfNotExist, bool *createdRef) { _ASSERTE(path != nullptr); _ASSERTE(path[0] != '\0'); @@ -283,13 +273,12 @@ int SharedMemoryHelpers::CreateOrOpenFile(LPCSTR path, bool createIfNotExist, bo // File does not exist, create the file openFlags |= O_CREAT | O_EXCL; - mode_t mode = isSessionScope ? PermissionsMask_CurrentUser_ReadWrite : PermissionsMask_AllUsers_ReadWrite; - fileDescriptor = Open(path, openFlags, mode); + fileDescriptor = Open(path, openFlags, PermissionsMask_AllUsers_ReadWrite); _ASSERTE(fileDescriptor != -1); // The permissions mask passed to open() is filtered by the process' permissions umask, so open() may not set all of // the requested permissions. Use chmod() to set the proper permissions. - if (chmod(path, mode) != 0) + if (chmod(path, PermissionsMask_AllUsers_ReadWrite) != 0) { CloseFile(fileDescriptor); unlink(path); @@ -675,7 +664,7 @@ SharedMemoryProcessDataHeader *SharedMemoryProcessDataHeader::CreateOrOpen( SharedMemoryHelpers::VerifyStringOperation(SharedMemoryManager::CopySharedMemoryBasePath(filePath)); SharedMemoryHelpers::VerifyStringOperation(filePath.Append('/')); SharedMemoryHelpers::VerifyStringOperation(id.AppendSessionDirectoryName(filePath)); - if (!SharedMemoryHelpers::EnsureDirectoryExists(filePath, true /* isGlobalLockAcquired */, id.IsSessionScope(), false /* setStickyFlag */, createIfNotExist)) + if (!SharedMemoryHelpers::EnsureDirectoryExists(filePath, true /* isGlobalLockAcquired */, createIfNotExist)) { _ASSERTE(!createIfNotExist); return nullptr; @@ -688,7 +677,7 @@ SharedMemoryProcessDataHeader *SharedMemoryProcessDataHeader::CreateOrOpen( SharedMemoryHelpers::VerifyStringOperation(filePath.Append(id.GetName(), id.GetNameCharCount())); bool createdFile; - int fileDescriptor = SharedMemoryHelpers::CreateOrOpenFile(filePath, createIfNotExist, id.IsSessionScope(), &createdFile); + int fileDescriptor = SharedMemoryHelpers::CreateOrOpenFile(filePath, createIfNotExist, &createdFile); if (fileDescriptor == -1) { _ASSERTE(!createIfNotExist); @@ -1163,8 +1152,6 @@ void SharedMemoryManager::AcquireCreationDeletionFileLock() if (!SharedMemoryHelpers::EnsureDirectoryExists( *gSharedFilesPath, false /* isGlobalLockAcquired */, - false /* hasCurrentUserAccessOnly */, - true /* setStickyFlag */, false /* createIfNotExist */, true /* isSystemDirectory */)) { @@ -1172,14 +1159,10 @@ void SharedMemoryManager::AcquireCreationDeletionFileLock() } SharedMemoryHelpers::EnsureDirectoryExists( *s_runtimeTempDirectoryPath, - false /* isGlobalLockAcquired */, - false /* hasCurrentUserAccessOnly */, - false /* setStickyFlag */); + false /* isGlobalLockAcquired */); SharedMemoryHelpers::EnsureDirectoryExists( *s_sharedMemoryDirectoryPath, - false /* isGlobalLockAcquired */, - false /* hasCurrentUserAccessOnly */, - true /* setStickyFlag */); + false /* isGlobalLockAcquired */); s_creationDeletionLockFileDescriptor = SharedMemoryHelpers::OpenDirectory(*s_sharedMemoryDirectoryPath); if (s_creationDeletionLockFileDescriptor == -1) { diff --git a/src/coreclr/pal/src/synchobj/mutex.cpp b/src/coreclr/pal/src/synchobj/mutex.cpp index aeadf6f1fcf..3fff2e7917f 100644 --- a/src/coreclr/pal/src/synchobj/mutex.cpp +++ b/src/coreclr/pal/src/synchobj/mutex.cpp @@ -1115,7 +1115,7 @@ SharedMemoryProcessDataHeader *NamedMutexProcessData::CreateOrOpen( SharedMemoryHelpers::BuildSharedFilesPath(lockFilePath, SHARED_MEMORY_LOCK_FILES_DIRECTORY_NAME); if (created) { - SharedMemoryHelpers::EnsureDirectoryExists(lockFilePath, true /* isGlobalLockAcquired */, false /* hasCurrentUserAccessOnly */, true /* setStickyFlag */); + SharedMemoryHelpers::EnsureDirectoryExists(lockFilePath, true /* isGlobalLockAcquired */); } // Create the session directory @@ -1124,7 +1124,7 @@ SharedMemoryProcessDataHeader *NamedMutexProcessData::CreateOrOpen( SharedMemoryHelpers::VerifyStringOperation(id->AppendSessionDirectoryName(lockFilePath)); if (created) { - SharedMemoryHelpers::EnsureDirectoryExists(lockFilePath, true /* isGlobalLockAcquired */, id->IsSessionScope(), false /* setStickyFlag */); + SharedMemoryHelpers::EnsureDirectoryExists(lockFilePath, true /* isGlobalLockAcquired */); autoCleanup.m_lockFilePath = &lockFilePath; autoCleanup.m_sessionDirectoryPathCharCount = lockFilePath.GetCount(); } @@ -1132,7 +1132,7 @@ SharedMemoryProcessDataHeader *NamedMutexProcessData::CreateOrOpen( // Create or open the lock file SharedMemoryHelpers::VerifyStringOperation(lockFilePath.Append('/')); SharedMemoryHelpers::VerifyStringOperation(lockFilePath.Append(id->GetName(), id->GetNameCharCount())); - int lockFileDescriptor = SharedMemoryHelpers::CreateOrOpenFile(lockFilePath, created, id->IsSessionScope()); + int lockFileDescriptor = SharedMemoryHelpers::CreateOrOpenFile(lockFilePath, created); if (lockFileDescriptor == -1) { _ASSERTE(!created); -- GitLab