FileDescriptor creation do not respect Group Write permissions

[makadaw]

When open file descriptor for write and set option .create created file does not have group write permission. Checked with all group permissions .groupWrite, .groupReadWrite, .groupReadWriteExecute

In case below, expect to have permissions 0020, but get 0000

FileDescriptor.open(filePath,
                                  .writeOnly,
                                  options: [.create],
                                  permissions: [.groupWrite])

System: MacOS 10.15.7
Swift toolchain: Swift Development Snapshot 2021-02-16
SwiftPackage revision 2e9c1a71185c828416751283b40697725da550b6

[milseman]

Weird. I opened #27 to ensure that we are properly mapping to the right open syscall (and I verified with a breakpoint as well).

At this point, it would be an issue with how the OS handles the open syscall. I'll figure out what the next step is for creating an issue against the OS. Could you provide precise context on what you were doing? E.g., what was the state of the filesystem prior to and after the call?

[weissi]

This is how UNIX works and expected assuming that your umask has the 0o20 bit set which is likely because the default umask is 0o022. It masks the write permissions for group/other out unless you change umask.

This is also documented. See for example here

The effective mode is modified by the process's umask in
the usual way: in the absence of a default ACL, the mode
of the created file is (mode & ~umask).

Darwin's man 2 open has to say

The oflag argument may indicate that the file is to be created if it does not exist (by specifying the O_CREAT flag). In this
case, open() and openat() require an additional argument mode_t mode; the file is created with mode mode as described in
chmod(2) and modified by the process' umask value (see umask(2)).

And here are the docs for the default umask:

The typical default value for the process umask is
S_IWGRP | S_IWOTH (octal 022). In the usual case where the mode
argument to open(2) is specified as:

S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH

(octal 0666) when creating a new file, the permissions on the
resulting file will be:

S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH

(because 0666 & ~022 = 0644; i.e., rw-r--r--).

[weissi]

If you want to actually want permissions to be the effective mode, you'd need to chmod too. Maybe permissions isn't the best name for the argument in open? Should probably be mode. Especially given that not only permissions are set with this argument but also stuff setuid/setgid.

[makadaw]

@weissi, now it makes more sense. I can also check tomorrow how S_IWOTH works to confirm the behavior.

@milseman I really just call code from my message and then check file status. The file did not exist before I call it and created it in the tmp folder.

[milseman]

This is how UNIX works and expected assuming that your umask has the 0o20 bit set which is likely because the default umask is 0o022

Well, this seems like pretty strong rationale to add umask support to System in the very near term, alongside chmod.

Maybe permissions isn't the best name for the argument in open? Should probably be mode.

I would really like to avoid the very generic term "mode" if at all possible. If I didn't know the convention and the C mode_t type, I would assume "mode" referred to read vs write ("access mode"), or even append vs replace. We should definitely call this out in the docs and probably consider a more descriptive argument label ("requestedPermissions"?).

IIUC, the argument is a requested file permissions, from which umask bits are subtracted. Since FilePermissions is an OptionSet (and thus SetAlgebra), there should be ways to specify and model this. Is there any way to query this mask without overwriting it? Otherwise, we may only be able to do something like this:

extension FilePermissions {

  /// Set the process's file mode creation mask to `restricted`, which turns off
  /// any present options during calls to `open`, `mkdir`, `mkfifo`, or `mknod`.
  ///
  /// Returns the previous value of the file mode mask.
  ///
  /// The corresponding C function is `umask`.
  public static func setFileCreationMask(
    removing restricted: FilePermissions
  ) -> FilePermissions {
    FilePermissions(rawValue: umask(restricted.rawValue))
  }
}

If we had a Process type or some such, it could be hosted there instead. Off the cuff, having it be an instance method on FilePermissions would feel weird, so it is static.

Especially given that not only permissions are set with this argument but also stuff setuid/setgid.

Is the argument that type type FilePermissions also needs to be changed? I do agree that the other bits (e.g. saveText) are oddballs, but I'm not sure a different name would be better.

[weissi]

This is how UNIX works and expected assuming that your umask has the 0o20 bit set which is likely because the default umask is 0o022

Well, this seems like pretty strong rationale to add umask support to System in the very near term, alongside chmod.

Hmm, I mean you're typically not supposed to ignore umask unless the user truly asked you to set a specific mode. And changing the umask is really very rare, don't think I've ever done it in a program, only on the shell using the umask binary. So we should be aware of umask but not sure if we need to give the user access right now, other things feel much more important to me.

Maybe permissions isn't the best name for the argument in open? Should probably be mode.

I would really like to avoid the very generic term "mode" if at all possible. If I didn't know the convention and the C mode_t type, I would assume "mode" referred to read vs write ("access mode"), or even append vs replace. We should definitely call this out in the docs and probably consider a more descriptive argument label ("requestedPermissions"?).

Sounds good! What about setuid/setgid/sticky bit and friends though? They don't feel like permissions (to me) but can still be used in open's mode argument (to some capacity, YMMV).

IIUC, the argument is a requested file permissions, from which umask bits are subtracted.

Sort of, I think they are mostly subtracted but I think setuid/setgid also also typically subtracted on top. But the sticky bit usually isn't. I'm not sure anymore, has been a long time since I looked into that but the bottom line is that I think there is extra magic going on in the kernel after subtracting umask.

Since FilePermissions is an OptionSet (and thus SetAlgebra), there should be ways to specify and model this. Is there any way to query this mask without overwriting it?

Nope, you'd need to do let oldMask = umask(0); umask(oldMask) to just read it (by writing the old value back). Docs here

---

It is impossible to use umask() to fetch a process's umask
without at the same time changing it. A second call to umask()
would then be needed to restore the umask. The nonatomicity of
these two steps provides the potential for races in multithreaded
programs.

Since Linux 4.7, the umask of any process can be viewed via the
Umask field of /proc/[pid]/status. Inspecting this field in
/proc/self/status allows a process to retrieve its umask without
at the same time changing it.

---

.

Otherwise, we may only be able to do something like this:

extension FilePermissions {

  /// Set the process's file mode creation mask to `restricted`, which turns off
  /// any present options during calls to `open`, `mkdir`, `mkfifo`, or `mknod`.
  ///
  /// Returns the previous value of the file mode mask.
  ///
  /// The corresponding C function is `umask`.
  public static func setFileCreationMask(
    removing restricted: FilePermissions
  ) -> FilePermissions {
    FilePermissions(rawValue: umask(restricted.rawValue))
  }
}

If we had a Process type or some such, it could be hosted there instead. Off the cuff, having it be an instance method on FilePermissions would feel weird, so it is static.'

Yes, definitely feels better on Process because it affects the whole process.

Especially given that not only permissions are set with this argument but also stuff setuid/setgid.

Is the argument that type type FilePermissions also needs to be changed? I do agree that the other bits (e.g. saveText) are oddballs, but I'm not sure a different name would be better.

Good question if I'm arguing that or not. But the "sticky bit" or setuid/gid don't feel like "permissions" to me but maybe you could see them as such. For posterity, here are the docs for S_ISVTX which is commonly referred to as the "sticky bit"

The sticky bit (S_ISVTX) on a directory means that a file in that
directory can be renamed or deleted only by the owner of the
file, by the owner of the directory, and by a privileged process.

No idea what the sticky bit does on non-directories, if anything :). But yeah, the sticky bit is more like an anti-permission rather than a permission IYSWIM.

[milseman]

changing the umask is really very rare

The fact that we cannot atomically fetch umask is worrying. If a process changing it is very rare, then we can at least provide:

extension FilePermissions {
  /// The default process-wide file mode creation mask,, which turns off
  /// any present options during calls to `open`, `mkdir`, `mkfifo`, or `mknod`.
  ///
  /// The file mode creation mask can be changed with (TODO: `setFileCreationMask` or `umask`?)
  ///
  /// The corresponding C function is `umask`.
  public static var defaultFileCreationMask: FilePermissions {
    FilePermissions(rawValue: 0o22)
  }
}

Since it's not affecting anything, a static member on FilePermissions makes sense. Having a decl here also makes it easier to refer to from open's docs.

They don't feel like permissions (to me) ... Good question if I'm arguing that or not. But the "sticky bit" or setuid/gid don't feel like "permissions" to me

Hmm. From first principals, we want a RawRepresentable struct + OptionSet which provides a Swifty type-safe alternative to the weakly typed mode_t and or-ing global constants. We're looking at designing the interface to the type but not its core function or capabilities. It's fairly common that extra stuff gets retrofitted onto unused bits for these types. We care about the global namespace and this pertains to files, so the most general name we'd want to go with would probably be FileMode. I don't think we'd go with FileDescriptor.Mode as it's not really specific to descriptors the way access modes or cursor positions are.

I don't think the name FilePermissions is actively bad/harmful. But it is removed enough from mode_t that it might not be obvious that this is just the low-level wrapper type and not some higher-level library representation of the concept of permissions (which could be more nuanced than mode, e.g. ACL lists).

The best opportunity or argument we may have for a rename would be with Window support. @compnerd, comments on this?

IIUC, Windows has file attributes (File.GetAttributes), which is like the result of stat's S_IFMT (yet another thing packed into mode_t sometimes). The only real overlap WRT permissions is whether a file is read-only, and _wsopen_s can take the S_IWRITE option to make a newly created file writable (all files are readable). An alternate name could be FileAttributes (i.e. go with the Windows name), but I don't want to introduce confusion with chattr and xattr.

When we get around to providing stat/File.GetAttributes, we'd probably provide the result as a RawRepresentable struct and might want to use such a FileMode type for the corresponding field. mkdir produces undefined results if bits other than the bottom 9 are used, while mknod uses them. I'm uncertain what open does with the extra bits... We could definitely use some more type safety here. We could add a FileMode type which can be used for this purpose, and deprecate anything non-permissions-y inside FilePermissions, treating it as a good time to also fix the Windows permissions story.

Rust went with fs::Permissions, which wraps the mode/attributes, but provides only the intersection of the platforms as API (readOnly). Rust provides fs::set_permissions which will invoke chmod/SetFileAttributes.

[lorentey]

System calls its version of mode_t FilePermissions. If that's not the right name, that's unfortunate; but I don't think changing it would be a good idea at this point.

There is no expectation that mode_t would translate to Windows SDK APIs -- the Windows port ought to be able to lovingly import the corresponding concepts as true to their nature as possible.

[lorentey]

S_IFMT's packed bits cannot be modeled well with an OptionSet like FilePermissions anyway -- I think System's wrapper struct for struct stat just needs to model the file format with a standalone property.