fix: read the umask under one lock

plain_file_mode and safe_extract_zip set the umask to 0 to read it. Two threads doing so could restore 0 last, leaving every later file world-writable. Both now read it through process_umask, which holds a lock.

scripts/artifacts.py

@@ -12,6 +12,7 @@ import contextlib
 import json
 import os
 import tempfile
+import threading
 import re
 
 
@@ -62,12 +63,26 @@ def write_if_changed(
     return True
 
 
+_UMASK_LOCK = threading.Lock()
+
+
+def process_umask() -> int:
+    """The process umask, read without letting a second thread see it at 0.
+
+    Reading the umask sets it, so it is put back at once. Two threads doing
+    that unguarded interleave as A sets 0 and reads 022, B sets 0 and reads 0,
+    A restores 022, B restores 0: the process keeps umask 0 and every file it
+    creates afterwards is world-writable.
+    """
+    with _UMASK_LOCK:
+        umask = os.umask(0)
+        os.umask(umask)
+    return umask
+
+
 def plain_file_mode() -> int:
     """The mode open() gives a new file: 0o666 less the process umask."""
-    # Reading the umask sets it, so it is put back at once.
+    return 0o666 & ~process_umask()
-    umask = os.umask(0)
-    os.umask(umask)
-    return 0o666 & ~umask
 
 
 def set_plain_mode(path: str | os.PathLike) -> None:

scripts/ziptools.py

@@ -14,7 +14,7 @@ import sys
 import tempfile
 import zipfile
 
-from artifacts import set_plain_mode
+from artifacts import process_umask, set_plain_mode
 from hashing import md5sum
 
 
@@ -251,9 +251,7 @@ def safe_extract_zip(
     """
     dest = os.path.realpath(dest_dir)
     os.makedirs(dest, exist_ok=True)
-    # Reading the umask sets it, so it is put back at once.
+    umask = process_umask()
-    umask = os.umask(0)
-    os.umask(umask)
     with zipfile.ZipFile(zip_path, "r") as zf:
         members = zf.infolist()
         if len(members) > max_members:

tests/test_atomic_artifacts.py

@@ -154,6 +154,38 @@ class ScratchFilesGetThePlainMode(unittest.TestCase):
         artifacts.write_text_atomic(str(self.dir / "a.json"), "{}")
         self.assertEqual(self._mode(self.dir / "a.json"), 0o664)
 
+    def test_threads_reading_the_mode_leave_the_umask_alone(self):
+        """mame_devices fetches on eight threads and every cache write reads
+        the umask: unguarded, two readers interleaved and left it at 0."""
+        import threading  # noqa: PLC0415
+
+        interval = sys.getswitchinterval()
+        sys.setswitchinterval(1e-6)
+        self.addCleanup(sys.setswitchinterval, interval)
+        modes: set[int] = set()
+
+        def read() -> None:
+            seen = {artifacts.plain_file_mode() for _ in range(20000)}
+            modes.update(seen)
+
+        threads = [threading.Thread(target=read) for _ in range(8)]
+        for thread in threads:
+            thread.start()
+        for thread in threads:
+            thread.join()
+        current = os.umask(0o022)
+        self.assertEqual(current, 0o022)
+        self.assertEqual(modes, {0o644})
+
+    def test_no_script_reads_the_umask_unguarded(self):
+        offenders = [
+            f"{path.name}:{number}"
+            for path in sorted((REPO_ROOT / "scripts").rglob("*.py"))
+            for number, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1)
+            if "os.umask(" in line and path.name != "artifacts.py"
+        ]
+        self.assertEqual(offenders, [])
+
     def test_packs_and_their_manifest(self):
         import zipfile  # noqa: PLC0415