diff --git a/scripts/artifacts.py b/scripts/artifacts.py index 82a120302..7cbfebc17 100644 --- a/scripts/artifacts.py +++ b/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. - umask = os.umask(0) - os.umask(umask) - return 0o666 & ~umask + return 0o666 & ~process_umask() def set_plain_mode(path: str | os.PathLike) -> None: diff --git a/scripts/ziptools.py b/scripts/ziptools.py index cb82d41b9..e8ad8bdfb 100644 --- a/scripts/ziptools.py +++ b/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 = os.umask(0) - os.umask(umask) + umask = process_umask() with zipfile.ZipFile(zip_path, "r") as zf: members = zf.infolist() if len(members) > max_members: diff --git a/tests/test_atomic_artifacts.py b/tests/test_atomic_artifacts.py index e106975b8..f54363823 100644 --- a/tests/test_atomic_artifacts.py +++ b/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