From 5da648d45078912ac90e95b6293089845be6b9da Mon Sep 17 00:00:00 2001 From: selmanozleyen Date: Fri, 28 Aug 2026 09:59:03 +0200 Subject: [PATCH 1/5] fix: refuse writes to a store zarr-python opened read-only zarr-python enforces read-only in the store itself -- `Store._check_writable`, reached from the concrete store's `_set`. This pipeline never gets there: it is handed a `StoreConfig` and builds its own Rust store, writable whatever mode the array was opened in, so a write to a `mode="r"` array succeeds here and raises through the default pipeline. Silent divergence, and silent data loss for anyone relying on the mode. The flag has to be carried from Python because nothing on the Rust side can infer it: the mode is zarr-python's, not the store's. One guard, at the only entry point Python can write through. `store_chunk_bytes` and `store_chunk_subset_bytes` are reached only from it. The tests open STRICT. zarr's own refusal message is byte-identical to the guard's, so with a fallback available they would pass whether the guard fired or zarr-python served the write. --- python/zarrs/_internal.pyi | 1 + python/zarrs/pipeline.py | 1 + src/lib.rs | 13 ++++++++ tests/test_read_only_store.py | 56 +++++++++++++++++++++++++++++++++++ 4 files changed, 71 insertions(+) create mode 100644 tests/test_read_only_store.py diff --git a/python/zarrs/_internal.pyi b/python/zarrs/_internal.pyi index 2f676818..d324408a 100644 --- a/python/zarrs/_internal.pyi +++ b/python/zarrs/_internal.pyi @@ -31,6 +31,7 @@ class CodecPipelineImpl: num_threads: builtins.int | None = None, direct_io: builtins.bool = False, file_handle_cache_size: builtins.int = 0, + store_is_read_only: builtins.bool = False, ) -> CodecPipelineImpl: ... def retrieve_chunks_and_apply_index( self, diff --git a/python/zarrs/pipeline.py b/python/zarrs/pipeline.py index b3368219..9bb3ccbb 100644 --- a/python/zarrs/pipeline.py +++ b/python/zarrs/pipeline.py @@ -65,6 +65,7 @@ def get_codec_pipeline_impl( file_handle_cache_size=config.get( "codec_pipeline.file_handle_cache_size", 0 ), + store_is_read_only=store.read_only, ) except TypeError as e: if strict: diff --git a/src/lib.rs b/src/lib.rs index 9ef05f2d..fa7cde07 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -52,6 +52,11 @@ pub(crate) struct CodecPipelineImpl { pub(crate) num_threads: usize, pub(crate) fill_value: FillValue, pub(crate) data_type: DataType, + /// Whether zarr-python opened this store read-only. + /// + /// Not inferable here: `StoreConfig` builds a writable Rust store whatever mode the + /// array was opened in. + pub(crate) store_is_read_only: bool, } impl CodecPipelineImpl { @@ -219,6 +224,7 @@ impl CodecPipelineImpl { num_threads=None, direct_io=false, file_handle_cache_size=0, + store_is_read_only=false, ))] #[new] fn new( @@ -230,6 +236,7 @@ impl CodecPipelineImpl { num_threads: Option, direct_io: bool, file_handle_cache_size: usize, + store_is_read_only: bool, ) -> PyResult { store_config.direct_io(direct_io); store_config.file_handle_cache_size(file_handle_cache_size); @@ -278,6 +285,7 @@ impl CodecPipelineImpl { num_threads, fill_value, data_type, + store_is_read_only, }) } @@ -397,6 +405,11 @@ impl CodecPipelineImpl { value: &Bound<'_, PyUntypedArray>, write_empty_chunks: bool, ) -> PyResult<()> { + if self.store_is_read_only { + return Err(PyValueError::new_err( + "store was opened in read-only mode and does not support writing", + )); + } enum InputValue<'a> { Array(ArrayBytes<'a>), Constant(FillValue), diff --git a/tests/test_read_only_store.py b/tests/test_read_only_store.py new file mode 100644 index 00000000..24fd65b9 --- /dev/null +++ b/tests/test_read_only_store.py @@ -0,0 +1,56 @@ +"""A store opened read-only must refuse writes, as zarr-python's own pipeline does. + +zarr-python enforces this in the store itself -- `Store._check_writable`, reached from the +concrete store's `_set`. This pipeline never gets there: it is handed a `StoreConfig` and +builds its own Rust store, writable whatever mode the array was opened in. Without the guard +a write to a `mode="r"` array SUCCEEDS here and raises through the default pipeline. + +Opened STRICT throughout, and that is what makes the assertion mean anything: zarr's own +refusal message is byte-identical to the Rust guard's, so with a fallback available these +tests would pass whether the guard fired or zarr-python served the write. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +import numpy as np +import pytest +import zarr + +if TYPE_CHECKING: + from pathlib import Path + + +@pytest.fixture +def array(tmp_path: Path) -> tuple[Path, np.ndarray]: + values = np.arange(64, dtype=np.float32) + path = tmp_path / "a" + zarr.create_array(path, dtype=values.dtype, shape=values.shape, chunks=(16,))[:] = ( + values + ) + return path, values + + +def open_strict(path: Path, mode: str) -> zarr.Array: + """Open with no fallback. `strict` has to be set BEFORE the open: that is when the + pipeline decides whether it has one.""" + with zarr.config.set({"codec_pipeline.strict": True}): + return zarr.open_array(path, mode=mode) + + +def test_write_to_a_read_only_array_raises(array: tuple[Path, np.ndarray]) -> None: + path, values = array + z = open_strict(path, "r") + with pytest.raises(ValueError, match="read-only"): + z[0:16] = -1.0 + np.testing.assert_array_equal(zarr.open_array(path, mode="r")[:], values) + + +def test_a_writable_array_still_writes(array: tuple[Path, np.ndarray]) -> None: + path, values = array + z = open_strict(path, "r+") + z[0:16] = -1.0 + expected = values.copy() + expected[0:16] = -1.0 + np.testing.assert_array_equal(zarr.open_array(path, mode="r")[:], expected) From 522f389a40fe3ebd67131fbcb95152a8d55fa3ea Mon Sep 17 00:00:00 2001 From: selmanozleyen Date: Mon, 31 Aug 2026 18:36:28 +0200 Subject: [PATCH 2/5] fix: remove store_is_read_only parameter and update usage to reflect read-only state from StoreConfig --- python/zarrs/_internal.pyi | 1 - python/zarrs/pipeline.py | 1 - src/lib.rs | 9 ++--- src/store.rs | 71 +++++++++++++++++++++++--------------- 4 files changed, 46 insertions(+), 36 deletions(-) diff --git a/python/zarrs/_internal.pyi b/python/zarrs/_internal.pyi index d324408a..2f676818 100644 --- a/python/zarrs/_internal.pyi +++ b/python/zarrs/_internal.pyi @@ -31,7 +31,6 @@ class CodecPipelineImpl: num_threads: builtins.int | None = None, direct_io: builtins.bool = False, file_handle_cache_size: builtins.int = 0, - store_is_read_only: builtins.bool = False, ) -> CodecPipelineImpl: ... def retrieve_chunks_and_apply_index( self, diff --git a/python/zarrs/pipeline.py b/python/zarrs/pipeline.py index 9bb3ccbb..b3368219 100644 --- a/python/zarrs/pipeline.py +++ b/python/zarrs/pipeline.py @@ -65,7 +65,6 @@ def get_codec_pipeline_impl( file_handle_cache_size=config.get( "codec_pipeline.file_handle_cache_size", 0 ), - store_is_read_only=store.read_only, ) except TypeError as e: if strict: diff --git a/src/lib.rs b/src/lib.rs index fa7cde07..443ec1a0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -52,10 +52,7 @@ pub(crate) struct CodecPipelineImpl { pub(crate) num_threads: usize, pub(crate) fill_value: FillValue, pub(crate) data_type: DataType, - /// Whether zarr-python opened this store read-only. - /// - /// Not inferable here: `StoreConfig` builds a writable Rust store whatever mode the - /// array was opened in. + /// Whether zarr-python opened this store read-only; see `StoreConfig::read_only`. pub(crate) store_is_read_only: bool, } @@ -224,7 +221,6 @@ impl CodecPipelineImpl { num_threads=None, direct_io=false, file_handle_cache_size=0, - store_is_read_only=false, ))] #[new] fn new( @@ -236,7 +232,6 @@ impl CodecPipelineImpl { num_threads: Option, direct_io: bool, file_handle_cache_size: usize, - store_is_read_only: bool, ) -> PyResult { store_config.direct_io(direct_io); store_config.file_handle_cache_size(file_handle_cache_size); @@ -285,7 +280,7 @@ impl CodecPipelineImpl { num_threads, fill_value, data_type, - store_is_read_only, + store_is_read_only: store_config.read_only, }) } diff --git a/src/store.rs b/src/store.rs index 0176fa85..b0ad2ff5 100644 --- a/src/store.rs +++ b/src/store.rs @@ -22,7 +22,17 @@ pub use self::http::HttpStoreConfig; pub use self::obstore::ObStoreConfig; #[derive(Debug, Clone)] -pub enum StoreConfig { +pub struct StoreConfig { + pub kind: StoreKind, + /// Whether zarr-python opened the store read-only, i.e. `mode="r"`. + /// + /// zarr-python resolves the mode into the store itself (`Store.read_only`), so it comes + /// off the same object every other field here does. + pub read_only: bool, +} + +#[derive(Debug, Clone)] +pub enum StoreKind { Filesystem(FilesystemStoreConfig), Http(HttpStoreConfig), ObStore(ObStoreConfig), @@ -35,10 +45,10 @@ impl<'py> FromPyObject<'_, 'py> for StoreConfig { fn extract(store: Borrowed<'_, 'py, PyAny>) -> PyResult { let name = store.get_type().name()?; let name = name.to_str()?; - match name { + let kind = match name { "LocalStore" => { let root: String = store.getattr("root")?.call_method0("__str__")?.extract()?; - Ok(StoreConfig::Filesystem(FilesystemStoreConfig::new(root))) + StoreKind::Filesystem(FilesystemStoreConfig::new(root)) } "FsspecStore" => { let fs = store.getattr("fs")?; @@ -48,13 +58,14 @@ impl<'py> FromPyObject<'_, 'py> for StoreConfig { let storage_options: HashMap> = fs.getattr("storage_options")?.extract()?; match fs_name { - "HTTPFileSystem" => Ok(StoreConfig::Http(HttpStoreConfig::new( - &path, - &storage_options, - )?)), - _ => Err(PyErr::new::(format!( - "zarrs-python does not support {fs_name} (FsspecStore) stores" - ))), + "HTTPFileSystem" => { + StoreKind::Http(HttpStoreConfig::new(&path, &storage_options)?) + } + _ => { + return Err(PyErr::new::(format!( + "zarrs-python does not support {fs_name} (FsspecStore) stores" + ))); + } } } "ObjectStore" => { @@ -62,29 +73,35 @@ impl<'py> FromPyObject<'_, 'py> for StoreConfig { let external_object_store: PyExternalObjectStore = underlying_store.extract()?; let object_store: Arc = external_object_store.into_dyn(); - Ok(StoreConfig::ObStore(ObStoreConfig::new(object_store))) + StoreKind::ObStore(ObStoreConfig::new(object_store)) } - _ => Err(PyErr::new::(format!( - "zarrs-python does not support {name} stores" - ))), - } + _ => { + return Err(PyErr::new::(format!( + "zarrs-python does not support {name} stores" + ))); + } + }; + Ok(StoreConfig { + kind, + read_only: store.getattr("read_only")?.extract()?, + }) } } impl StoreConfig { pub fn direct_io(&mut self, flag: bool) { - match self { - StoreConfig::Filesystem(config) => config.direct_io(flag), - StoreConfig::Http(_config) => (), - StoreConfig::ObStore(_config) => (), + match &mut self.kind { + StoreKind::Filesystem(config) => config.direct_io(flag), + StoreKind::Http(_config) => (), + StoreKind::ObStore(_config) => (), } } pub fn file_handle_cache_size(&mut self, size: usize) { - match self { - StoreConfig::Filesystem(config) => config.file_handle_cache_size(size), - StoreConfig::Http(_config) => (), - StoreConfig::ObStore(_config) => (), + match &mut self.kind { + StoreKind::Filesystem(config) => config.file_handle_cache_size(size), + StoreKind::Http(_config) => (), + StoreKind::ObStore(_config) => (), } } } @@ -99,10 +116,10 @@ impl TryFrom<&StoreConfig> for ReadableWritableListableStorage { type Error = PyErr; fn try_from(value: &StoreConfig) -> Result { - match value { - StoreConfig::Filesystem(config) => config.try_into(), - StoreConfig::Http(config) => config.try_into(), - StoreConfig::ObStore(config) => config.try_into(), + match &value.kind { + StoreKind::Filesystem(config) => config.try_into(), + StoreKind::Http(config) => config.try_into(), + StoreKind::ObStore(config) => config.try_into(), } } } From 409ba1330843a1939d8f539a776dd127e1fb54e9 Mon Sep 17 00:00:00 2001 From: selmanozleyen Date: Tue, 1 Sep 2026 00:37:13 +0200 Subject: [PATCH 3/5] fix: update CodecPipelineImpl to use ReadableStorage and handle read-only state correctly --- src/lib.rs | 51 ++++++++++++++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 17 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 443ec1a0..686e6960 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -26,7 +26,7 @@ use zarrs::array::{ use zarrs::config::global_config; use zarrs::convert::array_metadata_v2_to_v3; use zarrs::plugin::ZarrVersion; -use zarrs::storage::{ReadableWritableListableStorage, StorageHandle, StoreKey}; +use zarrs::storage::{ReadableStorage, ReadableWritableListableStorage, StorageHandle, StoreKey}; mod chunk_item; mod concurrency; @@ -44,7 +44,12 @@ use crate::utils::{PyCodecErrExt, PyErrExt as _}; #[gen_stub_pyclass] #[pyclass] pub(crate) struct CodecPipelineImpl { - pub(crate) store: ReadableWritableListableStorage, + /// Read handle, deliberately the READ-ONLY type: `set`/`erase` are not in its interface, + /// so the only way to reach a write is `writable()`. + pub(crate) store_read: ReadableStorage, + /// The writable handle -- `None` when zarr-python opened the store read-only. Same object + /// as `store_read`; the read-only case simply never keeps a writable view of it. + pub(crate) store_write: Option, pub(crate) codec_chain: Arc, pub(crate) codec_options: CodecOptions, pub(crate) chunk_concurrent_minimum: usize, @@ -52,8 +57,6 @@ pub(crate) struct CodecPipelineImpl { pub(crate) num_threads: usize, pub(crate) fill_value: FillValue, pub(crate) data_type: DataType, - /// Whether zarr-python opened this store read-only; see `StoreConfig::read_only`. - pub(crate) store_is_read_only: bool, } impl CodecPipelineImpl { @@ -63,7 +66,10 @@ impl CodecPipelineImpl { codec_chain: &CodecChain, codec_options: &CodecOptions, ) -> PyResult> { - let value_encoded = self.store.get(&item.key).map_py_err::()?; + let value_encoded = self + .store_read + .get(&item.key) + .map_py_err::()?; let value_decoded = if let Some(value_encoded) = value_encoded { let value_encoded: Vec = value_encoded.into(); // zero-copy in this case codec_chain @@ -82,6 +88,13 @@ impl CodecPipelineImpl { Ok(value_decoded) } + /// The writable store, or zarr-python's own refusal, verbatim. + fn writable(&self) -> PyResult<&ReadableWritableListableStorage> { + self.store_write.as_ref().ok_or_else(|| { + PyValueError::new_err("store was opened in read-only mode and does not support writing") + }) + } + fn store_chunk_bytes( &self, item: &ChunkItem, @@ -93,8 +106,10 @@ impl CodecPipelineImpl { .validate(item.num_elements, &self.data_type) .map_codec_err()?; + let store = self.writable()?; + if value_decoded.is_fill_value(&self.fill_value) { - self.store.erase(&item.key).map_py_err::() + store.erase(&item.key).map_py_err::() } else { let value_encoded = codec_chain .encode( @@ -108,7 +123,7 @@ impl CodecPipelineImpl { .map_codec_err()?; // Store the encoded chunk - self.store + store .set(&item.key, value_encoded.into()) .map_py_err::() } @@ -254,6 +269,8 @@ impl CodecPipelineImpl { let store: ReadableWritableListableStorage = (&store_config).try_into().map_py_err::()?; + let store_write = (!store_config.read_only).then(|| store.clone()); + let store_read: ReadableStorage = store.readable(); let data_type = DataType::from_metadata(&metadata_v3.data_type).map_py_err::()?; @@ -272,7 +289,7 @@ impl CodecPipelineImpl { .map_py_err::()?; Ok(Self { - store, + store_read, codec_chain, codec_options, chunk_concurrent_minimum, @@ -280,7 +297,7 @@ impl CodecPipelineImpl { num_threads, fill_value, data_type, - store_is_read_only: store_config.read_only, + store_write, }) } @@ -311,7 +328,7 @@ impl CodecPipelineImpl { if !partial_chunk_items.is_empty() { let key_decoder_pairs = iter_concurrent_limit!(chunk_concurrent_limit, partial_chunk_items, map, |item| { - let storage_handle = Arc::new(StorageHandle::new(self.store.clone())); + let storage_handle = Arc::new(StorageHandle::new(self.store_read.clone())); let input_handle = StoragePartialDecoder::new(storage_handle, item.key.clone()); let partial_decoder = self .codec_chain @@ -355,8 +372,10 @@ impl CodecPipelineImpl { // See zarrs::array::Array::retrieve_chunk_subset_into if is_whole_chunk(&item) { // See zarrs::array::Array::retrieve_chunk_into - if let Some(chunk_encoded) = - self.store.get(&item.key).map_py_err::()? + if let Some(chunk_encoded) = self + .store_read + .get(&item.key) + .map_py_err::()? { // Decode the encoded data into the output buffer let chunk_encoded: Vec = chunk_encoded.into(); @@ -400,11 +419,9 @@ impl CodecPipelineImpl { value: &Bound<'_, PyUntypedArray>, write_empty_chunks: bool, ) -> PyResult<()> { - if self.store_is_read_only { - return Err(PyValueError::new_err( - "store was opened in read-only mode and does not support writing", - )); - } + // Fail before decoding anything; the write site checks again by construction. + self.writable()?; + enum InputValue<'a> { Array(ArrayBytes<'a>), Constant(FillValue), From 40deb6e3b2c3e2a31de1cc6a0f61160e601381a7 Mon Sep 17 00:00:00 2001 From: selmanozleyen Date: Tue, 1 Sep 2026 00:37:37 +0200 Subject: [PATCH 4/5] fix: update documentation to clarify read-only behavior in Rust store pipeline --- tests/test_read_only_store.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/test_read_only_store.py b/tests/test_read_only_store.py index 24fd65b9..bb58c0c6 100644 --- a/tests/test_read_only_store.py +++ b/tests/test_read_only_store.py @@ -2,11 +2,12 @@ zarr-python enforces this in the store itself -- `Store._check_writable`, reached from the concrete store's `_set`. This pipeline never gets there: it is handed a `StoreConfig` and -builds its own Rust store, writable whatever mode the array was opened in. Without the guard -a write to a `mode="r"` array SUCCEEDS here and raises through the default pipeline. +builds its own Rust store, so nothing zarr-python checks is in the path -- a `mode="r"` write +SUCCEEDS unless this side refuses it. It refuses by keeping a writable handle only when the +store is writable; the read handle has no `set`/`erase` in its interface at all. Opened STRICT throughout, and that is what makes the assertion mean anything: zarr's own -refusal message is byte-identical to the Rust guard's, so with a fallback available these +refusal message is byte-identical to the Rust one, so with a fallback available these tests would pass whether the guard fired or zarr-python served the write. """ From ecc20a54c05725cbac71eab4842c1d264894f008 Mon Sep 17 00:00:00 2001 From: selmanozleyen Date: Tue, 1 Sep 2026 16:57:19 +0200 Subject: [PATCH 5/5] applied @ilan-gold's suggestions --- src/lib.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 686e6960..c53e73de 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -46,10 +46,10 @@ use crate::utils::{PyCodecErrExt, PyErrExt as _}; pub(crate) struct CodecPipelineImpl { /// Read handle, deliberately the READ-ONLY type: `set`/`erase` are not in its interface, /// so the only way to reach a write is `writable()`. - pub(crate) store_read: ReadableStorage, + pub(crate) readable_store: ReadableStorage, /// The writable handle -- `None` when zarr-python opened the store read-only. Same object - /// as `store_read`; the read-only case simply never keeps a writable view of it. - pub(crate) store_write: Option, + /// as `readable_store`; the read-only case simply never keeps a writable view of it. + pub(crate) writable_store: Option, pub(crate) codec_chain: Arc, pub(crate) codec_options: CodecOptions, pub(crate) chunk_concurrent_minimum: usize, @@ -67,7 +67,7 @@ impl CodecPipelineImpl { codec_options: &CodecOptions, ) -> PyResult> { let value_encoded = self - .store_read + .readable_store .get(&item.key) .map_py_err::()?; let value_decoded = if let Some(value_encoded) = value_encoded { @@ -90,7 +90,7 @@ impl CodecPipelineImpl { /// The writable store, or zarr-python's own refusal, verbatim. fn writable(&self) -> PyResult<&ReadableWritableListableStorage> { - self.store_write.as_ref().ok_or_else(|| { + self.writable_store.as_ref().ok_or_else(|| { PyValueError::new_err("store was opened in read-only mode and does not support writing") }) } @@ -269,8 +269,8 @@ impl CodecPipelineImpl { let store: ReadableWritableListableStorage = (&store_config).try_into().map_py_err::()?; - let store_write = (!store_config.read_only).then(|| store.clone()); - let store_read: ReadableStorage = store.readable(); + let writable_store = (!store_config.read_only).then(|| store.clone()); + let readable_store: ReadableStorage = store.readable(); let data_type = DataType::from_metadata(&metadata_v3.data_type).map_py_err::()?; @@ -289,7 +289,7 @@ impl CodecPipelineImpl { .map_py_err::()?; Ok(Self { - store_read, + readable_store, codec_chain, codec_options, chunk_concurrent_minimum, @@ -297,7 +297,7 @@ impl CodecPipelineImpl { num_threads, fill_value, data_type, - store_write, + writable_store, }) } @@ -328,7 +328,7 @@ impl CodecPipelineImpl { if !partial_chunk_items.is_empty() { let key_decoder_pairs = iter_concurrent_limit!(chunk_concurrent_limit, partial_chunk_items, map, |item| { - let storage_handle = Arc::new(StorageHandle::new(self.store_read.clone())); + let storage_handle = Arc::new(StorageHandle::new(self.readable_store.clone())); let input_handle = StoragePartialDecoder::new(storage_handle, item.key.clone()); let partial_decoder = self .codec_chain @@ -373,7 +373,7 @@ impl CodecPipelineImpl { if is_whole_chunk(&item) { // See zarrs::array::Array::retrieve_chunk_into if let Some(chunk_encoded) = self - .store_read + .readable_store .get(&item.key) .map_py_err::()? {