From 80ca504d085b4ff935fee8b011a2136cc75160fe Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Thu, 18 Nov 2021 14:26:58 +0100 Subject: [PATCH] FileStatus.Unix/Process.Unix: align caching of user identity. (#60160) * FileStatus.Unix/Process.Unix: align implementation. Process: remove the user identity caching and extend the logic to avoid retrieving the identity in most cases by checking if all x-bits are set or not set. FileStatus: use same group check as Process. FileStatus: cache the read only flag instead of caching the identity. --- .../Unix/System.Native/Interop.GetEGid.cs | 14 --- .../Unix/System.Native/Interop.GetUid.cs | 13 --- ...etGroups.cs => Interop.IsMemberOfGroup.cs} | 16 +++- .../Native/Unix/System.Native/entrypoints.c | 1 - .../Native/Unix/System.Native/pal_uid.c | 5 - .../Native/Unix/System.Native/pal_uid.h | 8 -- .../src/System.Diagnostics.Process.csproj | 8 +- .../src/System/Diagnostics/Process.Unix.cs | 52 +++++----- .../System.Private.CoreLib.Shared.projitems | 10 +- .../src/System/IO/FileStatus.Unix.cs | 96 ++++++++++++------- 10 files changed, 112 insertions(+), 111 deletions(-) delete mode 100644 src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetEGid.cs delete mode 100644 src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetUid.cs rename src/libraries/Common/src/Interop/Unix/System.Native/{Interop.GetGroups.cs => Interop.IsMemberOfGroup.cs} (74%) diff --git a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetEGid.cs b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetEGid.cs deleted file mode 100644 index 6ee390bf8b35e..0000000000000 --- a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetEGid.cs +++ /dev/null @@ -1,14 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System; -using System.Runtime.InteropServices; - -internal static partial class Interop -{ - internal static partial class Sys - { - [GeneratedDllImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetEGid")] - internal static partial uint GetEGid(); - } -} diff --git a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetUid.cs b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetUid.cs deleted file mode 100644 index e82897460d9d8..0000000000000 --- a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetUid.cs +++ /dev/null @@ -1,13 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Runtime.InteropServices; - -internal static partial class Interop -{ - internal static partial class Sys - { - [GeneratedDllImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetUid")] - internal static partial uint GetUid(); - } -} diff --git a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetGroups.cs b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.IsMemberOfGroup.cs similarity index 74% rename from src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetGroups.cs rename to src/libraries/Common/src/Interop/Unix/System.Native/Interop.IsMemberOfGroup.cs index 0754fabbe6d98..c302dbe3dafa0 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetGroups.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.IsMemberOfGroup.cs @@ -8,8 +8,13 @@ internal static partial class Interop { internal static partial class Sys { - internal static unsafe uint[]? GetGroups() + internal static unsafe bool IsMemberOfGroup(uint gid) { + if (gid == GetEGid()) + { + return true; + } + const int InitialGroupsLength = #if DEBUG 1; @@ -28,7 +33,7 @@ internal static partial class Sys if (rv >= 0) { // success - return groups.Slice(0, rv).ToArray(); + return groups.Slice(0, rv).IndexOf(gid) >= 0; } else if (rv == -1 && Interop.Sys.GetLastError() == Interop.Error.EINVAL) { @@ -37,13 +42,16 @@ internal static partial class Sys } else { - // failure - return null; + // failure (unexpected) + return false; } } while (true); } + [GeneratedDllImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetEGid")] + private static partial uint GetEGid(); + [GeneratedDllImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetGroups", SetLastError = true)] private static unsafe partial int GetGroups(int ngroups, uint* groups); } diff --git a/src/libraries/Native/Unix/System.Native/entrypoints.c b/src/libraries/Native/Unix/System.Native/entrypoints.c index 2440e3fcd41a3..6175dec7fbad0 100644 --- a/src/libraries/Native/Unix/System.Native/entrypoints.c +++ b/src/libraries/Native/Unix/System.Native/entrypoints.c @@ -244,7 +244,6 @@ static const Entry s_sysNative[] = DllImportEntry(SystemNative_GetEGid) DllImportEntry(SystemNative_SetEUid) DllImportEntry(SystemNative_GetGroupList) - DllImportEntry(SystemNative_GetUid) DllImportEntry(SystemNative_CreateAutoreleasePool) DllImportEntry(SystemNative_DrainAutoreleasePool) DllImportEntry(SystemNative_iOSSupportVersion) diff --git a/src/libraries/Native/Unix/System.Native/pal_uid.c b/src/libraries/Native/Unix/System.Native/pal_uid.c index 6571fb1e600cb..e1f64f03d5019 100644 --- a/src/libraries/Native/Unix/System.Native/pal_uid.c +++ b/src/libraries/Native/Unix/System.Native/pal_uid.c @@ -102,11 +102,6 @@ int32_t SystemNative_SetEUid(uint32_t euid) return seteuid(euid); } -uint32_t SystemNative_GetUid() -{ - return getuid(); -} - #ifdef USE_GROUPLIST_LOCK static pthread_mutex_t s_groupLock = PTHREAD_MUTEX_INITIALIZER; #endif diff --git a/src/libraries/Native/Unix/System.Native/pal_uid.h b/src/libraries/Native/Unix/System.Native/pal_uid.h index d0b16eef97a03..b9a24f4230460 100644 --- a/src/libraries/Native/Unix/System.Native/pal_uid.h +++ b/src/libraries/Native/Unix/System.Native/pal_uid.h @@ -74,14 +74,6 @@ PALEXPORT int32_t SystemNative_SetEUid(uint32_t euid); */ PALEXPORT int32_t SystemNative_GetGroupList(const char* name, uint32_t group, uint32_t* groups, int32_t* ngroups); -/** -* Gets and returns the real user's identity. -* Implemented as shim to getuid(2). -* -* Always succeeds. -*/ -PALEXPORT uint32_t SystemNative_GetUid(void); - /** * Gets groups associated with current process. * diff --git a/src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj b/src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj index c7e947ec99cc6..1cb5b551f599d 100644 --- a/src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj +++ b/src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj @@ -251,8 +251,6 @@ Link="Common\Interop\Unix\Interop.GetSetPriority.cs" /> - - - + = 0)) + bool groupCanExecute = (permissions & Interop.Sys.Permissions.S_IXGRP) != 0; + bool otherCanExecute = (permissions & Interop.Sys.Permissions.S_IXOTH) != 0; + + // Avoid group check when group and other have same permissions. + if (groupCanExecute == otherCanExecute) { - // A group we're a member of owns the file. - return (permissions & Interop.Sys.Permissions.S_IXGRP) != 0; + return groupCanExecute; } - // Other. - return (permissions & Interop.Sys.Permissions.S_IXOTH) != 0; + if (Interop.Sys.IsMemberOfGroup(fileinfo.Gid)) + { + return groupCanExecute; + } + else + { + return otherCanExecute; + } } private static long s_ticksPerSecond; @@ -1063,14 +1079,6 @@ private static unsafe void EnsureInitialized() throw new Win32Exception(); } - s_euid = Interop.Sys.GetEUid(); - s_egid = Interop.Sys.GetEGid(); - s_groups = Interop.Sys.GetGroups(); - if (s_groups != null) - { - Array.Sort(s_groups); - } - // Register our callback. Interop.Sys.RegisterForSigChld(&OnSigChild); SetDelayedSigChildConsoleConfigurationHandler(); diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index 2d07a361eefba..01fd0776225fa 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -1969,9 +1969,6 @@ Common\Interop\Unix\System.Native\Interop.GetDefaultTimeZone.Android.cs - - Common\Interop\Unix\System.Native\Interop.GetEGid.cs - Common\Interop\Unix\System.Native\Interop.GetHostName.cs @@ -2153,9 +2150,10 @@ - - Common\Interop\Unix\System.Native\Interop.GetEUid.cs - + + Common\Interop\Unix\System.Native\Interop.GetPwUid.cs diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.Unix.cs index 5a85ac4f3c29f..05b4bbcff6a2a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.Unix.cs @@ -30,13 +30,6 @@ internal partial struct FileStatus // Exists as of the last refresh private bool _exists; -#if !TARGET_BROWSER - // Caching the euid/egid to avoid extra p/invokes - // Should get reset on refresh - private uint? _euid; - private uint? _egid; -#endif - private bool IsFileCacheInitialized => _initializedFileCache == 0; // Check if the main path (without following symlinks) has the hidden attribute set @@ -58,36 +51,76 @@ internal bool HasReadOnlyFlag { return false; } + #if TARGET_BROWSER - const Interop.Sys.Permissions readBit = Interop.Sys.Permissions.S_IRUSR; - const Interop.Sys.Permissions writeBit = Interop.Sys.Permissions.S_IWUSR; + var mode = (Interop.Sys.Permissions)(_fileCache.Mode & (int)Interop.Sys.Permissions.Mask); + bool isUserReadOnly = (mode & Interop.Sys.Permissions.S_IRUSR) != 0 && // has read permission + (mode & Interop.Sys.Permissions.S_IWUSR) == 0; // but not write permission + return isUserReadOnly; #else - Interop.Sys.Permissions readBit, writeBit; - - if (_fileCache.Uid == (_euid ??= Interop.Sys.GetEUid())) + if (_isReadOnlyCache == 0) { - // User effectively owns the file - readBit = Interop.Sys.Permissions.S_IRUSR; - writeBit = Interop.Sys.Permissions.S_IWUSR; + return false; } - else if (_fileCache.Gid == (_egid ??= Interop.Sys.GetEGid())) + else if (_isReadOnlyCache == 1) { - // User belongs to a group that effectively owns the file - readBit = Interop.Sys.Permissions.S_IRGRP; - writeBit = Interop.Sys.Permissions.S_IWGRP; + return true; } else { - // Others permissions - readBit = Interop.Sys.Permissions.S_IROTH; - writeBit = Interop.Sys.Permissions.S_IWOTH; + bool isReadOnly = IsModeReadOnlyCore(); + _isReadOnlyCache = isReadOnly ? 1 : 0; + return isReadOnly; } #endif + } + } + +#if !TARGET_BROWSER + // HasReadOnlyFlag cache. + private int _isReadOnlyCache; + + private bool IsModeReadOnlyCore() + { + var mode = (Interop.Sys.Permissions)(_fileCache.Mode & (int)Interop.Sys.Permissions.Mask); + + bool isUserReadOnly = (mode & Interop.Sys.Permissions.S_IRUSR) != 0 && // has read permission + (mode & Interop.Sys.Permissions.S_IWUSR) == 0; // but not write permission + bool isGroupReadOnly = (mode & Interop.Sys.Permissions.S_IRGRP) != 0 && // has read permission + (mode & Interop.Sys.Permissions.S_IWGRP) == 0; // but not write permission + bool isOtherReadOnly = (mode & Interop.Sys.Permissions.S_IROTH) != 0 && // has read permission + (mode & Interop.Sys.Permissions.S_IWOTH) == 0; // but not write permission + + // If they are all the same, no need to check user/group. + if ((isUserReadOnly == isGroupReadOnly) && (isGroupReadOnly == isOtherReadOnly)) + { + return isUserReadOnly; + } + + if (_fileCache.Uid == Interop.Sys.GetEUid()) + { + // User owns the file. + return isUserReadOnly; + } + + // System files often have the same permissions for group and other (umask 022). + if (isGroupReadOnly == isUserReadOnly) + { + return isGroupReadOnly; + } - return (_fileCache.Mode & (int)readBit) != 0 && // has read permission - (_fileCache.Mode & (int)writeBit) == 0; // but not write permission + if (Interop.Sys.IsMemberOfGroup(_fileCache.Gid)) + { + // User belongs to group that owns the file. + return isGroupReadOnly; + } + else + { + // Other permissions. + return isOtherReadOnly; } } +#endif // Checks if the main path is a symbolic link // Only call if Refresh has been successfully called at least once @@ -376,12 +409,6 @@ internal void RefreshCaches(ReadOnlySpan path) _isDirectory = CacheHasDirectoryFlag(target); } -#if !TARGET_BROWSER - // These are cached and used when retrieving the read-only attribute - _euid = null; - _egid = null; -#endif - _exists = true; // Checks if the specified cache has the directory attribute set @@ -425,8 +452,13 @@ private static long UnixTimeSecondsToNanoseconds(DateTimeOffset time, long secon return (time.UtcDateTime.Ticks - DateTimeOffset.UnixEpoch.Ticks - seconds * TicksPerSecond) * NanosecondsPerTick; } - private bool TryRefreshFileCache(ReadOnlySpan path) => - VerifyStatCall(Interop.Sys.LStat(path, out _fileCache), out _initializedFileCache); + private bool TryRefreshFileCache(ReadOnlySpan path) + { +#if !TARGET_BROWSER + _isReadOnlyCache = -1; +#endif + return VerifyStatCall(Interop.Sys.LStat(path, out _fileCache), out _initializedFileCache); + } // Receives the return value of a stat or lstat call. // If the call is unsuccessful, sets the initialized parameter to a positive number representing the last error info.