From fe934a54537fb4d9d6a2b661191f6031354cfb67 Mon Sep 17 00:00:00 2001 From: Fabian Isensee Date: Thu, 5 Nov 2026 23:01:47 +0100 Subject: [PATCH 1/2] Free vlmeta contents and name arrays; check meta reads vlmeta.get_vlmeta() copied the buffer that blosc2_vlmeta_get() mallocs into a bytes object and never freed it, so every vlmeta read (including `name in vlmeta`, `vlmeta[:]` and NDArray attribute reads) leaked the decompressed content. vlmeta.get_names() and to_dict() leaked their array of name pointers in the same way. to_dict() also kept those pointers across its get_vlmeta() calls, which re-sync (free and reload) the vlmetalayers when another handle has rewritten the frame, so it read freed names. It now copies the names (as the bytes keys it returns) and frees the array before reading. meta__getitem__() ignored the return code of blosc2_meta_get(): for a missing name it copied and freed an uninitialized pointer, so `schunk.meta["missing"] = x` (which does not check the name first) aborted the process. It now raises KeyError, or RuntimeError for other errors, and meta__setitem__() passes the name as given so the message names it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01DiPFztdpnKMJkzrjT36BuP --- src/blosc2/blosc2_ext.pyx | 56 ++++++++++++++++++++++++--------------- tests/test_schunk.py | 9 +++++++ tests/test_swmr.py | 15 +++++++++++ tests/test_vlmeta.py | 34 ++++++++++++++++++++++++ 4 files changed, 92 insertions(+), 22 deletions(-) diff --git a/src/blosc2/blosc2_ext.pyx b/src/blosc2/blosc2_ext.pyx index de90e254c..fc59fc7f4 100644 --- a/src/blosc2/blosc2_ext.pyx +++ b/src/blosc2/blosc2_ext.pyx @@ -5733,12 +5733,15 @@ cdef class vlmeta: cdef int32_t content_len if rc < 0: raise KeyError - if rc >= 0: - rc = blosc2_vlmeta_get(self.schunk, name, &content, &content_len) + rc = blosc2_vlmeta_get(self.schunk, name, &content, &content_len) + # On errors, blosc2_vlmeta_get() leaves nothing to free if rc < 0: _raise_callback_error() raise RuntimeError - return content[:content_len] + try: + return content[:content_len] + finally: + free(content) @_handle_locked def del_vlmeta(self, name): @@ -5754,22 +5757,26 @@ cdef class vlmeta: @_handle_locked def get_names(self): cdef char** names = malloc(self.schunk.nvlmetalayers * sizeof (char *)) - rc = blosc2_vlmeta_get_names(self.schunk, names) - if rc != self.schunk.nvlmetalayers: - raise RuntimeError - res = [names[i].decode("utf-8") for i in range(rc)] - return res + try: + rc = blosc2_vlmeta_get_names(self.schunk, names) + if rc != self.schunk.nvlmetalayers: + raise RuntimeError + return [names[i].decode("utf-8") for i in range(rc)] + finally: + free(names) @_handle_locked def to_dict(self): cdef char** names = malloc(self.schunk.nvlmetalayers * sizeof (char*)) - rc = blosc2_vlmeta_get_names(self.schunk, names) - if rc != self.schunk.nvlmetalayers: - raise RuntimeError - res = {} - for i in range(rc): - res[names[i]] = unpackb(self.get_vlmeta(names[i]), list_hook=decode_tuple) - return res + try: + rc = blosc2_vlmeta_get_names(self.schunk, names) + if rc != self.schunk.nvlmetalayers: + raise RuntimeError + # Copies (as bytes keys): the names point into the vlmetalayers, which get_vlmeta() may re-sync + keys = [names[i] for i in range(rc)] + finally: + free(names) + return {key: unpackb(self.get_vlmeta(key), list_hook=decode_tuple) for key in keys} @_handle_locked @@ -5782,20 +5789,25 @@ def meta__contains__(self, name): @_handle_locked def meta__getitem__(self, name): cdef blosc2_schunk *schunk = self.c_schunk - name = name.encode("utf-8") if isinstance(name, str) else name + name_ = name.encode("utf-8") if isinstance(name, str) else name cdef uint8_t *content cdef int32_t content_len - n = blosc2_meta_get(schunk, name, &content, &content_len) - res = PyBytes_FromStringAndSize( content, content_len) - free(content) - - return res + cdef int n = blosc2_meta_get(schunk, name_, &content, &content_len) + # On errors, blosc2_meta_get() leaves nothing to free (content may be uninitialized) + if n == BLOSC2_ERROR_NOT_FOUND: + raise KeyError(f"{name} not found") + if n < 0: + raise RuntimeError("Could not get the metalayer") + try: + return PyBytes_FromStringAndSize( content, content_len) + finally: + free(content) @_handle_locked def meta__setitem__(self, name, content): cdef blosc2_schunk *schunk = self.c_schunk - name = name.encode("utf-8") if isinstance(name, str) else name old_content = meta__getitem__(self, name) + name = name.encode("utf-8") if isinstance(name, str) else name if len(old_content) != len(content): raise ValueError("The length of the content in a metalayer cannot change.") blosc2_meta_update(schunk, name, content, len(content)) diff --git a/tests/test_schunk.py b/tests/test_schunk.py index 03aa802d7..cbee008b7 100644 --- a/tests/test_schunk.py +++ b/tests/test_schunk.py @@ -186,6 +186,15 @@ def test_schunk(contiguous, urlpath, mode, mmap_mode, nbytes, cparams, dparams, blosc2.remove_urlpath(urlpath) +def test_schunk_meta_setitem_missing_name(): + # This used to copy and free an uninitialized pointer, which aborted the process + schunk = blosc2.SChunk(meta={"test": 1}) + with pytest.raises(KeyError, match="missing not found"): + schunk.meta["missing"] = 2 + assert "missing" not in schunk.meta + assert schunk.meta["test"] == 1 + + def test_schunk_info_has_human_sizes(): schunk = blosc2.SChunk(chunksize=32) schunk.append_data(b"a" * 32) diff --git a/tests/test_swmr.py b/tests/test_swmr.py index f2cb340af..d039f08c9 100644 --- a/tests/test_swmr.py +++ b/tests/test_swmr.py @@ -136,6 +136,21 @@ def test_vlmeta_follows(tmp_path): assert r.schunk.vlmeta["heartbeat"] == "brand-new and longer" +def test_vlmeta_to_dict_follows(tmp_path): + # to_dict() used to keep pointers to the cached names across its first read, which re-syncs + # (frees and reloads) the vlmetalayers after another handle has rewritten them + urlpath = tmp_path / "vlmeta_to_dict.b2nd" + w = create_array(urlpath) + for i in range(8): + w.schunk.vlmeta[f"name{i}"] = i + r = blosc2.open(str(urlpath)) + + w.schunk.vlmeta["name0"] = "brand-new and longer" + expected = {f"name{i}".encode(): i for i in range(8)} + expected[b"name0"] = "brand-new and longer" + assert r.schunk.vlmeta.to_dict() == expected + + WRITER_SCRIPT = """ import sys import numpy as np diff --git a/tests/test_vlmeta.py b/tests/test_vlmeta.py index 51bc5eac1..36a8cc891 100644 --- a/tests/test_vlmeta.py +++ b/tests/test_vlmeta.py @@ -5,6 +5,9 @@ # SPDX-License-Identifier: BSD-3-Clause ####################################################################### +import os +import sys + import numpy as np import pytest @@ -150,3 +153,34 @@ def test_vlmeta_empty_list_roundtrip(): assert schunk.vlmeta["empty"] == [] assert schunk.vlmeta["nested"] == {"rows": [[], ["x"]]} + + +def _rss_bytes(): + with open("/proc/self/statm") as f: + return int(f.read().split()[1]) * os.sysconf("SC_PAGE_SIZE") + + +def _rss_growth(op, n): + for _ in range(10): + op() + rss = _rss_bytes() + for _ in range(n): + op() + return _rss_bytes() - rss + + +@pytest.mark.skipif(not sys.platform.startswith("linux"), reason="reads the RSS from /proc") +def test_vlmeta_read_frees_content(): + # Each read used to leak the 1 MiB content (200 MiB here) + schunk = blosc2.SChunk() + schunk.vlmeta["big"] = bytes(2**20) + assert _rss_growth(lambda: schunk.vlmeta["big"], 200) < 50 * 2**20 + + +@pytest.mark.skipif(not sys.platform.startswith("linux"), reason="reads the RSS from /proc") +def test_vlmeta_names_are_freed(): + # Each listing used to leak an array of 4096 pointers (32 MiB here) + schunk = blosc2.SChunk() + for i in range(4096): + schunk.vlmeta[str(i)] = i + assert _rss_growth(lambda: list(schunk.vlmeta), 1000) < 8 * 2**20 From b5bf3009a89a5a74fe983bd34260f59a8269ebf2 Mon Sep 17 00:00:00 2001 From: Fabian Isensee Date: Fri, 6 Nov 2026 09:02:52 +0100 Subject: [PATCH 2/2] Look up a vlmeta name once per read vlmeta.get_vlmeta() called blosc2_vlmeta_exists() and then blosc2_vlmeta_get(), which does the same lookup again. Each lookup re-syncs a file-backed frame (frame_check_stale() stats the file and reads the frame header, plus the lock calls with locking), so every vlmeta read, `name in vlmeta` hit and vlmeta.get() hit did that twice. Call blosc2_vlmeta_get() alone. It returns DATA, MEMORY_ALLOC, NULL_POINTER or READ_BUFFER when reading the content fails, which stay RuntimeError; every other error comes from its lookup (name not found, name too long, frame re-sync or lock) and stays KeyError, as when the separate blosc2_vlmeta_exists() call rejected it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01DiPFzsfE5i74cBSxZN13M8 --- src/blosc2/blosc2_ext.pyx | 10 +++++++--- tests/test_vlmeta.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/blosc2/blosc2_ext.pyx b/src/blosc2/blosc2_ext.pyx index fc59fc7f4..f5f4710e1 100644 --- a/src/blosc2/blosc2_ext.pyx +++ b/src/blosc2/blosc2_ext.pyx @@ -237,6 +237,7 @@ cdef extern from "blosc2.h": BLOSC2_ERROR_SCHUNK_SPECIAL BLOSC2_ERROR_PLUGIN_IO BLOSC2_ERROR_FILE_REMOVE + BLOSC2_ERROR_NULL_POINTER ctypedef enum: BLOSC2_DEFINED_CODECS_START @@ -5728,12 +5729,15 @@ cdef class vlmeta: @_handle_locked def get_vlmeta(self, name): name = name.encode("utf-8") if isinstance(name, str) else name - rc = blosc2_vlmeta_exists(self.schunk, name) cdef uint8_t* content cdef int32_t content_len - if rc < 0: + # One lookup (for a file-backed frame, each lookup reads the frame header). These four errors + # come from reading the content; the others are lookup errors (not found, name too long, frame + # re-sync or lock), which were a KeyError from a blosc2_vlmeta_exists() call before and still are. + cdef int rc = blosc2_vlmeta_get(self.schunk, name, &content, &content_len) + if rc < 0 and rc not in (BLOSC2_ERROR_DATA, BLOSC2_ERROR_MEMORY_ALLOC, BLOSC2_ERROR_NULL_POINTER, + BLOSC2_ERROR_READ_BUFFER): raise KeyError - rc = blosc2_vlmeta_get(self.schunk, name, &content, &content_len) # On errors, blosc2_vlmeta_get() leaves nothing to free if rc < 0: _raise_callback_error() diff --git a/tests/test_vlmeta.py b/tests/test_vlmeta.py index 36a8cc891..c0b81e172 100644 --- a/tests/test_vlmeta.py +++ b/tests/test_vlmeta.py @@ -184,3 +184,34 @@ def test_vlmeta_names_are_freed(): for i in range(4096): schunk.vlmeta[str(i)] = i assert _rss_growth(lambda: list(schunk.vlmeta), 1000) < 8 * 2**20 + + +def _read_syscalls(): + # Read syscalls (read, pread, ...) of this process so far + with open("/proc/self/io") as f: + return next(int(line.split()[1]) for line in f if line.startswith("syscr:")) + + +@pytest.mark.skipif(not os.path.exists("/proc/self/io"), reason="needs Linux task I/O accounting") +@pytest.mark.parametrize("contiguous", [True, False]) +def test_vlmeta_read_looks_name_up_once(tmp_path, contiguous): + # Each lookup re-reads the frame header from the file (one pread); reads used to look the name up twice + urlpath = str(tmp_path / "frame.b2frame") + schunk = blosc2.SChunk(urlpath=urlpath, contiguous=contiguous) + schunk.vlmeta["small"] = 1 + reader = blosc2.open(urlpath, mode="r") + assert reader.vlmeta["small"] == 1 + n = 100 + syscr = _read_syscalls() + for _ in range(n): + reader.vlmeta["small"] + # One read syscall per vlmeta read (two before) + assert _read_syscalls() - syscr < 1.5 * n + + +def test_vlmeta_name_too_long_is_keyerror(): + # Longer than BLOSC2_METALAYER_NAME_MAXLEN: the lookup rejects it, so it cannot exist + schunk = blosc2.SChunk() + assert "x" * 40 not in schunk.vlmeta + with pytest.raises(KeyError): + schunk.vlmeta["x" * 40]