filelock 3.10.0 changes umask to 0 in multithreaded processes, causing world-writable files to be created #204
Comments
|
Can you put in a PR to have a thread lock here; to make it thread safe? 😊 |
|
I'm not sure what the fix should look like -- I don't think that just putting a thread lock around the code snippet I highlighted is sufficient. That would prevent multiple threads from entering the umask checking code at once, but it doesn't prevent an issue where thread A changes the umask and then thread B takes over execution and starts writing files with the new umask. |
|
Wouldn't #205 work? two threads can't be at the same time within the umask changing code; so that part will become atomic 🤔 |
|
My understanding is that a lock creates a critical section which two threads cannot execute at the same time, but it doesn't prevent a second thread from running entirely. So if I have two threads which interleave like this:
...then thread B will have created a file using the wrong umask. I could be wrong on this though, I don't know all of the subtleties of how Python handles threading. |
|
Threads are never parallel, because they are not processes. So only one thread can run at any time. A thread lock ensures that only one thread can be in that code segment at any time. So it should work. |
|
Guess this should ensure that two parallel locks will create correct file permissions. However, if there's another thread creating a file (not a lock file) in another thread, that can still be incorrect. I'm not sure that's solve-able 🤔 |
|
@jahrules cc on this as is change you've added 🤔 |
|
Very interesting. I'm researching this now. |
|
Guess we can use something like https://stackoverflow.com/a/53357635 to read at first invocation the state, and reuse it. That would make most cases work (assuming most of the time users don't change this, so we don't need to keep updating it). |
Are you sure? I see thread interleaving when I test it out: import os
import threading
import time
def check_umask():
while True:
with threading.Lock():
old_umask = os.umask(0)
print("Changed umask to 0")
time.sleep(1)
os.umask(old_umask)
print("Reset umask back")
time.sleep(0.1)
def print_forever():
while True:
print("X")
time.sleep(0.1)
if __name__ == '__main__':
t1 = threading.Thread(target=check_umask)
t2 = threading.Thread(target=print_forever)
t1.start()
t2.start()
t1.join()
t2.join()All of those |
|
You're creating a new thread lock per thread there. Move the thread lock to a global and will work as described. |
Oh, good point. It's still the same result, though. I don't think threading locks prevent execution of other threads, they just create a critical section. https://docs.python.org/3/library/threading.html#lock-objects seems to agree as well. |
Note my comment above:
|
|
sorry, catching up here. @chriskuehl I 100% agree that it's the threads writing between os.umask(0) and the revert. This is an unfortunate side effect of threads sharing the global namespace with the process that spawned the thread. Im currently pondering a solution. |
|
Here's my suggestion: Since I don't think there is a convenient workaround for setting os.umask not affecting files potentially being written in other threads; I would propose updating the code here. removing both os.umask here: and replacing with a call to os.fchown() inside of _unix.py/_windows.py files right after the call to os.open() |
|
The proposed change works for UNIX based systems but has some issues with Windows. Im currently researching a solution |
|
Sorry for a silly question, but why does EDIT: I see |
|
That is a bad API because other people would run into the same issue as we here. |
|
The fix in #206 is causing issues, today our CI started failing trying to install poetry in Docker/Podman container once 3.10.2 was released, 3.10.0 is working fine Added details to the PR #206 (comment) |
I don't think there's really an issue here. The default umask is 0o022 which will default to permissions of 644 (-rw-r--r--). If the user has requested different permissions, then they will be set following that going from less stringent to more stringent in 99.9% of cases. If the user has changed their umask themselves; then yes, it will create the filelock with 0o777 and then chmod will make it 0o644. But it should be noted that before my changes, this was how the lock files already worked. We just wanted to expose the ability to have permissions other than 0o644 so that users could have lockfiles which are usable by multiple usercodes without having to know about umask or take a manual action to change the permissions. |
|
Albeit, before this commit, the prior solution was not thread-safe which is not any better. Before this change (I mean before the multi-user locks) you are right that setting However, I think that 0.1% case can be covered by passing Perhaps this is better as a separate PR? |
|
I think this is a fair change and simple enough to implement. I agree.
…On Thu, Mar 23, 2023, 5:40 PM TheMatt2 ***@***.***> wrote:
Albeit, before this commit, the prior solution was not thread-safe which
is not any better.
And you are right that so long as os.umask() is a reasonable value, there
is no issue.
Before this change (I mean before the multi-user locks) you are right that
setting os.umask(0) would give the locks 0o777 perms. The difference is,
now that there is an explicit mode argument, there is now a situation
where a user may *think* the lock will be securely and properly held, but
then has an issue due to this moment between file creation and permission
set.
However, I think that 0.1% case can be covered by passing self._mode to
os.open() as well.
Just because its not "common" doesn't make it unimportant.
Perhaps this is better as a separate PR?
—
Reply to this email directly, view it on GitHub
<#204 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APCURVG4NHTYVM7WUTXAE43W5S7LTANCNFSM6AAAAAAWEFK3PU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
We are encountering an issue where a recent change in filelock (#192) is sometimes causing virtualenvs we create to be created with world-writable files, e.g.:
This is causing issues for us because tools like https://github.com/Yelp/aactivator refuse to automatically activate world-writable
activatefiles. On multi-user systems this may also be a security concern.I believe I've tracked it down to a change in #192 to call
umask(0)before acquiring the lock:https://github.com/tox-dev/py-filelock/blob/66b2d49720a215c6c03bd5ab1a840defaa00436a/src/filelock/_api.py#L182-L186
My hypothesis is that code from another thread is being run in between the call to
umask(0)and the call to reset the umask back to its original value, which causes files to be created while umask 0 is active.Reproduction while creating a virtualenv
I can reproduce this even without tox in a virtualenv with these dependencies:
...and by calling this command:
Note that this reproduces for me only about 50% of the time; other times it has the normal (
-rw-r--r--) permissions as expected since my umask is0o22. I'm assuming it only happens some of the time due to getting lucky with thread scheduling.If I manually patch virtualenv by inserting this into the import path early on:
...then you can see the calls to
os.umaskfrom different interleaved threads:Downgrading to filelock 3.9.1 makes the problem go away.
The text was updated successfully, but these errors were encountered: