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.

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

Implement UnixFileMode APIs #69980

Merged
merged 42 commits into from
Jun 23, 2022
Merged
Show file tree
Hide file tree
Changes from 37 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

This file was deleted.

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
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
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
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:

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

Is it expected that the compiler produces warning?

Copy link
Member

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:

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

and
<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

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

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

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
Member

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
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
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,217 @@
// 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.CompilerServices;
using Xunit;

namespace System.IO.Tests
{
public abstract class BaseGetSetUnixFileMode : FileSystemTest
{
protected abstract UnixFileMode GetMode(string path);
protected abstract void SetMode(string path, UnixFileMode mode);

// When false, the Get API returns (UnixFileMode)(-1) when the file doesn't exist.
protected virtual bool GetThrowsWhenDoesntExist => false;

// The FileSafeHandle APIs require a readable file to open the handle.
tmds marked this conversation as resolved.
Show resolved Hide resolved
protected virtual bool GetModeNeedsReadableFile => false;

// When false, the Get API returns (UnixFileMode)(-1) when the platform is not supported (Windows).
protected virtual bool GetModeThrowsPNSE => true;

// Determines if the derived Test class is for directories or files.
protected virtual bool IsDirectory => false;

// On OSX, directories created under /tmp have same group as /tmp.
// Because that group is different from the test user's group, chmod
// returns EPERM when trying to setgid on directories, and for files
// chmod filters out the bit.
// We skip the tests with setgid.
private bool CanSetGroup => !PlatformDetection.IsBsdLike;

private string CreateTestItem(string path = null, [CallerMemberName] string memberName = null, [CallerLineNumber] int lineNumber = 0)
{
path = path ?? GetTestFilePath(null, memberName, lineNumber);
if (IsDirectory)
{
Directory.CreateDirectory(path);
}
else
{
File.Create(path).Dispose();
}
return path;
}

[PlatformSpecific(TestPlatforms.AnyUnix)]
[Fact]
public void Get()
{
string path = CreateTestItem();

UnixFileMode mode = GetMode(path); // Doesn't throw.

Assert.NotEqual((UnixFileMode)(-1), mode);

UnixFileMode required = UnixFileMode.UserRead | UnixFileMode.UserWrite;
if (IsDirectory)
{
required = UnixFileMode.UserExecute;
}
tmds marked this conversation as resolved.
Show resolved Hide resolved
Assert.True((mode & required) == required);

if (!PlatformDetection.IsBrowser)
{
// The umask should prevent this file from being writable by others.
Assert.Equal(UnixFileMode.None, mode & UnixFileMode.OtherWrite);
}
}

[PlatformSpecific(TestPlatforms.AnyUnix & ~TestPlatforms.Browser)]
[Theory]
[MemberData(nameof(TestUnixFileModes))]
public void SetThenGet(UnixFileMode mode)
{
if (!CanSetGroup && (mode & UnixFileMode.SetGroup) != 0)
{
return; // Skip
}
if (GetModeNeedsReadableFile)
{
// Ensure the file remains readable.
mode |= UnixFileMode.UserRead;
}

string path = CreateTestItem();

SetMode(path, mode);

Assert.Equal(mode, GetMode(path));
}

[PlatformSpecific(TestPlatforms.AnyUnix & ~TestPlatforms.Browser)]
[Theory]
[MemberData(nameof(TestUnixFileModes))]
public void SetThenGet_SymbolicLink(UnixFileMode mode)
{
if (!CanSetGroup && (mode & UnixFileMode.SetGroup) != 0)
{
return; // Skip
}
if (GetModeNeedsReadableFile)
{
// Ensure the file remains readable.
mode |= UnixFileMode.UserRead;
}

string path = CreateTestItem();

string linkPath = GetTestFilePath();
File.CreateSymbolicLink(linkPath, path);

SetMode(linkPath, mode);

Assert.Equal(mode, GetMode(linkPath));
Assert.Equal(mode, GetMode(path));
}

[PlatformSpecific(TestPlatforms.AnyUnix)]
[Fact]
public void FileDoesntExist()
{
string path = GetTestFilePath();

if (GetThrowsWhenDoesntExist)
{
Assert.Throws<FileNotFoundException>(() => GetMode(path));
}
else
{
Assert.Equal((UnixFileMode)(-1), GetMode(path));
}
Assert.Throws<FileNotFoundException>(() => SetMode(path, AllAccess));
}

[PlatformSpecific(TestPlatforms.AnyUnix)]
[Fact]
public void FileDoesntExist_SymbolicLink()
{
string path = GetTestFilePath();
string linkPath = GetTestFilePath();
File.CreateSymbolicLink(linkPath, path);

Assert.Throws<FileNotFoundException>(() => SetMode(linkPath, AllAccess));

if (GetThrowsWhenDoesntExist)
{
Assert.Throws<FileNotFoundException>(() => GetMode(path));
}
else
{
Assert.Equal((UnixFileMode)(-1), GetMode(path));
}
}

[PlatformSpecific(TestPlatforms.AnyUnix)]
[Fact]
public void ParentDirDoesntExist()
{
string path = Path.Combine(GetTestFilePath(), "dir", "file");

if (GetThrowsWhenDoesntExist)
{
Assert.Throws<DirectoryNotFoundException>(() => GetMode(path));
}
else
{
Assert.Equal((UnixFileMode)(-1), GetMode(path));
}
Assert.Throws<DirectoryNotFoundException>(() => SetMode(path, AllAccess));
}

[PlatformSpecific(TestPlatforms.AnyUnix)]
[Fact]
public void NullPath()
{
Assert.Throws<ArgumentNullException>(() => GetMode(null));
Assert.Throws<ArgumentNullException>(() => SetMode(null, AllAccess));
}

[PlatformSpecific(TestPlatforms.AnyUnix)]
[Fact]
public void InvalidPath()
{
Assert.Throws<ArgumentException>(() => GetMode(string.Empty));
Assert.Throws<ArgumentException>(() => SetMode(string.Empty, AllAccess));
}

[PlatformSpecific(TestPlatforms.AnyUnix)]
[Theory]
[InlineData((UnixFileMode)(1 << 12))]
public void InvalidMode(UnixFileMode mode)
{
string path = CreateTestItem();

Assert.Throws<ArgumentException>(() => SetMode(path, mode));
}

[PlatformSpecific(TestPlatforms.Windows)]
[Fact]
public void Unsupported()
{
string path = CreateTestItem();

Assert.Throws<PlatformNotSupportedException>(() => SetMode(path, AllAccess));

if (GetModeThrowsPNSE)
{
Assert.Throws<PlatformNotSupportedException>(() => GetMode(path));
}
else
{
Assert.Equal((UnixFileMode)(-1), GetMode(path));
}
}
}
}
Loading