Permissions::readonly() doesn't behave as expected with files

[Firstyear]

When using the Permissions module on MacOS it doesn't behave as expected. Readonly returns if the file lacks any write bits, rather than returning if the current calling uid/gid can write to the file. This can be seen in the code:

https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/fs.rs#L359

This is a bit misleading because the framing of https://doc.rust-lang.org/std/fs/struct.Permissions.html#method.readonly seems to say "an unwritable file" without context which to me I interpretted as "unwritable file - by the current user" not unwriteable by all permission bits.

I'd be happy with a disclaimer on the documentation of readonly to indicate this only checks that no write bits exist on unix so that it's clearer what it's checking for, but I'd also be happy if this function was altered to check 'is it readonly in the current calling context' but I understand that's a really challenging problem in-itself given posix acls etc.

I expected to see this happen: A file as root:root 640 should be readonly == true when called from "william:william"

Instead, this happened: The file is listed as "not" readonly even though the current user can not write the file OR the documentation about what readonly() does clearly explains what it is looking for on each operating system.

Thanks!

[nbdd0121]

Permissions::readonly does not (and cannot) perform any I/O operation to determine if the file is actually readable. It can only peek into the platform-specific permission representation (which is the "777" format on Unix). It also does not encode any information about owner user or group, so it couldn't even check if you are the current user/group.

I think the usage of name "read-only" is pretty clear that it implies nobody could write to it, not that the current user cannot write to it (as in read-only file system), but we could possibly improve the documentation to make it clearer.

@rustbot modify labels: +T-doc +C-enhancement -C-bug

[Firstyear]

Permissions::readonly does not (and cannot) perform any I/O operation to determine if the file is actually readable. It can only peek into the platform-specific permission representation (which is the "777" format on Unix). It also does not encode any information about owner user or group, so it couldn't even check if you are the current user/group.

Yep that's fine, it's pretty complex to work out if something really is readonly especially with ACLs having deny rules etc....

I think the usage of name "read-only" is pretty clear that it implies nobody could write to it, not that the current user cannot write to it (as in read-only file system), but we could possibly improve the documentation to make it clearer.

I think it's currently not clear because I interpreted this as "read-only to the calling users context" so making the documentation explicit in what occurs behind the scenes would be good.

[nbdd0121]

I think it's currently not clear because I interpreted this as "read-only to the calling users context" so making the documentation explicit in what occurs behind the scenes would be good.

But the doc does not mention about user at all? Permissions is retrieved from Metadata, which describes the nature about a file itself rather than how it can be interacted with.

[Timmmm]

I ran into this issue too. It's definitely not clear that readonly() will be false if any user/group/other write bit is set. As @Firstyear said that's just wrong, so it should at least be documented.

    pub fn readonly(&self) -> bool {
        // check if any class (owner, group, others) has write permission
        self.mode & 0o222 == 0
    }

Even accounting for the fact that it can't easily check if you are in the relevant group, sometimes having group membership and group write permission is not enough. E.g. to delete a file you need user write permission.

It's actually a bit worse than that - the set_readonly() implementation is weird too:

    pub fn set_readonly(&mut self, readonly: bool) {
        if readonly {
            // remove write permission for all classes; equivalent to `chmod a-w <file>`
            self.mode &= !0o222;
        } else {
            // add write permission for all classes; equivalent to `chmod a+w <file>`
            self.mode |= 0o222;
        }
    }

If you set_readonly(false) a file you actually make it world-writable! Probably not what was intended.

It may be too late to change these and honestly it's not clear how you would even fix them (I would say probably ignore the group/other bits). But either way we should definitely add a note saying that these functions almost certainly do not do what you want on Unix, and you should use std::os::unix::fs::PermissionsExt instead. I would go so far as to say they should be deprecated on Unix.

[gibfahn]

Just hit this issue. Needed to check if a file was writeable (equivalent of [ -w $file ] in shell) and assumed readonly would do what I wanted.

Fortunately Google put this issue as the second result.

Would also be nice for the readonly docs to include an example of how to actually check "can I write to this file?"

[Firstyear]

@gibfahn The issue on unix systems is posix acls, so you can't actually check very easily :(

[gibfahn]

Yeah I have now run into that. I ended up with this, no idea if it's correct:

/// Checks whether a path to an existing file can be written to.
/// If the file doesn't exist, this function will return false.
pub(crate) fn is_writeable(path: impl AsRef<Path>) -> bool {
    File::options()
        .write(true)
        // Make sure we don't accidentally truncate the file.
        .truncate(false)
        .open(path.as_ref())
        .is_ok()
}

[nbdd0121]

If you are only targeting POSIX, then you should be using the access function from libc.

[crumblingstatue]

I would also like this documented, because I (naively) expected this to work for determining whether the current user can write to the file.

[Timmmm]

Jinx!

[Firstyear]

@Timmmm I'm happy to merge our PR's or close mine in favour of yours, what do you prefer :)

[Timmmm]

I'll merge some of yours into mine if that's ok? I think your change is clearer that a) it's Unix-like that it applies to so it includes Mac, and b) the set_readonly() only applies to the in-memory setting.

[Firstyear]

Sounds great to me :)