From 1806507631dc83005809779561eb273f52a68682 Mon Sep 17 00:00:00 2001 From: Fabian Isensee Date: Sat, 7 Nov 2026 09:00:16 +0100 Subject: [PATCH] Give replaced files the mode of a direct write, not mkstemp's 0600 Since to_b2z() became atomic (3ea96190, 4.8.0) it builds the zip in tempfile.mkstemp() and os.replace()s it onto the target. mkstemp() always creates 0o600, so every .b2z written or rewritten since then is private to its owner whatever the umask, and a file the user had chmod-ed loses its mode. On shared filesystems the group can no longer read the stores. RemoteStore exports, RemoteFile/OrdinaryFile downloads and the RemoteStore disk cache's manifests use the same pattern and have the same problem. Add core.mkstemp_for_replace(): it creates the temporary file beside the target with os.open(..., O_CREAT | O_EXCL, 0o666), so the kernel applies the umask (or the directory's default ACL) exactly as for a plain open(); the umask is never read, since os.umask() can only be read by setting it process-wide, which races with other threads. If the target exists, the temporary file is created no more open than it and then gets its permission bits (fchmod), as an in-place write would have kept them (4.7.0 behaviour). Windows is unchanged. Use it at every mkstemp() + os.replace() site; to_b2z() and the RemoteStore export now write the zip through the fd, so a read-only target can still be replaced, as on main. publish_materialized() keeps a replaced .b2z's mode too. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SwMZj6jcia1iL27SPfQCoh --- src/blosc2/b2view/ordinary_file.py | 11 ++- src/blosc2/core.py | 36 ++++++++ src/blosc2/dict_store.py | 11 ++- src/blosc2/remote_file.py | 4 +- src/blosc2/remote_store.py | 10 +- src/blosc2/remote_store_cache.py | 5 +- src/blosc2/store_materialize.py | 7 ++ tests/test_file_mode.py | 144 +++++++++++++++++++++++++++++ 8 files changed, 212 insertions(+), 16 deletions(-) create mode 100644 tests/test_file_mode.py diff --git a/src/blosc2/b2view/ordinary_file.py b/src/blosc2/b2view/ordinary_file.py index 135b7b8f4..647412433 100644 --- a/src/blosc2/b2view/ordinary_file.py +++ b/src/blosc2/b2view/ordinary_file.py @@ -3,13 +3,18 @@ import mimetypes import os import stat -import tempfile from pathlib import Path, PurePosixPath from urllib.parse import urlsplit from urllib.request import url2pathname import blosc2 -from blosc2.core import find_url_separator, fsspec_filesystem, is_fsspec_url, parse_container_url +from blosc2.core import ( + find_url_separator, + fsspec_filesystem, + is_fsspec_url, + mkstemp_for_replace, + parse_container_url, +) MAX_READ_BYTES = 16 << 20 TRANSFER_BYTES = 1 << 20 @@ -179,7 +184,7 @@ def download(self, destination, *, overwrite=False, progress=None, cancel=None): raise FileExistsError(destination) if self.fs is None and destination.exists() and os.path.samefile(self.path, destination): raise ValueError("Destination must differ from the source file") - fd, temporary = tempfile.mkstemp(prefix=".b2view-download-", dir=destination.parent) + fd, temporary = mkstemp_for_replace(destination, prefix=".b2view-download-") try: with os.fdopen(fd, "wb") as output, self._stream(streaming=True) as source: done = 0 diff --git a/src/blosc2/core.py b/src/blosc2/core.py index dd017a2ef..499ecd95f 100644 --- a/src/blosc2/core.py +++ b/src/blosc2/core.py @@ -8,6 +8,7 @@ # Avoid checking the name of type annotations at run time from __future__ import annotations +import contextlib import copy import ctypes import ctypes.util @@ -20,10 +21,12 @@ import platform import re import shutil +import stat import subprocess import sys import urllib.parse import urllib.request +import uuid from dataclasses import asdict, fields from functools import cache, lru_cache from typing import TYPE_CHECKING, ClassVar @@ -2319,6 +2322,39 @@ def remove_urlpath(path: str) -> None: blosc2_ext.remove_urlpath(path) +def mkstemp_for_replace(target: str | os.PathLike, *, prefix: str = "", suffix: str = "") -> tuple[int, str]: + """Create a temporary file next to *target*, to be ``os.replace()``-d onto it. + + Returns ``(fd, path)`` like ``tempfile.mkstemp()``, but the file gets the + permissions that writing *target* in place would leave, not mkstemp's + 0o600: an existing *target* keeps its permission bits, a new one gets + 0o666 less the umask (or the directory's default ACL). The kernel applies + the umask at creation, so it is never read: ``os.umask()`` can only be read + by setting it, process-wide, which races with other threads. + """ + target = os.path.abspath(target) + # 122 random bits: unlike mkstemp's short names, no retry loop is needed. + path = os.path.join(os.path.dirname(target), f"{prefix}{uuid.uuid4().hex}{suffix}") + mode = None + # Windows only has a read-only flag (and a read-only target cannot be replaced). + if os.name != "nt": + with contextlib.suppress(OSError): # no target: a new file + mode = stat.S_IMODE(os.stat(target).st_mode) + # Never more open than the target, not even until the chmod below: whoever + # opened the file meanwhile could read what is written to it later. + fd = os.open( + path, + os.O_RDWR | os.O_CREAT | os.O_EXCL | getattr(os, "O_BINARY", 0), + 0o666 if mode is None else mode & 0o666, + ) + if mode is not None: + # Through the fd, so a symlink swapped in for path cannot redirect it. + # Best effort: some filesystems (e.g. vfat) refuse chmod. + with contextlib.suppress(OSError): + os.fchmod(fd, mode) + return fd, path + + def schunk_from_cframe( cframe: bytes | str, copy: bool = False, *, deserialize: str = "safe" ) -> blosc2.SChunk: diff --git a/src/blosc2/dict_store.py b/src/blosc2/dict_store.py index 4d337aa3b..24ce455d1 100644 --- a/src/blosc2/dict_store.py +++ b/src/blosc2/dict_store.py @@ -20,6 +20,7 @@ import blosc2 from blosc2.c2array import C2Array +from blosc2.core import mkstemp_for_replace from blosc2.deserialization import get_deserialize, normalize_deserialize, set_deserialize from blosc2.embed_store import EmbedStore from blosc2.info import InfoReporter @@ -939,7 +940,9 @@ def to_b2z(self, overwrite=False, filename=None) -> os.PathLike[Any] | str: The file is written to a temporary sibling and moved onto ``filename`` atomically: concurrent readers of an existing target see either the old archive or the new one, never a partial write. On Windows, the - final replace fails if another process holds the target open. + final replace fails if another process holds the target open. An + existing target keeps its permission bits; a new one gets the usual + ``0o666`` less the umask. Examples -------- @@ -987,10 +990,10 @@ def to_b2z(self, overwrite=False, filename=None) -> os.PathLike[Any] | str: # atomically onto the target: concurrent readers of the target see either # the old zip or the new one, never a torn state. (On Windows the final # replace fails if another process holds the target open.) - fd, tmp_path = tempfile.mkstemp(dir=os.path.dirname(os.path.abspath(b2z_path)), suffix=".b2z.tmp") - os.close(fd) + fd, tmp_path = mkstemp_for_replace(b2z_path, suffix=".b2z.tmp") try: - with zipfile.ZipFile(tmp_path, "w", zipfile.ZIP_STORED) as zf: + # Through the fd: the file may already have a read-only target's mode + with os.fdopen(fd, "w+b") as tmp_file, zipfile.ZipFile(tmp_file, "w", zipfile.ZIP_STORED) as zf: # Write all files (except estore_path) first (sorted by size) for filepath in filepaths: arcname = os.path.relpath(filepath, self.working_dir) diff --git a/src/blosc2/remote_file.py b/src/blosc2/remote_file.py index 8d19b2804..ec68f927d 100644 --- a/src/blosc2/remote_file.py +++ b/src/blosc2/remote_file.py @@ -3,12 +3,12 @@ import hashlib import os import struct -import tempfile import time import weakref from pathlib import Path import blosc2 +from blosc2.core import mkstemp_for_replace from blosc2.remote_array import RemoteMetadataMapping from blosc2.remote_object import RemoteObject @@ -255,7 +255,7 @@ def download(self, destination, *, overwrite=False, progress=None, cancel=None): destination = Path(destination).absolute() if not overwrite and destination.exists(): raise FileExistsError(destination) - fd, temporary = tempfile.mkstemp(prefix=".b2view-download-", dir=destination.parent) + fd, temporary = mkstemp_for_replace(destination, prefix=".b2view-download-") try: with os.fdopen(fd, "wb") as file: done = 0 diff --git a/src/blosc2/remote_store.py b/src/blosc2/remote_store.py index 58a52a25d..fd88e3775 100644 --- a/src/blosc2/remote_store.py +++ b/src/blosc2/remote_store.py @@ -20,7 +20,7 @@ import numpy as np import blosc2 -from blosc2.core import parse_container_url, storage_options_fingerprint +from blosc2.core import mkstemp_for_replace, parse_container_url, storage_options_fingerprint from blosc2.info import InfoReporter, format_nbytes_info, format_store_tree from blosc2.proxy import CacheCoordinator from blosc2.proxy_source import Traffic @@ -1809,8 +1809,9 @@ def save_selection( # noqa: C901 ) staging_dir = tempfile.mkdtemp(prefix="b2z-export-", dir=dest_dir) - fd, tmp_zip = tempfile.mkstemp(prefix="export-", suffix=".b2z.tmp", dir=dest_dir) - os.close(fd) + fd, tmp_zip = mkstemp_for_replace(dest_abs, prefix="export-", suffix=".b2z.tmp") + # Written through the fd: the file may already have a read-only target's mode + tmp_file = os.fdopen(fd, "w+b") try: exported_caches = [] for orig_key in candidates: @@ -1894,7 +1895,7 @@ def save_selection( # noqa: C901 filepaths.append(fp) filepaths.sort(key=os.path.getsize, reverse=True) - with zipfile.ZipFile(tmp_zip, "w", zipfile.ZIP_STORED) as zf: + with tmp_file, zipfile.ZipFile(tmp_file, "w", zipfile.ZIP_STORED) as zf: for fp in filepaths: arcname = os.path.relpath(fp, staging_dir) zf.write(fp, arcname) @@ -1903,6 +1904,7 @@ def save_selection( # noqa: C901 os.replace(tmp_zip, dest_abs) return dest_abs finally: + tmp_file.close() if os.path.exists(tmp_zip): with contextlib.suppress(OSError): os.unlink(tmp_zip) diff --git a/src/blosc2/remote_store_cache.py b/src/blosc2/remote_store_cache.py index 2dfd9e040..346ff14c1 100644 --- a/src/blosc2/remote_store_cache.py +++ b/src/blosc2/remote_store_cache.py @@ -4,7 +4,6 @@ import json import os import shutil -import tempfile import threading import time import uuid @@ -14,7 +13,7 @@ import msgpack import blosc2 -from blosc2.core import cache_directory_name +from blosc2.core import cache_directory_name, mkstemp_for_replace from blosc2.msgpack_utils import msgpack_packb @@ -190,7 +189,7 @@ def discard_old_generations(self, active): def atomic_write(path, data): """Publish complete metadata and sync its directory before returning.""" - fd, name = tempfile.mkstemp(prefix="publish-", dir=path.parent) + fd, name = mkstemp_for_replace(path, prefix="publish-") try: with os.fdopen(fd, "wb") as stream: stream.write(data) diff --git a/src/blosc2/store_materialize.py b/src/blosc2/store_materialize.py index 8ed06c4fa..f6d63d355 100644 --- a/src/blosc2/store_materialize.py +++ b/src/blosc2/store_materialize.py @@ -4,6 +4,7 @@ import contextlib import os +import shutil import tempfile import uuid @@ -45,6 +46,12 @@ def publish_materialized(staged, destination, overwrite, staging): raise FileExistsError(destination) previous = os.path.join(staging, "previous") os.replace(destination, previous) + if os.name != "nt" and os.path.isfile(previous) and os.path.isfile(staged): + # A replaced .b2z keeps its mode, as with to_b2z(); both paths now lie + # in the private staging directory, so a chmod by path is safe. Best + # effort: some filesystems (e.g. vfat) refuse chmod. + with contextlib.suppress(OSError): + shutil.copymode(previous, staged) try: os.replace(staged, destination) except BaseException: diff --git a/tests/test_file_mode.py b/tests/test_file_mode.py new file mode 100644 index 000000000..bd2728eb5 --- /dev/null +++ b/tests/test_file_mode.py @@ -0,0 +1,144 @@ +####################################################################### +# Copyright (c) 2019-present, Blosc Development Team +# All rights reserved. +# +# SPDX-License-Identifier: BSD-3-Clause +####################################################################### + +"""Files written through a temporary file and os.replace() get the permissions +of a direct write, not tempfile.mkstemp()'s 0o600: a new file follows the umask, +an overwritten one keeps its mode.""" + +import os +import stat +import threading +from types import SimpleNamespace + +import numpy as np +import pytest + +import blosc2 +from blosc2.b2view.ordinary_file import OrdinaryFile +from blosc2.core import mkstemp_for_replace + +pytestmark = pytest.mark.skipif(os.name == "nt" or blosc2.IS_WASM, reason="POSIX permission bits") + + +@pytest.fixture(params=[0o022, 0o002], ids=["umask022", "umask002"]) +def umask(request): + old = os.umask(request.param) + yield request.param + os.umask(old) + + +def file_mode(path): + return stat.S_IMODE(os.stat(path).st_mode) + + +def check_modes(path, write, umask): + """write(overwrite) creates path, then overwrites it.""" + write(False) + assert file_mode(path) == 0o666 & ~umask + os.chmod(path, 0o640) + write(True) + assert file_mode(path) == 0o640 + + +def test_temporary_file_not_more_open_than_target(tmp_path, umask, monkeypatch): + # Whoever opens the temporary file before its chmod could read what is written later + target = tmp_path / "private.b2z" + target.write_bytes(b"secret") + target.chmod(0o600) + created = [] + real_open = os.open + + def recording_open(*args, **kwargs): + fd = real_open(*args, **kwargs) + created.append(stat.S_IMODE(os.fstat(fd).st_mode)) + return fd + + monkeypatch.setattr(os, "open", recording_open) + fd, path = mkstemp_for_replace(target) + os.close(fd) + assert created == [0o600] + assert file_mode(path) == 0o600 + + +@pytest.mark.parametrize("rewrite_mode", ["a", "w"]) +def test_b2z_store(tmp_path, umask, rewrite_mode): + path = tmp_path / "store.b2z" + + def write(overwrite): # closing the store writes the .b2z (TreeStore: via DictStore.to_b2z()) + with blosc2.TreeStore(str(path), mode=rewrite_mode if overwrite else "w") as store: + store["/a"] = np.arange(10) + + check_modes(path, write, umask) + assert not list(tmp_path.glob("*.tmp")) + + +def test_read_only_b2z_rewrite(tmp_path): + # os.replace() needs no write access to the old file, so this works as on main + path = tmp_path / "store.b2z" + with blosc2.TreeStore(str(path), mode="w") as store: + store["/a"] = np.arange(10) + path.chmod(0o444) + with blosc2.TreeStore(str(path), mode="a") as store: + store["/b"] = np.arange(5) + assert file_mode(path) == 0o444 + with blosc2.TreeStore(str(path), mode="r") as store: + assert sorted(store.keys()) == ["/a", "/b"] + + +def test_to_b2z(tmp_path, umask): + source = str(tmp_path / "store.b2d") + copy = tmp_path / "copy.b2z" + with blosc2.DictStore(source, mode="w") as store: + store["/a"] = np.arange(10) + with blosc2.DictStore(source, mode="r") as store: + check_modes(copy, lambda overwrite: store.to_b2z(filename=str(copy), overwrite=overwrite), umask) + + +def test_materialize(tmp_path, umask): + source = str(tmp_path / "source.b2d") + destination = tmp_path / "copy.b2z" + with blosc2.TreeStore(source, mode="w") as tree: + tree["/a"] = np.arange(10) + with blosc2.TreeStore(source, mode="r") as tree: + check_modes(destination, lambda overwrite: tree.materialize(destination, overwrite=overwrite), umask) + + +def test_remote_store_export_and_disk_cache(tmp_path, umask): + fsspec = pytest.importorskip("fsspec") + source = tmp_path / "source.b2z" + with blosc2.TreeStore(source, mode="w") as tree: + tree["/group/a"] = np.arange(100) + url = f"memory://{tmp_path.name}-file-mode.b2z" + fsspec.filesystem("memory").pipe(url, source.read_bytes()) + cache = tmp_path / "cache" + export = tmp_path / "export.b2z" + with blosc2.RemoteStore.with_sparse_cache(url, cache) as store: + with store["group/a"] as a: + np.testing.assert_array_equal(a[:10], np.arange(10)) + check_modes(export, lambda overwrite: store.save(export, overwrite=overwrite), umask) + # The cache's manifests (active_generation.json, ...) like its arrays + assert {file_mode(path) for path in cache.rglob("*") if path.is_file()} == {0o666 & ~umask} + + +def test_ordinary_file_download(tmp_path, umask): + source = tmp_path / "source.txt" + source.write_bytes(b"payload") + destination = tmp_path / "copy.txt" + file = OrdinaryFile(source) + check_modes(destination, lambda overwrite: file.download(destination, overwrite=overwrite), umask) + assert destination.read_bytes() == b"payload" + + +def test_remote_file_download(tmp_path, umask): + file = object.__new__(blosc2.RemoteFile) + file._owner = SimpleNamespace(lock=threading.RLock()) + file._meta = {"nchunks": 2, "nbytes": 5} + file._check_open = lambda: None + file._chunk = lambda index, cancel: [b"abc", b"de"][index] + destination = tmp_path / "download.txt" + check_modes(destination, lambda overwrite: file.download(destination, overwrite=overwrite), umask) + assert destination.read_bytes() == b"abcde"