Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement UnixFileMode APIs #69980

Merged
merged 42 commits into from
Jun 23, 2022
Merged

Implement UnixFileMode APIs #69980

Show file tree
Hide file tree
Changes from 39 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
11c2422
Implement UnixFileMode APIs on Unix.
tmds May 30, 2022
b182b4d
Throw PNSE on Windows, add UnsupportedOSPlatform.
tmds Jun 2, 2022
f143499
Fix API compat issue.
tmds Jun 2, 2022
5138601
Borrow a few things from SafeFileHandle API PR to this compiles.
tmds Jun 2, 2022
4d6c524
Fix System.IO.FileSystem.AccessControl compilation.
tmds Jun 2, 2022
813e24a
Add xml docs.
tmds Jun 2, 2022
7bd2ae3
Replace Interop.Sys.Permissions to System.IO.UnixFileMode.
tmds Jun 3, 2022
d67d738
Throw PNSE immediately on Windows.
tmds Jun 3, 2022
3841bfb
Add ODE to xml docs of methods that accept a handle.
tmds Jun 3, 2022
d1e7ea8
Don't throw (PNSE) from FileSystemInfo.UnixFileMode getter on Windows.
tmds Jun 3, 2022
f7db626
Minor style fix.
tmds Jun 3, 2022
88828a4
Get rid of some casts.
tmds Jun 5, 2022
cd4104f
Add tests for creating a file/directory with UnixFileMode.
tmds Jun 7, 2022
c937c28
Some CI envs don't have a umask exe, try retrieving via a shell builtin.
tmds Jun 7, 2022
a12832c
Update expected test mode values.
tmds Jun 7, 2022
d294651
Fix OSX
tmds Jun 8, 2022
3df946a
Fix Windows build.
tmds Jun 8, 2022
f3c55bf
Add ArgumentException tests.
tmds Jun 8, 2022
91e0891
Fix Windows build.
tmds Jun 8, 2022
6e74d98
Add get/set tests.
tmds Jun 8, 2022
757c1e5
Update test for Windows.
tmds Jun 8, 2022
53539ec
Make setters target link instead of link target.
tmds Jun 8, 2022
873660e
Linux: fix SetUnixFileMode
tmds Jun 10, 2022
d9c7789
Fix OSX compilation.
tmds Jun 10, 2022
3dba808
Try make all tests pass in CI.
tmds Jun 10, 2022
33d3e6f
For link, operate on target permissions.
tmds Jun 17, 2022
18e70c9
Skip tests on Browser.
tmds Jun 17, 2022
b2423c6
Add tests for 'Get' that doesn't use a 'Set' first.
tmds Jun 17, 2022
777b77d
Don't perform exist check for handles.
tmds Jun 17, 2022
4e93e11
Fix Get test for wasm.
tmds Jun 17, 2022
60d24a7
Review xml comments.
tmds Jun 17, 2022
8f8ff2d
Add comment to test.
tmds Jun 17, 2022
97df035
GetUnixFileMode for handle won't throw UnauthorizedAccessException.
tmds Jun 18, 2022
7bfa541
Apply suggestions from code review
tmds Jun 21, 2022
01f6ff3
PR feedback.
tmds Jun 21, 2022
6e91cf1
Update enum doc to say 'owner' instead of 'user'.
tmds Jun 21, 2022
4cb6316
Use UnixFileMode in library.
tmds Jun 21, 2022
7046ea9
Use UnixFileMode in library tests.
tmds Jun 21, 2022
e55011d
Fix Windows build.
tmds Jun 21, 2022
bc7383b
Fix missing FileAccess when changing to FileStreamOptions API.
tmds Jun 21, 2022
d1f043d
PR feedback.
tmds Jun 22, 2022
168bdf5
Fix Argument_InvalidUnixCreateMode message.
tmds Jun 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 0 additions & 33 deletions src/libraries/Common/src/Interop/Unix/System.Native/Interop.Permissions.cs
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,8 +276,6 @@
Link="Common\Interop\Unix\System.Native\Interop.Access.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Stat.cs"
Link="Common\Interop\Unix\Interop.Stat.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Permissions.cs"
Link="Common\Interop\Unix\Interop.Permissions.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.GetEUid.cs"
Link="Common\Interop\Unix\Interop.GetEUid.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.IsMemberOfGroup.cs"
Expand Down
12 changes: 7 additions & 5 deletions src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,10 +763,12 @@ private static bool IsExecutable(string fullPath)
return false;
}

Interop.Sys.Permissions permissions = ((Interop.Sys.Permissions)fileinfo.Mode) & Interop.Sys.Permissions.S_IXUGO;
const UnixFileMode AllExecute = UnixFileMode.UserExecute | UnixFileMode.GroupExecute | UnixFileMode.OtherExecute;

UnixFileMode permissions = ((UnixFileMode)fileinfo.Mode) & AllExecute;

// Avoid checking user/group when permission.
if (permissions == Interop.Sys.Permissions.S_IXUGO)
if (permissions == AllExecute)
{
return true;
}
Expand All @@ -785,11 +787,11 @@ private static bool IsExecutable(string fullPath)
if (euid == fileinfo.Uid)
{
// We own the file.
return (permissions & Interop.Sys.Permissions.S_IXUSR) != 0;
return (permissions & UnixFileMode.UserExecute) != 0;
}

bool groupCanExecute = (permissions & Interop.Sys.Permissions.S_IXGRP) != 0;
bool otherCanExecute = (permissions & Interop.Sys.Permissions.S_IXOTH) != 0;
bool groupCanExecute = (permissions & UnixFileMode.GroupExecute) != 0;
tmds marked this conversation as resolved.
Show resolved Hide resolved
bool otherCanExecute = (permissions & UnixFileMode.OtherExecute) != 0;

// Avoid group check when group and other have same permissions.
if (groupCanExecute == otherCanExecute)
Expand Down
18 changes: 5 additions & 13 deletions src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ public void ProcessNameMatchesScriptName()
string scriptName = GetTestFileName();
string filename = Path.Combine(TestDirectory, scriptName);
File.WriteAllText(filename, $"#!/bin/sh\nsleep 600\n"); // sleep 10 min.
ChMod(filename, "744"); // set x-bit
File.SetUnixFileMode(filename, ExecutablePermissions);

using (var process = Process.Start(new ProcessStartInfo { FileName = filename }))
{
Expand Down Expand Up @@ -235,7 +235,7 @@ public void ProcessStart_UseShellExecute_OnUnix_FallsBackWhenNotRealExecutable()
// Create a file that has the x-bit set, but which isn't a valid script.
string filename = WriteScriptFile(TestDirectory, GetTestFileName(), returnValue: 0);
File.WriteAllText(filename, $"not a script");
ChMod(filename, "744"); // set x-bit
File.SetUnixFileMode(filename, ExecutablePermissions);

RemoteInvokeOptions options = new RemoteInvokeOptions();
options.StartInfo.EnvironmentVariables["PATH"] = path;
Expand Down Expand Up @@ -508,7 +508,7 @@ public void TestStartOnUnixWithBadPermissions()
{
string path = GetTestFilePath();
File.Create(path).Dispose();
ChMod(path, "644");
File.SetUnixFileMode(path, UnixFileMode.UserRead | UnixFileMode.UserWrite);

Win32Exception e = Assert.Throws<Win32Exception>(() => Process.Start(path));
Assert.NotEqual(0, e.NativeErrorCode);
Expand All @@ -520,7 +520,7 @@ public void TestStartOnUnixWithBadFormat()
{
string path = GetTestFilePath();
File.Create(path).Dispose();
ChMod(path, "744"); // set x-bit
File.SetUnixFileMode(path, ExecutablePermissions);

Win32Exception e = Assert.Throws<Win32Exception>(() => Process.Start(path));
Assert.NotEqual(0, e.NativeErrorCode);
Expand Down Expand Up @@ -936,14 +936,6 @@ private static int GetWaitStateReferenceCount(object waitState)
return (int)referenCountField.GetValue(waitState);
}

[DllImport("libc")]
private static extern int chmod(string path, int mode);

private static void ChMod(string filename, string mode)
{
Assert.Equal(0, chmod(filename, Convert.ToInt32(mode, 8)));
}

[DllImport("libc")]
private static extern uint geteuid();
[DllImport("libc")]
Expand Down Expand Up @@ -1001,7 +993,7 @@ private string WriteScriptFile(string directory, string name, int returnValue)
{
string filename = Path.Combine(directory, name);
File.WriteAllText(filename, $"#!/bin/sh\nexit {returnValue}\n");
ChMod(filename, "744"); // set x-bit
File.SetUnixFileMode(filename, ExecutablePermissions);
return filename;
}

Expand Down
3 changes: 0 additions & 3 deletions src/libraries/System.Diagnostics.Process/tests/ProcessTests.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,5 @@ private string WriteScriptFile(string directory, string name, int returnValue)
File.WriteAllText(filename, $"exit {returnValue}");
return filename;
}

private static void ChMod(string filename, string mode)
=> throw new PlatformNotSupportedException();
}
}
6 changes: 5 additions & 1 deletion src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ namespace System.Diagnostics.Tests
{
public partial class ProcessTests : ProcessTestBase
{
const UnixFileMode ExecutablePermissions = UnixFileMode.UserRead | UnixFileMode.UserExecute | UnixFileMode.UserWrite |
UnixFileMode.GroupRead | UnixFileMode.GroupExecute |
UnixFileMode.GroupRead | UnixFileMode.GroupExecute;

private class FinalizingProcess : Process
{
public static volatile bool WasFinalized;
Expand Down Expand Up @@ -2193,7 +2197,7 @@ public void LongProcessNamesAreSupported()
// Instead of using sleep directly, we wrap it with a script.
sleepPath = GetTestFilePath();
File.WriteAllText(sleepPath, $"#!/bin/sh\nsleep 600\n"); // sleep 10 min.
ChMod(sleepPath, "744");
File.SetUnixFileMode(sleepPath, ExecutablePermissions);
}
else
{
Expand Down
1 change: 0 additions & 1 deletion src/libraries/System.Formats.Tar/src/System.Formats.Tar.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.FChMod.cs" Link="Common\Interop\Unix\System.Native\Interop.FChMod.cs" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interop.FChMod.cs

Can the Interop.FChMod.cs file also be deleted from this .csproj?

<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Link.cs" Link="Common\Interop\Unix\System.Native\Interop.Link.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.MkFifo.cs" Link="Common\Interop\Unix\System.Native\Interop.MkFifo.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Permissions.cs" Link="Common\Interop\Unix\Interop.Permissions.cs" />
tmds marked this conversation as resolved.
Show resolved Hide resolved
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Stat.cs" Link="Common\Interop\Unix\Interop.Stat.cs" />
<Compile Include="$(CommonPath)System\IO\Archiving.Utils.Unix.cs" Link="Common\System\IO\Archiving.Utils.Unix.cs" />
</ItemGroup>
Expand Down
5 changes: 4 additions & 1 deletion src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Diagnostics;
using Microsoft.Win32.SafeHandles;
using System.IO;

namespace System.Formats.Tar
{
Expand Down Expand Up @@ -53,7 +54,9 @@ partial void SetModeOnFile(SafeFileHandle handle, string destinationFileName)
// If the permissions weren't set at all, don't write the file's permissions.
if (permissions != 0)
{
Interop.CheckIo(Interop.Sys.FChMod(handle, permissions), destinationFileName);
#pragma warning disable CA1416 // Validate platform compatibility
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this to avoid the error for using an API that is not supported on Windows.
The file is only used on Unix, so maybe there is a better way to handle this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@buyaa-n @ViktorHofer the File.SetUnixFileMode method is marked as [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("windows")] and this particular C# file is compiled only for Unix:

runtime/src/libraries/System.Formats.Tar/src/System.Formats.Tar.csproj

Lines 52 to 53 in e55011d

<ItemGroup Condition="'$(TargetPlatformIdentifier)' == 'Unix'">
<Compile Include="System\Formats\Tar\TarEntry.Unix.cs" />

Is it expected that the compiler produces warning?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not expected, maybe it is loaded in cross platform build? What is the whole warning message? Message includes the call site info

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it by mistake to TarEntry.Unix.cs. It is only required for ZipFileExtensions.ZipArchiveEntry.Extract.Unix.cs.
And there it makes sense because the file is included in $(NetCoreAppCurrent) target.

/home/tmds/repos/runtime/src/libraries/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFileExtensions.ZipArchiveEntry.Extract.Unix.cs(26,17): error CA1416: This call site is reachable on all platforms. 'File.SetUnixFileMode(SafeFileHandle, UnixFileMode)' is unsupported on: 'windows'. [/home/tmds/repos/runtime/src/libraries/System.IO.Compression.ZipFile/src/System.IO.Compression.ZipFile.csproj]

The csproj file has:

runtime/src/libraries/System.IO.Compression.ZipFile/src/System.IO.Compression.ZipFile.csproj

Line 4 in 3282677

<TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)</TargetFrameworks>

and

runtime/src/libraries/System.IO.Compression.ZipFile/src/System.IO.Compression.ZipFile.csproj

Lines 25 to 27 in 3282677

<ItemGroup Condition="'$(TargetPlatformIdentifier)' == ''">
<Compile Include="System\IO\Compression\ZipFileExtensions.ZipArchive.Create.Unix.cs" />
<Compile Include="System\IO\Compression\ZipFileExtensions.ZipArchiveEntry.Extract.Unix.cs" />

Copy link
Member

@eerhardt eerhardt Jun 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TFMs on this project seem incorrect to me. It has changed quite a few times in the last year.

  1. When I made it respect file modes in Compression.ZipFile support for Unix Permissions #55531 to have a separate "Unix" build
  2. @ViktorHofer changed it to have an "agnostic" TFM (basically the "Windows" build) in Add NetCoreAppCurrent rid agnostic tfm to libs #64518
  3. Lastly in April with Added test for extracting zip files with invalid characters in Windows #67332 to add back a "Windows" TFM and make the "unix" side the agnostic build.

So unfortunately, because of these TFM changes, the "Unix" build doesn't get the attribute that says "this isn't Windows", which means the analyzer is handicapped here.

Instead of disabling the warning, I think just adding Debug.Assert(!OperatingSystem.IsWindows()); at the top of the method would be best for now. Long term, I hope we can settle on a manageable TFM plan.

Copy link
Member

@ViktorHofer ViktorHofer Jun 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long term, I hope we can settle on a manageable TFM plan.

@eerhardt can you please elaborate on why you think that the current strategy of choosing target frameworks isn't manageable? cc @ericstj

Some libraries intentionally define a TargetPlatform agnostic target framework which serves as the default configuration to reduce the number of inner builds in the build graph. Such a TargetPlatform agnostic target framework is either truly platform agnostic or it targets a specific platform that is believed to be a good default (in most / all cases Unix).

Copy link
Member

@ViktorHofer ViktorHofer Jun 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@buyaa-n @ViktorHofer the File.SetUnixFileMode method is marked as [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("windows")] and this particular C# file is compiled only for Unix:

The SDK unconditionally adds supported platforms and one of those is Windows. The SDK itself doesn't make it possible to only target Windows, macOS or Linux via a target platform, but we in dotnet/runtime allow that via the Microsoft.DotNet.TargetFramework.Sdk.

Because of that, we would need to remove these unconditionally set SupportedPlatform items and instead add them conditionally similar to how it is done for Browser.

Here you can see that the SupportedPlatform metadata is added for Browser when the platform is Browser, or when it's agnostic and the library doesn't have a Browser specific implementation.

I think we would want the same for Unix and for any other platform.

EDIT:
Please see ViktorHofer#2 (my fork, just to get feedback). That logic calculates the SupportedPlatforms items correctly based on the RID graph. This implementation is completely static but solves the noted issues.

To make this dynamic, an msbuild task in the TargetFramework.Sdk would be required that takes the build rid graph (OSGroups.json), TargetFrameworks, TargetFramework and TargetPlatformIdentifier as an input and outputs the SupportedPlatform item to be added.

cc @buyaa-n @jeffhandley

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SDK unconditionally adds supported platforms and one of those is Windows. The SDK itself doesn't make it possible to only target Windows, macOS or Linux via a target platform, but we in dotnet/runtime allow that via the Microsoft.DotNet.TargetFramework.Sdk.

SDK supported platforms cause warnings only for cross platform targets, has no effect targeted builds. I might missing something but still do not understand why we could not use Linux or Unix targets for the above mentioned build and keep the $(NetCoreAppCurrent) only for real cross platform targets.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the discussion into #71251.

File.SetUnixFileMode(handle, (UnixFileMode)permissions);
#pragma warning disable CA1416
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion ...ssion.ZipFile/src/System/IO/Compression/ZipFileExtensions.ZipArchiveEntry.Extract.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ static partial void ExtractExternalAttributes(FileStream fs, ZipArchiveEntry ent
// include the permissions, or was made on Windows.
if (permissions != 0)
{
Interop.CheckIo(Interop.Sys.FChMod(fs.SafeFileHandle, permissions), fs.Name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the Interop file be removed from this .csproj?

#pragma warning disable CA1416 // Validate platform compatibility
File.SetUnixFileMode(fs.SafeFileHandle, (UnixFileMode)permissions);
#pragma warning restore CA1416
}
}
}
Expand Down
10 changes: 9 additions & 1 deletion src/libraries/System.IO.FileSystem/tests/Base/AllGetSetAttributes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,15 @@ public void SymLinksReflectSymLinkAttributes()
try
{
Assert.Equal(FileAttributes.ReadOnly, FileAttributes.ReadOnly & GetAttributes(path));
Assert.NotEqual(FileAttributes.ReadOnly, FileAttributes.ReadOnly & GetAttributes(linkPath));
if (OperatingSystem.IsWindows())
{
Assert.NotEqual(FileAttributes.ReadOnly, FileAttributes.ReadOnly & GetAttributes(linkPath));
}
else
{
// On Unix, Get/SetAttributes FileAttributes.ReadOnly operates on the target of the link.
tmds marked this conversation as resolved.
Show resolved Hide resolved
Assert.Equal(FileAttributes.ReadOnly, FileAttributes.ReadOnly & GetAttributes(linkPath));
}
}
finally
{
Expand Down
Loading