transaction changes default group/guest permissions

[brandon-smith-xperi]

Hi,

Saving a file via fsspec.open() results in default (macOS) permissions 644.
Saving a file via fs.transaction, fsopen() strips group and guest permissions to 600.

Is this intended?

Here's a code snippet:

import fsspec

# correct permissions 644
with fsspec.open('correct_permissions.bin', 'wb') as f:
    f.write(b'hello')

# incorrect permissions 600
fs, urlpath = fsspec.core.url_to_fs('incorrect_permissions.bin')
with fs.transaction, fs.open(urlpath, 'wb') as f:
    f.write(b'hello')

Thanks!

[martindurant]

Thanks for letting us know, and I am surprised!
Do you know where the default is defined?

The implementation for finalising a transation uses shutil.mv to put the complete file in its required position. Perhaps some reading of what that means will help us to understand what's going on.

If your temporary directory and the output path on the same storage device?

[brandon-smith-xperi]

Thanks @martindurant,

• Sorry, I'm not sure where the default is defined.
• I am working on the same storage device.
• I tried a simple shutil.move() and it works as expected (preserving 644 permissions).

Are you able to reproduce the issue with the original code snippet?

[martindurant]

Yes, I do see the same behaviour.

I believe the culprit is tempfile.mkstemp() which is used to make the file before committing it. I suppose it isn't unreasonable that explicitly temp files should have different permissions to regular files.

[martindurant]

https://docs.python.org/3/library/tempfile.html#tempfile.mkstemp

The file is readable and writable only by the creating user ID

At commit time, we can change the permission bits, but it would still be good to know where 644 comes from, in case that might be configured differently on some system; also, what all this does on Windows.

[brandon-smith-xperi]

Maybe something like this is helpful?
https://stackoverflow.com/questions/7150826/how-can-i-get-the-default-file-permissions-in-python

I have no idea what happens on Windows.

[martindurant]

Ah, it's right then in the definition:

os.open(path, flags, mode=511, *, dir_fd=None)

So I would suggest adding this as a PR, if you would like to:

--- a/fsspec/implementations/local.py
+++ b/fsspec/implementations/local.py
@@ -413,6 +413,10 @@ class LocalFileOpener(io.IOBase):
         if self.autocommit:
             raise RuntimeError("Can only commit if not already set to autocommit")
         shutil.move(self.temp, self.path)
+        try:
+            os.chmod(self.path, 511)
+        except RuntimeError:
+            pass

(I'm not actually sure what exception you can get except FileNotFOund, which should never happen here).

[brandon-smith-xperi]

I think 511 == 0o777, so that will change the file permissions to 777.

[martindurant]

You are right in the conversions, but

In [ ]: os.open("afile", flags=os.O_CREAT, mode=511)
In [ ]: %ls -l afile
-rwxr-xr-x  1 mdurant  staff  0 12 Mar 10:28 afile*

you see no "w" for group or all.

EDIT

In [ ]: os.chmod("afile", 511)
In [ ]: %ls -l afile
-rwxrwxrwx  1 mdurant  staff  0 12 Mar 10:28 afile*

(so O_CREAT takes prescedence in open(), and we need to find where that is defined!)

[martindurant]

More info: fopen() in posix says:

Any created file will have the mode S_IRUSR | S_IWUSR | S_IRGRP |
S_IWGRP | S_IROTH | S_IWOTH (0666), as modified by the process's
umask value (see umask(2))

and this is what builtins open() does. This is 0o666 - 0o022 -> 0o655, -rw-r--r-- (minus here means "set to zero if not zero", i.e., && (not x) ).,

os.open(), by contrast, is doing 0o777 - 0o022 where 777 is the default mode and 0o022 is my umask.

TL;DR: should we set to 0o655 as the most likely value? Please do make a PR to do this.