From fee81e80d40f479b4a2e47ffca91ebaa171f79d6 Mon Sep 17 00:00:00 2001 From: James Truher Date: Thu, 21 Sep 2023 11:26:42 -0700 Subject: [PATCH 1/9] Change the way the mode string is created on Unix system. --- .../CoreCLR/CorePsPlatform.cs | 92 ++++++------------- 1 file changed, 29 insertions(+), 63 deletions(-) diff --git a/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs b/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs index 783afe62915..4baede0d8f3 100644 --- a/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs +++ b/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs @@ -594,81 +594,47 @@ public class CommonStat private const char CanRead = 'r'; private const char CanWrite = 'w'; private const char CanExecute = 'x'; - - // helper for getting unix mode - private readonly Dictionary modeMap = new() - { - { StatMask.OwnerRead, CanRead }, - { StatMask.OwnerWrite, CanWrite }, - { StatMask.OwnerExecute, CanExecute }, - { StatMask.GroupRead, CanRead }, - { StatMask.GroupWrite, CanWrite }, - { StatMask.GroupExecute, CanExecute }, - { StatMask.OtherRead, CanRead }, - { StatMask.OtherWrite, CanWrite }, - { StatMask.OtherExecute, CanExecute }, - }; - - private readonly StatMask[] permissions = new StatMask[] - { - StatMask.OwnerRead, - StatMask.OwnerWrite, - StatMask.OwnerExecute, - StatMask.GroupRead, - StatMask.GroupWrite, - StatMask.GroupExecute, - StatMask.OtherRead, - StatMask.OtherWrite, - StatMask.OtherExecute - }; + private const char NoPerm = '-'; + private const char SetAndExec = 's'; + private const char SetAndNotExec = 'S'; + private const char StickyAndExec = 't'; + private const char StickyAndNotExec = 'T'; // The item type and the character representation for the first element in the stat string private readonly Dictionary itemTypeTable = new() { - { ItemType.BlockDevice, 'b' }, + { ItemType.BlockDevice, 'b' }, { ItemType.CharacterDevice, 'c' }, - { ItemType.Directory, 'd' }, - { ItemType.File, '-' }, - { ItemType.NamedPipe, 'p' }, - { ItemType.Socket, 's' }, - { ItemType.SymbolicLink, 'l' }, + { ItemType.Directory, 'd' }, + { ItemType.File, '-' }, + { ItemType.NamedPipe, 'p' }, + { ItemType.Socket, 's' }, + { ItemType.SymbolicLink, 'l' }, }; /// Convert the mode to a string which is usable in our formatting. /// The mode converted into a Unix style string similar to the output of ls. public string GetModeString() { - int offset = 0; char[] modeCharacters = new char[10]; - modeCharacters[offset++] = itemTypeTable[ItemType]; - - foreach (StatMask permission in permissions) - { - // determine whether we are setuid, sticky, or the usual rwx. - if ((Mode & (int)permission) == (int)permission) - { - if ((permission == StatMask.OwnerExecute && IsSetUid) || (permission == StatMask.GroupExecute && IsSetGid)) - { - // Check for setuid and add 's' - modeCharacters[offset] = 's'; - } - else if (permission == StatMask.OtherExecute && IsSticky && (ItemType == ItemType.Directory)) - { - // Directories are sticky, rather than setuid - modeCharacters[offset] = 't'; - } - else - { - modeCharacters[offset] = modeMap[permission]; - } - } - else - { - modeCharacters[offset] = '-'; - } - - offset++; - } + modeCharacters[0] = itemTypeTable[ItemType]; + bool isExecutable; + + UnixFileMode modeInfo = (UnixFileMode)Mode; + modeCharacters[1] = modeInfo.HasFlag(UnixFileMode.UserRead) ? CanRead : NoPerm; + modeCharacters[2] = modeInfo.HasFlag(UnixFileMode.UserWrite) ? CanWrite : NoPerm; + isExecutable = modeInfo.HasFlag(UnixFileMode.UserExecute); + modeCharacters[3] = modeInfo.HasFlag(UnixFileMode.SetUser) ? (isExecutable ? SetAndExec : SetAndNotExec) : (isExecutable ? CanExecute : NoPerm); + + modeCharacters[4] = modeInfo.HasFlag(UnixFileMode.GroupRead) ? CanRead : NoPerm; + modeCharacters[5] = modeInfo.HasFlag(UnixFileMode.GroupWrite) ? CanWrite : NoPerm; + isExecutable = modeInfo.HasFlag(UnixFileMode.GroupExecute); + modeCharacters[6] = modeInfo.HasFlag(UnixFileMode.SetGroup) ? (isExecutable ? SetAndExec : SetAndNotExec) : (isExecutable ? CanExecute : NoPerm); + + modeCharacters[7] = modeInfo.HasFlag(UnixFileMode.OtherRead) ? CanRead : NoPerm; + modeCharacters[8] = modeInfo.HasFlag(UnixFileMode.OtherWrite) ? CanWrite : NoPerm; + isExecutable = modeInfo.HasFlag(UnixFileMode.OtherExecute); + modeCharacters[9] = modeInfo.HasFlag(UnixFileMode.StickyBit) ? (isExecutable ? StickyAndExec : StickyAndNotExec) : (isExecutable ? CanExecute : NoPerm); return new string(modeCharacters); } From 205a65ec22164c41731f6b852091ff7b6757348d Mon Sep 17 00:00:00 2001 From: James Truher Date: Mon, 25 Sep 2023 13:42:45 -0700 Subject: [PATCH 2/9] Add tests to cover more modes. --- .../UnixStat.Tests.ps1 | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/test/powershell/Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1 index b14df30e7e2..edcbcf97b56 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1 @@ -37,6 +37,10 @@ Describe "UnixFileSystem additions" -Tag "CI" { @{ Mode = '555'; Perm = '-r-xr-xr-x'; Item = "${testFile}" }, @{ Mode = '666'; Perm = '-rw-rw-rw-'; Item = "${testFile}" }, @{ Mode = '777'; Perm = '-rwxrwxrwx'; Item = "${testFile}" }, + @{ Mode = '4644'; Perm = '-rwSr--r--'; Item = "${testFile}" }, + @{ Mode = '1644'; Perm = '-rw-r--r-T'; Item = "${testFile}" }, + @{ Mode = '2644'; Perm = '-rw-r-Sr--'; Item = "${testFile}" }, + @{ Mode = '7644'; Perm = '-rwSr-Sr-T'; Item = "${testFile}" }, @{ Mode = '4777'; Perm = '-rwsrwxrwx'; Item = "${testFile}" }, @{ Mode = '1777'; Perm = 'drwxrwxrwt'; Item = "${testDir}" } } @@ -58,8 +62,13 @@ Describe "UnixFileSystem additions" -Tag "CI" { It "Should present filemode '' string correctly as ''" -testCase $testCase { param ($Mode, $Perm, $Item ) chmod "$Mode" "${Item}" - $i = Get-Item $Item - $i.UnixMode | Should -Be $Perm + if ($LASTEXITCODE -ne 0) { + set-itresult -skip -because "chmod '$mode' failed" + } + else { + $i = Get-Item $Item + $i.UnixMode | Should -Be $Perm + } } It "Should retrieve the user name for the file" { From 5631c6fec9cabf7e35aa729761b574692fa0b7b2 Mon Sep 17 00:00:00 2001 From: James Truher Date: Mon, 25 Sep 2023 16:16:50 -0700 Subject: [PATCH 3/9] Add comment to explain why we check for chmod success. --- .../Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1 | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/powershell/Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1 index edcbcf97b56..a47de288766 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1 @@ -61,6 +61,8 @@ Describe "UnixFileSystem additions" -Tag "CI" { It "Should present filemode '' string correctly as ''" -testCase $testCase { param ($Mode, $Perm, $Item ) + # chmod can fail for some modes so be sure to handle that here. + # specifically, when setting setgid chmod can fail if the group is privileged. chmod "$Mode" "${Item}" if ($LASTEXITCODE -ne 0) { set-itresult -skip -because "chmod '$mode' failed" From fcc5c171d5cdc75534a82a449af7de08ac4a2711 Mon Sep 17 00:00:00 2001 From: "James Truher [MSFT]" Date: Thu, 28 Sep 2023 15:59:13 -0700 Subject: [PATCH 4/9] Update test/powershell/Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1 Co-authored-by: Ilya --- .../Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/powershell/Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1 index a47de288766..fcefb707a2e 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Management/UnixStat.Tests.ps1 @@ -69,7 +69,7 @@ Describe "UnixFileSystem additions" -Tag "CI" { } else { $i = Get-Item $Item - $i.UnixMode | Should -Be $Perm + $i.UnixMode | Should -BeExactly $Perm } } From b2959a07408431b6ff1df8c1476131b6db07e2c5 Mon Sep 17 00:00:00 2001 From: James Truher Date: Mon, 2 Oct 2023 16:19:48 -0700 Subject: [PATCH 5/9] Reduce allocations by providing some common mode strings as constants. Use stackalloc rather than new for character mode array. --- .../CoreCLR/CorePsPlatform.cs | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs b/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs index 4baede0d8f3..1bb0ce10032 100644 --- a/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs +++ b/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs @@ -612,14 +612,33 @@ public class CommonStat { ItemType.SymbolicLink, 'l' }, }; + private const string OwnerReadGroupReadOtherRead = "-r--r--r--"; + private const string OwnerReadWriteGroupReadOtherRead = "-rw-r--r--"; + private const string DirectoryOwnerFullGroupReadExecOtherReadExec = "drwxr-xr-x"; + /// Convert the mode to a string which is usable in our formatting. /// The mode converted into a Unix style string similar to the output of ls. public string GetModeString() { - char[] modeCharacters = new char[10]; + Span modeCharacters = stackalloc char[10]; modeCharacters[0] = itemTypeTable[ItemType]; bool isExecutable; + // We'll do a couple of common mode strings here to reduce allocations and improve performance a bit. + // On an Ubuntu system (docker), these 3 are roughly 70% of all the permissions + if ((Mode & 0xFFF) == 292) + { + return OwnerReadGroupReadOtherRead; + } + else if ((Mode & 0xFFF) == 420) + { + return OwnerReadWriteGroupReadOtherRead; + } + else if ((ItemType == ItemType.Directory) & ((Mode & 0xFFF) == 493)) + { + return DirectoryOwnerFullGroupReadExecOtherReadExec; + } + UnixFileMode modeInfo = (UnixFileMode)Mode; modeCharacters[1] = modeInfo.HasFlag(UnixFileMode.UserRead) ? CanRead : NoPerm; modeCharacters[2] = modeInfo.HasFlag(UnixFileMode.UserWrite) ? CanWrite : NoPerm; From 4f1d4fb6ba47c54b06ab4eba147f29f448d500ec Mon Sep 17 00:00:00 2001 From: James Truher Date: Mon, 2 Oct 2023 16:21:29 -0700 Subject: [PATCH 6/9] Revert "Reduce allocations by providing some common mode strings as constants." This reverts commit 22769e9cd46bfc3620f96081e3b3783fa4e2d485. --- .../CoreCLR/CorePsPlatform.cs | 21 +------------------ 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs b/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs index 1bb0ce10032..4baede0d8f3 100644 --- a/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs +++ b/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs @@ -612,33 +612,14 @@ public class CommonStat { ItemType.SymbolicLink, 'l' }, }; - private const string OwnerReadGroupReadOtherRead = "-r--r--r--"; - private const string OwnerReadWriteGroupReadOtherRead = "-rw-r--r--"; - private const string DirectoryOwnerFullGroupReadExecOtherReadExec = "drwxr-xr-x"; - /// Convert the mode to a string which is usable in our formatting. /// The mode converted into a Unix style string similar to the output of ls. public string GetModeString() { - Span modeCharacters = stackalloc char[10]; + char[] modeCharacters = new char[10]; modeCharacters[0] = itemTypeTable[ItemType]; bool isExecutable; - // We'll do a couple of common mode strings here to reduce allocations and improve performance a bit. - // On an Ubuntu system (docker), these 3 are roughly 70% of all the permissions - if ((Mode & 0xFFF) == 292) - { - return OwnerReadGroupReadOtherRead; - } - else if ((Mode & 0xFFF) == 420) - { - return OwnerReadWriteGroupReadOtherRead; - } - else if ((ItemType == ItemType.Directory) & ((Mode & 0xFFF) == 493)) - { - return DirectoryOwnerFullGroupReadExecOtherReadExec; - } - UnixFileMode modeInfo = (UnixFileMode)Mode; modeCharacters[1] = modeInfo.HasFlag(UnixFileMode.UserRead) ? CanRead : NoPerm; modeCharacters[2] = modeInfo.HasFlag(UnixFileMode.UserWrite) ? CanWrite : NoPerm; From cb92b5f62a27e5b5da8d6629b8c3dbcc92740585 Mon Sep 17 00:00:00 2001 From: James Truher Date: Mon, 2 Oct 2023 17:02:05 -0700 Subject: [PATCH 7/9] Reduce allocations by providing some common mode strings as constants. Use stackalloc rather than new for character mode array. --- .../CoreCLR/CorePsPlatform.cs | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs b/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs index 4baede0d8f3..4541e302c69 100644 --- a/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs +++ b/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs @@ -612,11 +612,30 @@ public class CommonStat { ItemType.SymbolicLink, 'l' }, }; + // We'll create a few common mode strings here to reduce allocations and improve performance a bit. + private const string OwnerReadGroupReadOtherRead = "-r--r--r--"; + private const string OwnerReadWriteGroupReadOtherRead = "-rw-r--r--"; + private const string DirectoryOwnerFullGroupReadExecOtherReadExec = "drwxr-xr-x"; + /// Convert the mode to a string which is usable in our formatting. /// The mode converted into a Unix style string similar to the output of ls. public string GetModeString() { - char[] modeCharacters = new char[10]; + // On an Ubuntu system (docker), these 3 are roughly 70% of all the permissions + if ((Mode & 0xFFF) == 292) + { + return OwnerReadGroupReadOtherRead; + } + else if ((Mode & 0xFFF) == 420) + { + return OwnerReadWriteGroupReadOtherRead; + } + else if ((ItemType == ItemType.Directory) & ((Mode & 0xFFF) == 493)) + { + return DirectoryOwnerFullGroupReadExecOtherReadExec; + } + + Span modeCharacters = stackalloc char[10]; modeCharacters[0] = itemTypeTable[ItemType]; bool isExecutable; From ef7e0a8b460fe279b52f75350c8edc1f542079c7 Mon Sep 17 00:00:00 2001 From: "James Truher [MSFT]" Date: Wed, 11 Oct 2023 16:12:04 -0700 Subject: [PATCH 8/9] Update src/System.Management.Automation/CoreCLR/CorePsPlatform.cs Co-authored-by: Dongbo Wang --- src/System.Management.Automation/CoreCLR/CorePsPlatform.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs b/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs index 4541e302c69..c6ad976a88e 100644 --- a/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs +++ b/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs @@ -626,7 +626,8 @@ public string GetModeString() { return OwnerReadGroupReadOtherRead; } - else if ((Mode & 0xFFF) == 420) + + if ((Mode & 0xFFF) == 420) { return OwnerReadWriteGroupReadOtherRead; } From 70292b0884d531e1333910e81d68f067bfebcef9 Mon Sep 17 00:00:00 2001 From: "James Truher [MSFT]" Date: Wed, 11 Oct 2023 16:12:15 -0700 Subject: [PATCH 9/9] Update src/System.Management.Automation/CoreCLR/CorePsPlatform.cs Co-authored-by: Dongbo Wang --- src/System.Management.Automation/CoreCLR/CorePsPlatform.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs b/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs index c6ad976a88e..dc5db5f2c48 100644 --- a/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs +++ b/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs @@ -601,7 +601,7 @@ public class CommonStat private const char StickyAndNotExec = 'T'; // The item type and the character representation for the first element in the stat string - private readonly Dictionary itemTypeTable = new() + private static readonly Dictionary itemTypeTable = new() { { ItemType.BlockDevice, 'b' }, { ItemType.CharacterDevice, 'c' }, @@ -631,7 +631,8 @@ public string GetModeString() { return OwnerReadWriteGroupReadOtherRead; } - else if ((ItemType == ItemType.Directory) & ((Mode & 0xFFF) == 493)) + + if (ItemType == ItemType.Directory & (Mode & 0xFFF) == 493) { return DirectoryOwnerFullGroupReadExecOtherReadExec; }