saltstack / salt

Software to automate the management and configuration of any infrastructure or application at scale. Get access to the Salt software package repository here:
https://repo.saltproject.io/
Apache License 2.0
13.98k stars 5.47k forks source link

[BUG] Salt's umask handling is not thread-safe (causes race conditions) #66607

Open kobus-v-schoor opened 1 month ago

kobus-v-schoor commented 1 month ago

Description Currently, the process' umask is handled in get_umask and set_umask.

Both these functions provide no thread-safety, and can enter into a race condition where the process umask is improperly set/restored. We are running into strange issues on some of our minions where directories are getting created (with file.directory) with 700 permissions (inconsistently, cannot always reproduce), corresponding to a 077 umask (which we do not use anywhere). Salt however does set a 077 umask in some places, making me suspect that the issue is a race-condition within salt. E.g.:

An example of how a race-condition could theoretically trigger this:


def thread_1(...):
    with set_umask(0o77):
        ...

def thread_2(...):
    with set_umask(0o000):
        ...

Threading.start(thread_1)
Threading.start(thread_2)

# thread_1 starts, set_umask's orig_mask gets set to 022, process' umask is set to 077
# thread_2 starts, set_umask's orig_mask gets set to 077, process' umask is set to 000
# thread_1 exits, restores umask to 022
# thread_2 exits, restores umask to 077
# chaos ensues...

I think the proper fix would be to lock access to set_umask, so that any code executing within its context-manager has a consistent umask (what happens if another thread changes the umask while the code wrapped in the cm is still executing? Would not be correct behaviour anyway). Additionally get_umask also needs to be locked, or changed to use a thread-safe way to get the umask (on Linux you can read /proc/self/status to get the umask in a thread-safe manner).

Setup (Please provide relevant configs and/or SLS files (be sure to remove sensitive info. There is no general set-up of Salt.)

Please be as specific as possible and give set-up details.

Steps to Reproduce the behavior (Include debug logs if possible and relevant)

Expected behavior A clear and concise description of what you expected to happen.

Screenshots If applicable, add screenshots to help explain your problem.

Versions Report

salt --versions-report (Provided by running salt --versions-report. Please also mention any differences in master/minion versions.) ```yaml Salt Version: Salt: 3005.5 Dependency Versions: cffi: 1.14.6 cherrypy: 18.6.1 dateutil: 2.8.1 docker-py: 5.0.3 gitdb: Not Installed gitpython: Not Installed Jinja2: 3.1.0 libgit2: Not Installed M2Crypto: 0.39.0 Mako: Not Installed msgpack: 1.0.2 msgpack-pure: Not Installed mysql-python: Not Installed pycparser: 2.21 pycrypto: Not Installed pycryptodome: 3.9.8 pygit2: Not Installed Python: 3.9.18 (main, Jan 25 2024, 23:24:48) python-gnupg: 0.4.8 PyYAML: 6.0.1 PyZMQ: 23.2.0 smmap: Not Installed timelib: 0.2.4 Tornado: 4.5.3 ZMQ: 4.3.4 System Versions: dist: ubuntu 22.04 jammy locale: utf-8 machine: x86_64 release: 5.15.0-60-generic system: Linux version: Ubuntu 22.04 jammy ```

Additional context Add any other context about the problem here.

kobus-v-schoor commented 1 month ago

https://github.com/saltstack/salt/issues/66055 Might be related to this issue