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

[API Proposal]: APIs for working with unix file mode #67837

Closed
tmds opened this issue Apr 11, 2022 · 50 comments
Closed

[API Proposal]: APIs for working with unix file mode #67837

tmds opened this issue Apr 11, 2022 · 50 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.IO blocking Marks issues that we want to fast track in order to unblock other important work

Comments

@tmds
Copy link
Member

tmds commented Apr 11, 2022

Background and motivation

Add direct support for getting/setting Unix file permissions and special bits.

These APIs allow to restrict/extend access to files and directories from .NET.

There have been some requests for these APIs: #925, #928, #17540.

The tar implementation can make use of them (#65951) without resorting to internal APIs.

API Proposal

namespace System.IO;

[System.FlagsAttribute]
public enum UnixFileMode
{
// From https://github.com/dotnet/runtime/issues/65951.
    None = 0,
    OtherExecute = 1,
    OtherWrite = 2,
    OtherRead = 4,
    GroupExecute = 8,
    GroupWrite = 16,
    GroupRead = 32,
    UserExecute = 64,
    UserWrite = 128,
    UserRead = 256,
    StickyBit = 512,
    SetGroup = 1024, // or, more generic name: GroupSpecial
    SetUser = 2048,  // or, more generic name: UserSpecial

// Maybe:
    AccessPermissionMask = UserRead | UserWrite | UserExecute | GroupRead | GroupWrite | GroupExecute | OtherRead | OtherWrite | OtherExecute,
    DefaultFileOpenPermissions = UserRead | UserWrite | GroupRead | GroupWrite | OtherRead | OtherWrite
}

static partial class Directory
{
    // Set mode when creating file, returns SafeFileHandle.
    public static SafeFileHandle OpenHandle (string path, FileMode mode = FileMode.Open, FileAccess access = FileAccess.Read, FileShare share = FileShare.Read, FileOptions options = FileOptions.None, long preallocationSize = 0, UnixFileMode unixCreateMode = UnixFileMode.DefaultFileOpenPermissions);

    // Set mode when creating directory
    public static DirectoryInfo CreateDirectory(string path, UnixFileMode unixCreateMode);
}

static partial class File
{
    public static SafeFileHandle OpenHandle (string path, System.IO.FileMode mode = System.IO.FileMode.Open, System.IO.FileAccess access = System.IO.FileAccess.Read, System.IO.FileShare share = System.IO.FileShare.Read, System.IO.FileOptions options = System.IO.FileOptions.None, long preallocationSize = 0);
    [EditorBrowsable(EditorBrowsableState.Never)]
    public static SafeFileHandle OpenHandle (string path, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize = 0); // Existing API, marked as Never Browsable to add a new default parameter.

    // Get/Set from SafeHandle, (.NET 7) https://github.com/dotnet/runtime/issues/20234
    public static UnixFileMode GetUnixFileMode(SafeFileHandle fileHandle);
    public static void SetUnixFileMode(SafeFileHandle fileHandle, UnixFileMode mode);
}

class FileSystemInfo
{
    // Get/Set from string path
    public UnixFileMode UnixFileMode { get; set; }
}

// Set mode when creating file, returns FileStream (by `File.Open`).
class FileStreamOptions
{
    // Set mode when creating file
    public UnixFileMode UnixCreateMode { get; set; } = UnixFileMode.DefaultFileOpenPermissions;
}

// `UnixFileMode` replaces `TarFileMode` (https://github.com/dotnet/runtime/issues/65951)
namespace System.Formats.Tar
{
-    enum TarFileMode { ... }
}

Notes

On Windows/non-Unix:

  • The unixCreateMode arg on Directory.CreateDirectory/File.OpenHandle and FileStreamOptions.UnixCreateMode are ignored. These APIs can be used in in cross-platform code.

  • FileSystemInfo.UnixFileMode get returns UnixFileMode.None.

  • FileSystemInfo.UnixFileMode set throws PNSE.

APIs that create a file/directory (Directory.CreateDirectory, FileStreamOptions.UnixCreateMode, File.OpenHandle) have 'POSIX behavior':

  • If the file/directory exists, the mode argument is ignored.

  • The OS kernel filters the provided mode by the process umask. The umask protects users from unintentionally opening up permissions. A regular Linux user has a umask of 0002 which prevents it from giving write permissions to other. Use-cases that require the exact mode, must make an additional call to SetUnixFileMode. See example 3.

  • note: creating and setting permissions is atomic to ensure permissions are applied immediately

UnixFileMode can replace TarFileMode introduced in #65951.

AccessPermissionMask, DefaultFileOpenPermissions enum values.

  • These are convenient masks. Example usage of these masks:
    // Only extract USR, GRP, and OTH file permissions, and ignore
    // S_ISUID, S_ISGID, and S_ISVTX bits.
    // It is off by default because it's possible that a file in an archive could have
    // one of these bits set and, unknown to the person extracting, could allow others to
    // execute the file as the user or group.
    const int ExtractPermissionMask = 0x1FF;
    int permissions = (int)Mode & ExtractPermissionMask;
    and
    // If the file gets created a new, we'll select the permissions for it. Most Unix utilities by default use 666 (read and
    // write for all), so we do the same (even though this doesn't match Windows, where by default it's possible to write out
    // a file and then execute it). No matter what we choose, it'll be subject to the umask applied by the system, such that the
    // actual permissions will typically be less than what we select here.
    private const Interop.Sys.Permissions DefaultOpenPermissions =
    Interop.Sys.Permissions.S_IRUSR | Interop.Sys.Permissions.S_IWUSR |
    Interop.Sys.Permissions.S_IRGRP | Interop.Sys.Permissions.S_IWGRP |
    Interop.Sys.Permissions.S_IROTH | Interop.Sys.Permissions.S_IWOTH;
    .

API Usage

Example 1: limit a file to be read/written by the owner only:

File.SetUnixFileMode("filename", UnixFileMode.UserRead | UnixFileMode.UserWrite);

Example 2: exclude 'Other' access to entries in a directory:

var di = new DirectoryInfo("somedir");
foreach (var fi in di.GetFileSystemInfos())
{
  fi.UnixFleMode = fi.UnixFileMode & ~(UnixFileMode.OtherExecute | UnixFileMode.OtherRead | UnixFileMode.OtherWrite);
}

Example 3: create new files/directories while extracting a tar file with the exact permissions of the tar file.

if (entry.EntryType == EntryType.Directory)
{
    DirectoryInfo directoryInfo = Directory.CreateDirectory(path, entry.Mode); // filtered by umask
    di.UnixFileMode = entry.Mode;
}
else if (entry.EntryType == EntryType.RegularFile)
{
    using var file = File.OpenHandle(path, entry.Mode); // filtered by umask
    File.SetUnixFileMode(file, entry.Mode);
}

Alternative Designs

No response

Risks

No response

@tmds tmds added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 11, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.IO untriaged New issue has not been triaged by the area owner labels Apr 11, 2022
@ghost
Copy link

ghost commented Apr 11, 2022

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Add direct support for getting/setting Unix file permissions and special bits.

These APIs allow to restrict/extend access to files and directories from .NET.

There have been some requests for these APIs: #925, #928, #17540.

The tar implementation can make use of them (#65951) without resorting to internal APIs.

API Proposal

namespace System.IO;

[System.FlagsAttribute]
public enum UnixFileMode
{
// From https://github.com/dotnet/runtime/issues/65951.
    None = 0,
    OtherExecute = 1,
    OtherWrite = 2,
    OtherRead = 4,
    GroupExecute = 8,
    GroupWrite = 16,
    GroupRead = 32,
    UserExecute = 64,
    UserWrite = 128,
    UserRead = 256,
    StickyBit = 512,
    GroupSpecial = 1024, // or: SetGroup
    UserSpecial = 2048,  // or: SetUser

// Maybe:
    AccessPermissions = {User,Group,Other}{Execute,Read,Write}
}

static partial class Directory
{
    // Set mode when creating
    public static DirectoryInfo CreateDirectory(string path, UnixFileMode fileMode);

    // Get/Set using path
    public static UnixFileMode GetUnixFileMode(string path);
    public static void SetUnixFileMode(string path, UnixFileMode fileMode);
}

static partial class File
{
    // Set mode when creating
    public static SafeFileHandle OpenHandle(string path UnixFileMode fileMode, System.IO.FileMode mode = System.IO.FileMode.Create, System.IO.FileAccess access = System.IO.FileAccess.Write, System.IO.FileShare share = System.IO.FileShare.None, System.IO.FileOptions options = System.IO.FileOptions.None, long preallocationSize = (long)0);

    // Get/Set using path
    public static UnixFileMode GetUnixFileMode(string path);
    public static void SetUnixFileMode(string path, UnixFileMode fileMode);

    // Get/Set using SafeHandle, (.NET 7) https://github.com/dotnet/runtime/issues/20234
    public static UnixFileMode GetUnixFileMode(SafeFileHandle fileHandle);
    public static void SetUnixFileMode(SafeFileHandle fileHandle, UnixFileMode fileMode);
}

class FileSystemInfo
{
    // Get/Set
    public UnixFileMode UnixFileMode { get; set; }
}

Notes

On Windows/non-Unix:

  • The argument for fileMode is ignored on Directory.CreateDirectory and File.OpenHandle to allow these APIs to be used in cross-platform code.

  • {Get/Set}UnixFileMode methods throw PNSE.

  • FileSystemInfo.UnixFileMode get returns UnixFileMode.None.

  • FileSystemInfo.UnixFileMode set throws PNSE.

APIs that create a file/directory (Directory.CreateDirectory, File.OpenHandle) have 'POSIX behavior':

  • If the file/directory exists, the mode argument is ignored.

  • The mode argument is filtered by the process umask.

File.OpenHandle

  • If the FileMode never creates a file (it does not match: FileMode.*Create*), ArgumentException is thrown.

  • note: FileMode, FileAccess, FileShare have sensible defaults which are different from existing OpenHandle.

API Usage

Example 1: limit a file to be read/written by the owner only:

File.SetUnixFileMode("filename", UnixFileMode.UserRead | UnixFileMode.UserWrite);

Example 2: exclude 'Other' access to entries in a directory:

var di = new DirectoryInfo("somedir");
foreach (var fi in di.GetFileSystemInfos())
{
  fi.UnixFleMode = fi.UnixFileMode & ~(UnixFileMode.OtherExecute | UnixFileMode.OtherRead | UnixFileMode.OtherWrite);
}

Example 3: create new files/directories while extracting a tar file with the exact permissions of the tar file.

if (entryType == EntryType.Directory)
{
    DirectoryInfo directoryInfo = Directory.CreateDirectory(path, entry.Mode); // restricted by umask
    di.Mode = entry.Mode;
}
else
{
    using var file = File.OpenHandle(path, entry.Mode); // restricted by umask
    File.SetUnixFileMode(file, entry,Mode);
}

Alternative Designs

No response

Risks

No response

Author: tmds
Assignees: -
Labels:

api-suggestion, area-System.IO, untriaged

Milestone: -

@tmds
Copy link
Member Author

tmds commented Apr 11, 2022

@eerhardt @carlossanlop @stephentoub @dotnet/area-system-io ptal.

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Apr 11, 2022

File.OpenHandle with UnixFileMode looks useful for fixing the race condition AzureAD/microsoft-authentication-extensions-for-dotnet#174 and could simplify NuGetExtractionFileIO.

@hez2010
Copy link
Contributor

hez2010 commented Apr 11, 2022

On Windows/non-Unix:

Shouldn't those API be put in a separate TFM net7.0-linux or net7.0-unix since they are not available on Windows platforms? In this way a warning can also be generated during compilation due to TFM mismatch, if users are trying to use unix APIs on Windows.

@eerhardt
Copy link
Member

eerhardt commented Apr 11, 2022

Shouldn't those API be put in a separate TFM net7.0-linux or net7.0-unix since they are not available on Windows platforms?

Those TFMs don't exist. We don't have TFMs for -linux or -unix.

But instead we can use the SupportedOSPlatform and UnsupportedOSPlatform attributes that work with https://docs.microsoft.com/dotnet/standard/analyzers/platform-compat-analyzer. Then you will get CA1416 warnings when you try using APIs that are unsupported in code that could run on Windows.

@eerhardt
Copy link
Member

This looks great @tmds. Thanks for writing this up. I have the following questions:

  1. Should we put the UnixFileMode on File.Create instead of File.OpenHandle?
  2. In Example 3, why are we setting the UnixFileMode twice? Once on File.OpenHandle and then again with File.SetUnixFileMode?
  3. The mode argument is filtered by the process umask.

    • Do we need a way to set umask from .NET? Or is this not a typical usage scenario in an app?
  4. Do we want convenience Flags values for UnixFileMode? For example, UserExecute | UserWrite | UserRead and OtherExecute | OtherWrite | OtherRead.

@tmds
Copy link
Member Author

tmds commented Apr 11, 2022

Should we put the UnixFileMode on File.Create instead of File.OpenHandle?

I'll add it in addition to the OpenHandle method. They can be considered independent during API review.

In Example 3, why are we setting the UnixFileMode twice? Once on File.OpenHandle and then again with File.SetUnixFileMode?

When you provide a mode to open, the kernel applies the umask so the effective mode for the file may be more restricted.
When you provide a mode to chmod, the umask is not applied.

The .NET example makes two calls. One for open, one for fchmod. The second one is to undo the effect of the umask.

If we don't want that, we can perform an fchmod as part of the first .NET call.

Consequently, if the file already exists (e.g. FileMode.OpenOrCreate), we'd be changing its permissions. That may be unexpected.

Do we need a way to set umask from .NET? Or is this not a typical usage scenario in an app?

The umask is process wide, so it doesn't work well if several threads are setting it to a value they need.

@eerhardt
Copy link
Member

Do we need a way to set umask from .NET? Or is this not a typical usage scenario in an app?

The umask is process wide, so it doesn't work well if several threads are setting it to a value they need.

I meant a public API that .NET developers could call to set the umask.

@tmds
Copy link
Member Author

tmds commented Apr 11, 2022

I meant a public API that .NET developers could call to set the umask.

Because .NET is multi-threaded, it would end badly.

One thread doing 'untar', would set it to umask 0.

While another thread doing File.OpenWrite would end up creating a file that is readable and writable by other, because umask became 0 instead of 2.

@eerhardt
Copy link
Member

Because .NET is multi-threaded, it would end badly.

I can write multi-threaded C code. Does it end badly there as well?

One thread doing 'untar', would set it to umask 0.

I'm not talking about tar. I'm talking about any .NET application. umask is a system-level API that allows applications to manipulate file modes. Do .NET applications need to control it?

@tmds
Copy link
Member Author

tmds commented Apr 11, 2022

I can write multi-threaded C code. Does it end badly there as well?

Yes.

One thread doing 'untar',

By this I meant, use the API to untar a file: #65951.

Do .NET applications need to control it?

I don't think there are strong use-cases for this. They should use the SetUnixFileMode to avoid multi-threading issues with umask.

@tmds
Copy link
Member Author

tmds commented Apr 11, 2022

I had a look. nodejs and python APIs that create files preserve the default behavior of applying umask.

Both also provide a umask function. Still, I don't think we should add it in .NET because of the multi-threading issues. The other APIs allow to set the permissions in a thread-safe way without using umask.

@tmds
Copy link
Member Author

tmds commented Apr 11, 2022

Should we put the UnixFileMode on File.Create

Something like:

    public static FileStream Create(string path, UnixFileMode permissions, FileOptions options = FileOptions.None)
    	=> Create(path, permissions, DefaultBufferSize, options);
    public static FileStream Create(string path, UnixFileMode permissions, int bufferSize, FileOptions options = FileOptions.None)
    	=> new FileStream(OpenHandle(path, permissions, FileMode.Create, FileAccess.ReadWrite, FileShare.None, options), FileAccess.ReadWrite, bufferSize);

The FileMode/FileAccess match the existing Create but they are different from what I had picked for OpenHandle.

@eerhardt what do you think?

@KalleOlaviNiemitalo
Copy link

Setting umask in ProcessStartInfo would be safe, but if an application cares about that, then it likely wants to control ulimit and file descriptors as well, so just adding umask support there would not be worthwhile.

@jkotas
Copy link
Member

jkotas commented Apr 11, 2022

UnixFileMode on File.Create

We have introduced FileStreamOptions that aggregate all file creation options to avoid the ever-growing list of overloads. Should the UnixFileMode be added on FileStreamOptions instead?

@tmds
Copy link
Member Author

tmds commented Apr 12, 2022

I've included it as a FileStreamOption, and extended the notes on the umask masking performed by the kernel.

@carlossanlop
Copy link
Member

Thank you for proposing this. Getting UnixFileMode approved and implemented in 7.0 with all these usages would allow us to remove TarFileMode, which is what ended up being approved here (instead of UnixFileMode): #65951

I think this proposal should explicitly mention that TarFileMode would be removed once UnixFileMode is approved. @bartonjs agree?

@eerhardt
Copy link
Member

I think this proposal looks good enough to be marked ready for review. Thoughts @adamsitnik @Jozkee?

@eerhardt
Copy link
Member

Should we put the UnixFileMode on File.Create

Looking again, File.Create is really a convenience API that doesn't have all the possible options. For example, there is no File.Create that lets you specify a FileShare. Since setting the UnixFileMode is more of an advanced API, I don't think we need an overload to File.Create that takes a UnixFileMode. Devs can use one of the above proposed APIs new FileStream or File.OpenHandle if they need to create a file with a specific UnixFileMode.

@tmds
Copy link
Member Author

tmds commented Apr 19, 2022

I think this proposal should explicitly mention that TarFileMode would be removed once UnixFileMode is approved.

I've added this to the proposal.

@tmds
Copy link
Member Author

tmds commented Apr 22, 2022

I think this proposal looks good enough to be marked ready for review. Thoughts @adamsitnik @Jozkee?

@adamsitnik @Jozkee do you have feedback? Can this move to ready for review?

@Jozkee
Copy link
Member

Jozkee commented Apr 25, 2022

// Set mode when creating
public static DirectoryInfo CreateDirectory(string path, UnixFileMode unixCreateMode);

Does the UnixFileMode sets atomically when the file or directory is created? If not, I think having only FileSystemInfo.UnixFileMode { get; set; } could suffice to address the same scenarios; but even if it sets atomically, it feels weird that the only overload to CreateDirectory takes a UnixFileMode but not any of the other properties already exposed.

// Set mode when creating
public static SafeFileHandle OpenHandle(string path, UnixFileMode unixCreateMode, FileMode mode = FileMode.CreateNew, FileAccess access = FileAccess.Write, FileShare share = FileShare.None, FileOptions options = FileOptions.None, long preallocationSize = 0);

As you are already expanding FileStreamOptions, File.Open(string, FileStreamOptions) already covers file creation scenarios and you can still get the handle from the returning FileStream. I think this API is not needed.

// Maybe:
AccessPermissions = {User,Group,Other}{Execute,Read,Write}

Can you elaborate on this?

@tmds
Copy link
Member Author

tmds commented Apr 26, 2022

Does the UnixFileMode sets atomically when the file or directory is created? If not, I think having only FileSystemInfo.UnixFileMode { get; set; } could suffice to address the same scenarios; but even if it sets atomically, it feels weird that the only overload to CreateDirectory takes a UnixFileMode but not any of the other properties already exposed.

Yes, it's atomic, and it is needed to restrict permissions immediately on creation.

As you are already expanding FileStreamOptions, File.Open(string, FileStreamOptions) already covers file creation scenarios and you can still get the handle from the returning FileStream. I think this API is not needed.

I'll keep it in the proposal, and let the API reviewers kick it out if they want.

AccessPermissions = {User,Group,Other}{Execute,Read,Write}

This is a mask to filter out the access permissions. It could be left out. This is an example use-case:

// Only extract USR, GRP, and OTH file permissions, and ignore
// S_ISUID, S_ISGID, and S_ISVTX bits.
// It is off by default because it's possible that a file in an archive could have
// one of these bits set and, unknown to the person extracting, could allow others to
// execute the file as the user or group.
const int ExtractPermissionMask = 0x1FF;
int permissions = (int)Mode & ExtractPermissionMask;

I've updated the proposal to include your feedback.

@tmds
Copy link
Member Author

tmds commented Apr 27, 2022

@Jozkee @adamsitnik when it looks good to you, can you move the issue to ready to review?

@Jozkee Jozkee added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Apr 28, 2022
@tmds
Copy link
Member Author

tmds commented May 2, 2022

However, I think methods that take a FileStreamOptions parameter should ignore the UnixFileMode if FileStreamOptions.Mode does not allow creating a file.

That's right. Otherwise all FileStreamOptions that don't have a *Create* mode would stop working.

@adamsitnik
Copy link
Member

Do we really need GetUnixFileMode and SetUnixFileMode for Directory, File and FileSystemInfo? Since it's not a cross-platform API I would expect that extending FileSystemInfo with the property should be enough.

static partial class Directory
{
-    public static UnixFileMode GetUnixFileMode(string path);
-    public static void SetUnixFileMode(string path, UnixFileMode mode);
}

static partial class File
{
-    public static UnixFileMode GetUnixFileMode(string path);
-    public static void SetUnixFileMode(string path, UnixFileMode mode);
}

class FileSystemInfo
{
    public UnixFileMode UnixFileMode { get; set; }
}

An alternative it to extend Path with two new methods (similar to Path.Exists)

note: FileMode, FileAccess, FileShare have sensible defaults which are different from existing OpenHandle

This might confuse some of our users, moreover I am not sure which overload IDE like VS is going to suggest first. If the one that accepts UnixFileMode which is not a cross-platform concept and might throw on Windows then it's bad. I would rather not add SafeFileHandle methods in the first iteration.

static partial class File
{
-    public static SafeFileHandle OpenHandle(string path, UnixFileMode unixCreateMode, FileMode mode = FileMode.CreateNew, FileAccess access = FileAccess.Write, FileShare share = FileShare.None, FileOptions options = FileOptions.None, long preallocationSize = 0);

-    public static UnixFileMode GetUnixFileMode(SafeFileHandle fileHandle);
-    public static void SetUnixFileMode(SafeFileHandle fileHandle, UnixFileMode mode);
}

@tmds
Copy link
Member Author

tmds commented May 5, 2022

Do we really need GetUnixFileMode and SetUnixFileMode for Directory, File and FileSystemInfo? Since it's not a cross-platform API I would expect that extending FileSystemInfo with the property should be enough.

I'll leave these out.

var mode = File.GetUnixFileMode(path);

becomes

var mode = new FileSystemInfo(path).UnixFileMode

I am not sure which overload IDE like VS is going to suggest first.

I'll leave it out.

C# doesn't lend itself to extending an existing method with additional default arguments.

Without this overload, there is no way to create a SafeFileHandle without having to carry along a FileStream (for the IDisposable).

using SafeFileHandle handle = File.OpenHandle(path, unixCreateMode: mode);

becomes

using FileStream fs = new FileStream(path, new FileStreamOptions { UnixCreateMode = mode });
SafeFileHandle handle = fs.SafeFileHandle;

I would rather not add SafeFileHandle methods in the first iteration.

Operating on a string path or a SafeFileHandle are functionally different. I'm going to keep these in the proposal because they match with the use-cases for #20234.

API reviewers can still kick them out.

@tmds
Copy link
Member Author

tmds commented May 5, 2022

These come from #65951. Perhaps @carlossanlop can elaborate why the names contain Special? I included the alternative names SetGroup/SetUser. @carlossanlop wdyt about these alternatives?

@carlossanlop can I replace the Special* enum names with the Set* names?

@bartonjs
Copy link
Member

bartonjs commented May 5, 2022

C# doesn't lend itself to extending an existing method with additional default arguments.

There's a pattern for it, with the work on the library author.

  • Add the signature of the new method with all of the defaulted parameters
  • Remove the defaults from the existing member, and make it defer to the new, longer method.
  • Mark the existing member [EditorBrowsable(EditorBrowsableState.Never)]

It doesn't work on virtuals, or for interfaces, but our guidelines say not to use defaulted parameters for either of those cases (except for a CancellationToken parameter)

Any existing compilations will call the old method, which just calls the new. Any new compilations will prefer the new method, unless they happened to be specifying every argument in their code, then they recompile to exactly what they were.

@tmds
Copy link
Member Author

tmds commented May 5, 2022

@bartonjs thanks, I'll add back the OpenHandle overload using this pattern.

@carlossanlop
Copy link
Member

can I replace the Special* enum names with the Set* names?

I welcome any name that would make the most sense, @tmds.

@bartonjs
Copy link
Member

bartonjs commented May 5, 2022

Putting my API Review hat on a bit early... "But isn't it only SetUID/SetGID for (executable) files, and it means something different, like 'sticky' on directories? I think that the POSIX header comments call it 'special', that seems like the more general name."

Just sharing 😄

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented May 6, 2022

https://man7.org/linux/man-pages/man0/sys_stat.h.0p.html shows descriptions of those bits copied from POSIX, I believe. They don't involve "special".

The S_ISGID bit has been used for:

  • On executable files, set the group ID of the process when executed.
  • On files not executable by the group, replace fcntl()/lockf() advisory locking with mandatory locking.
  • On directories, make new files and subdirectories inherit the group ID from this directory, not from the process.

(I wonder if the bit has been used for anything on block devices or character devices… like biff sets S_IXUSR and S_IXGRP on a tty.)

@tmds
Copy link
Member Author

tmds commented May 6, 2022

I'm going to replace the names in the proposal by SetUser/SetGroup because I think those are the most well-known names for these bits, and they represent the most common use-case.

API reviewers can make the final call.

@bartonjs
Copy link
Member

bartonjs commented May 6, 2022

Hm. Clearly some bad data got shoved into my head at some point.

New data accepted, position changed. Rename away 😄

@eerhardt
Copy link
Member

I'm going to replace the names in the proposal by SetUser/SetGroup because I think those are the most well-known names for these bits, and they represent the most common use-case.

It looks like golang calls them (roughly) the same: ModeSetuid and ModeSetgid. https://pkg.go.dev/io/fs#FileMode

I don't see Java having names for these. https://docs.oracle.com/javase/8/docs/api/java/nio/file/attribute/PosixFilePermission.html. Looks like there are just Read/Write/Execute for Group/Others/Owner.

@terrajobst
Copy link
Member

terrajobst commented May 24, 2022

Video

  • Let's remove Directory.OpenHandle
    • We don't have an OpenHandle on Directory today
  • File
    • Let's add overloads for string path
  • Let's also remove File.OpenHandle
    • We don't have an overload today that allows setting Windows ACLs, it's a low level API
    • This would also have implications for if we use the
  • None of the APIs will be marked as Linux only as it seems we can and should make them work on Windows by setting WSL related metadata in NTFS
  • UnixFileMode
    • Let's remove the mask because it messes with ToString()
namespace System.IO;

[System.FlagsAttribute]
public enum UnixFileMode
{
    None = 0,
    OtherExecute = 1,
    OtherWrite = 2,
    OtherRead = 4,
    GroupExecute = 8,
    GroupWrite = 16,
    GroupRead = 32,
    UserExecute = 64,
    UserWrite = 128,
    UserRead = 256,
    StickyBit = 512,
    SetGroup = 1024,
    SetUser = 2048
}
public partial class Directory
{
    public static DirectoryInfo CreateDirectory(string path, UnixFileMode unixCreateMode);
}
public partial class File
{
    public static UnixFileMode GetUnixFileMode(string path);
    public static UnixFileMode GetUnixFileMode(SafeFileHandle fileHandle);
    public static void SetUnixFileMode(string path, UnixFileMode mode);
    public static void SetUnixFileMode(SafeFileHandle fileHandle, UnixFileMode mode);
}
public partial class FileSystemInfo
{
    public UnixFileMode UnixFileMode { get; set; }
}
public partial class FileStreamOptions
{
    public UnixFileMode? UnixCreateMode { get; set; }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 24, 2022
@stephentoub
Copy link
Member

public static DirectoryInfo CreateDirectory(string path, UnixFileMode unixCreateMode);

Do we need an equivalent for creating a file? Otherwise, with the APIs I see approved above, it doesn't seem like there's a way for me to create a file and atomically have it have the "right" permissions, e.g. I might want a file locked down to just user, but I'll end up creating a file with the default permissions and by the time I've restricted them, someone else could have accessed it.

(Sorry if this was discussed; had to leave the meeting early...)

@Jozkee
Copy link
Member

Jozkee commented May 24, 2022

Do we need an equivalent for creating a file? Otherwise, with the APIs I see approved above, it doesn't seem like there's a way for me to create a file and atomically have it have the "right" permissions

FileStreamOptions options = new()
{
    Mode = FileMode.Create,
    UnixCreateMode = UnixFileMode.UserRead | UnixFileMode.UserWrite
}

File.Open(filePath, options).Dispose();

@deeprobin
Copy link
Contributor

Why is there no OSPlatformAttribute proposed? If I understand this right, this would only work on Unix-systems.

@tmds
Copy link
Member Author

tmds commented May 25, 2022

@Jozkee @adamsitnik @eerhardt you can assign this to me.

Why is there no OSPlatformAttribute proposed? If I understand this right, this would only work on Unix-systems.

Let's see where it makes sense as part of the PR.

@stephentoub
Copy link
Member

Why is there no OSPlatformAttribute proposed?

"None of the APIs will be marked as Linux only as it seems we can and should make them work on Windows by setting WSL related metadata in NTFS"

@stephentoub
Copy link
Member

FileStreamOptions

Ah, yes, thanks.

@deeprobin
Copy link
Contributor

Why is there no OSPlatformAttribute proposed?

"None of the APIs will be marked as Linux only as it seems we can and should make them work on Windows by setting WSL related metadata in NTFS"

Thanks. I overread that 🤦🏼‍♂️.

@danmoseley danmoseley removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 26, 2022
@teo-tsirpanis
Copy link
Contributor

Fixed by #69980.

@webczat
Copy link
Contributor

webczat commented Jun 25, 2022

no chown/chgrp? anyway thanks for this one!

@dotnet dotnet locked as resolved and limited conversation to collaborators Jul 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.IO blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

No branches or pull requests