Make set_umask/get_umask thread safe

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.

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

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