Write .b2z stores and downloads with the umask's permissions again, not 0600

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

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:

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)

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

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)

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)

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:

tests/test_file_mode.py

@@ -0,0 +1,144 @@
+#######################################################################
+# Copyright (c) 2019-present, Blosc Development Team <blosc@blosc.org>
+# 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"