filelock: filelock 3.10.0 changes umask to 0 in multithreaded processes, causing world-writable files to be created

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.:

$ ls -l venv/bin
total 56K
-rw-rw-rw- 1 ckuehl users 2.2K Mar 22 10:35 activate
-rw-rw-rw- 1 ckuehl users 1.5K Mar 22 10:35 activate.csh
-rw-rw-rw- 1 ckuehl users 3.0K Mar 22 10:35 activate.fish
-rw-rw-rw- 1 ckuehl users 3.3K Mar 22 10:35 activate.nu
-rw-rw-rw- 1 ckuehl users 1.8K Mar 22 10:35 activate.ps1
-rw-rw-rw- 1 ckuehl users 1.2K Mar 22 10:35 activate_this.py
[...]

$ ls -l venv/lib/python3.8/site-packages
total 48K
[...]
-rw-rw-rw- 1 ckuehl users  151 Mar 22 10:35 distutils-precedence.pth
[...]
-rw-rw-rw- 1 ckuehl users    0 Mar 22 10:35 setuptools-65.6.3.virtualenv

This is causing issues for us because tools like https://github.com/Yelp/aactivator refuse to automatically activate world-writable activate files. 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:

$ venv/bin/pip freeze --all
distlib==0.3.6
filelock==3.10.0
pip==23.0.1
platformdirs==2.5.2
setuptools==65.6.3
virtualenv==20.17.1
wheel==0.38.4

…and by calling this command:

$ venv/bin/python -m virtualenv --no-download --python /usr/bin/python3.8 test-venv
created virtual environment CPython3.8.13.final.0-64 in 104ms
  creator CPython3Posix(dest=/tmp/tmp.juMXqvrFzt/test-venv, clear=False, no_vcs_ignore=False, global=False)
  seeder FromAppData(download=False, pip=bundle, setuptools=bundle, wheel=bundle, via=copy, app_data_dir=/nail/home/ckuehl/.local/share/virtualenv)
    added seed packages: pip==23.0.1, setuptools==65.6.3, wheel==0.38.4
  activators BashActivator,CShellActivator,FishActivator,NushellActivator,PowerShellActivator,PythonActivator
$ ls -l test-venv/bin/activate
-rw-rw-rw- 1 ckuehl users 2.1K Mar 22 10:44 test-venv/bin/activate

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 is 0o22. 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:

import os
import threading
import traceback

real_umask = os.umask

def fake_umask(new_umask):
    pid = os.getpid()
    tid = threading.get_ident()
    print(f"{pid=} {tid=} Changing umask to {oct(new_umask)} from {traceback.format_stack()[-2]!r}")
    return real_umask(new_umask)

os.umask = fake_umask

…then you can see the calls to os.umask from different interleaved threads:

$ rm -rf test-venv && venv/bin/python -m virtualenv --no-download --python /usr/bin/python3.8 test-venv && ls -l test-venv/bin/activate
pid=3823951 tid=140034962657728 Changing umask to 0o0 from '  File "/tmp/tmp.juMXqvrFzt/venv/lib/python3.8/site-packages/filelock/_api.py", line 182, in acquire\n    previous_umask = os.umask(0)\n'
pid=3823951 tid=140034962657728 Changing umask to 0o22 from '  File "/tmp/tmp.juMXqvrFzt/venv/lib/python3.8/site-packages/filelock/_api.py", line 186, in acquire\n    os.umask(previous_umask)  # reset umask to initial value\n'
pid=3823951 tid=140034850154048 Changing umask to 0o0 from '  File "/tmp/tmp.juMXqvrFzt/venv/lib/python3.8/site-packages/filelock/_api.py", line 182, in acquire\n    previous_umask = os.umask(0)\n'
pid=3823951 tid=140034858546752 Changing umask to 0o0 from '  File "/tmp/tmp.juMXqvrFzt/venv/lib/python3.8/site-packages/filelock/_api.py", line 182, in acquire\n    previous_umask = os.umask(0)\n'
pid=3823951 tid=140034850154048 Changing umask to 0o22 from '  File "/tmp/tmp.juMXqvrFzt/venv/lib/python3.8/site-packages/filelock/_api.py", line 186, in acquire\n    os.umask(previous_umask)  # reset umask to initial value\n'
pid=3823951 tid=140034934462016 Changing umask to 0o0 from '  File "/tmp/tmp.juMXqvrFzt/venv/lib/python3.8/site-packages/filelock/_api.py", line 182, in acquire\n    previous_umask = os.umask(0)\n'
pid=3823951 tid=140034934462016 Changing umask to 0o22 from '  File "/tmp/tmp.juMXqvrFzt/venv/lib/python3.8/site-packages/filelock/_api.py", line 186, in acquire\n    os.umask(previous_umask)  # reset umask to initial value\n'
pid=3823951 tid=140034858546752 Changing umask to 0o0 from '  File "/tmp/tmp.juMXqvrFzt/venv/lib/python3.8/site-packages/filelock/_api.py", line 186, in acquire\n    os.umask(previous_umask)  # reset umask to initial value\n'
created virtual environment CPython3.8.13.final.0-64 in 112ms
  creator CPython3Posix(dest=/tmp/tmp.juMXqvrFzt/test-venv, clear=False, no_vcs_ignore=False, global=False)
  seeder FromAppData(download=False, pip=bundle, setuptools=bundle, wheel=bundle, via=copy, app_data_dir=/nail/home/ckuehl/.local/share/virtualenv)
    added seed packages: pip==23.0.1, setuptools==65.6.3, wheel==0.38.4
  activators BashActivator,CShellActivator,FishActivator,NushellActivator,PowerShellActivator,PythonActivator
-rw-rw-rw- 1 ckuehl users 2.1K Mar 22 10:53 test-venv/bin/activate

Downgrading to filelock 3.9.1 makes the problem go away.

About this issue

  • Original URL
  • State: closed
  • Created a year ago
  • Comments: 23 (18 by maintainers)

Most upvoted comments

I don’t think threading locks prevent execution of other threads, they just create a critical section.

Note my comment above:

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 🤔

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).