Skip to content
This repository has been archived by the owner on Jul 16, 2022. It is now read-only.

option to preserve the permissions #42

Open
pabs3 opened this issue May 2, 2019 · 8 comments
Open

option to preserve the permissions #42

pabs3 opened this issue May 2, 2019 · 8 comments

Comments

@pabs3
Copy link

pabs3 commented May 2, 2019

When I overwrite a file, the permissions of the file are changed:

$ touch foo

$ chmod 0123 foo

$ stat foo | sed -n 's/^\(Access.*\)Uid.*$/\1/p'
Access: (0123/---x-w--wx)  

$ python3
Python 3.7.3rc1 (default, Mar 13 2019, 11:01:15) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from atomicwrites import atomic_write
>>> with atomic_write('foo', overwrite=True) as f:
...  f.write('foo')
... 
3
>>> 

$ stat foo | sed -n 's/^\(Access.*\)Uid.*$/\1/p'
Access: (0600/-rw-------)  

The normal non-atomic method of overwriting a file does not change the mode:

$ touch foo

$ chmod 0777 foo

$ stat foo | sed -n 's/^\(Access.*\)Uid.*$/\1/p'
Access: (0777/-rwxrwxrwx)  

$ python3
Python 3.7.3rc1 (default, Mar 13 2019, 11:01:15) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> with open('foo', 'w') as f:
...  f.write('foo')
... 
3
>>> 

$ stat foo | sed -n 's/^\(Access.*\)Uid.*$/\1/p'
Access: (0777/-rwxrwxrwx)  

It would be nice to have an option to preserve the mode of the original file.

@pabs3
Copy link
Author

pabs3 commented May 2, 2019

With the proposed option turned on, when creating new files I think it would also be good to obey the system's umask instead of creating files with restricted permissions.

Until there is an option for this, I'm using this code as a workaround:

class AtomicWriterBetterPermissions(AtomicWriter):
        def commit(self, f):
                try:
                        mode = stat(self._path).st_mode
                except FileNotFoundError:
                        # Creating a new file, emulate what os.open() does
                        umask = os.umask(0)
                        umask(umask)
                        mode = 0o777 & ~umask
                os.chmod(f.name, mode)
                super().commit(f)

@anarcat
Copy link

anarcat commented May 2, 2019

                os.chmod(f.name, mode)

isn't there a possibility of a race here too? it seems to me the chmod should happen on a file descriptor, not a path.

@pabs3
Copy link
Author

pabs3 commented May 3, 2019 via email

@untitaker
Copy link
Owner

If you figure out a way to do this without any races lmk. I personally have no usecase for this and would think that people who care about this just call chmod with a fixed mask after writing the file to force it to a hardcoded value.

@pabs3
Copy link
Author

pabs3 commented Jun 24, 2019 via email

@scottj97
Copy link

+1 on this. Changing a file's permissions when overwriting is not expected behavior.

@andersk
Copy link

andersk commented Sep 19, 2019

                        umask = os.umask(0)

Since the umask is per-process, not per-thread, changing it is a potential security problem in the presence of multiple threads. (See https://bugs.python.org/issue21082.)

That’s also why it’s hard for callers to “just call chmod with a fixed mask after writing the file”—it’s difficult to compute the correct mask in a threadsafe way.

My approach for the corresponding bug in Click avoids this problem by replacing tempfile with direct calls to os.open, which naturally respects the umask: pallets/click#1400.

@untitaker
Copy link
Owner

it’s difficult to compute the correct mask in a threadsafe way.

My recommendation was to set it to a value that is hardcoded in your software. I recognize that this is not always possible but it's certainly the least likely to cause races.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants