diff --git a/changelog/66607.fixed.md b/changelog/66607.fixed.md new file mode 100644 index 000000000000..a8b3f43d2246 --- /dev/null +++ b/changelog/66607.fixed.md @@ -0,0 +1 @@ +Serialize ``set_umask``/``get_umask`` with a lock. The umask is process-global, so concurrent calls from different threads could restore a stale value and leave the process umask permanently changed — salt-api under rest_cherrypy would get stuck at ``0o277`` and return 500 for every ``client=ssh`` request until restarted. diff --git a/salt/utils/files.py b/salt/utils/files.py index 00b1f7663adf..1d2a419f9ef7 100644 --- a/salt/utils/files.py +++ b/salt/utils/files.py @@ -13,6 +13,7 @@ import stat import subprocess import tempfile +import threading import time import urllib.parse @@ -95,6 +96,12 @@ def helper(*args, **kwargs): log = logging.getLogger(__name__) +# The umask is global to the process, so concurrent calls to get_umask() and +# set_umask() must be serialized or they can restore each other's saved value +# and leave the process umask permanently changed. An RLock is used so a +# thread holding the lock can still nest set_umask() calls. +_umask_lock = threading.RLock() + LOCAL_PROTOS = ("", "file") REMOTE_PROTOS = ("http", "https", "ftp", "swift", "s3") VALID_PROTOS = ("salt", "file") + REMOTE_PROTOS @@ -476,8 +483,9 @@ def get_umask(): """ Returns the current umask """ - ret = os.umask(0) # pylint: disable=blacklisted-function - os.umask(ret) # pylint: disable=blacklisted-function + with _umask_lock: + ret = os.umask(0) # pylint: disable=blacklisted-function + os.umask(ret) # pylint: disable=blacklisted-function return ret @@ -490,11 +498,12 @@ def set_umask(mask): # Don't attempt on Windows, or if no mask was passed yield else: - orig_mask = os.umask(mask) # pylint: disable=blacklisted-function - try: - yield - finally: - os.umask(orig_mask) # pylint: disable=blacklisted-function + with _umask_lock: + orig_mask = os.umask(mask) # pylint: disable=blacklisted-function + try: + yield + finally: + os.umask(orig_mask) # pylint: disable=blacklisted-function def fopen(*args, **kwargs): diff --git a/tests/pytests/unit/utils/test_files.py b/tests/pytests/unit/utils/test_files.py index 00e9054ddc67..9e43bd800e94 100644 --- a/tests/pytests/unit/utils/test_files.py +++ b/tests/pytests/unit/utils/test_files.py @@ -5,6 +5,7 @@ import copy import io import os +import threading import pytest @@ -247,3 +248,58 @@ async def test_await_lock_raises_when_lock_path_is_directory(tmp_path): with pytest.raises(salt.exceptions.FileLockError, match="not a file"): async with salt.utils.files.await_lock(lock_fn, lock_fn=lock_fn, timeout=1): pass + + +@pytest.mark.skip_on_windows(reason="set_umask is a no-op on Windows") +def test_set_umask_is_serialized_across_threads(): + """ + The umask is process-global. If two threads overlap inside set_umask, + one restores the other's temporary mask and the process umask stays + changed permanently (issue #66607). A thread must not be able to enter + set_umask while another thread is inside it, and the original umask + must survive concurrent use. + """ + orig = salt.utils.files.get_umask() + holder_entered = threading.Event() + release_holder = threading.Event() + contender_done = [] + + def holder(): + with salt.utils.files.set_umask(0o277): + holder_entered.set() + release_holder.wait(timeout=10) + + def contender(): + with salt.utils.files.set_umask(0o022): + contender_done.append(True) + + holder_thread = threading.Thread(target=holder) + holder_thread.start() + try: + assert holder_entered.wait(timeout=10) + contender_thread = threading.Thread(target=contender) + contender_thread.start() + # While the holder is inside set_umask, the contender must block + contender_thread.join(timeout=0.5) + assert not contender_done + release_holder.set() + contender_thread.join(timeout=10) + assert contender_done + finally: + release_holder.set() + holder_thread.join(timeout=10) + + assert salt.utils.files.get_umask() == orig + + +@pytest.mark.skip_on_windows(reason="set_umask is a no-op on Windows") +def test_set_umask_nests_in_a_single_thread(): + """ + A thread already holding the umask lock must be able to nest + set_umask calls without deadlocking. + """ + orig = salt.utils.files.get_umask() + with salt.utils.files.set_umask(0o277): + with salt.utils.files.set_umask(0o022): + pass + assert salt.utils.files.get_umask() == orig