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"