From fe934a54537fb4d9d6a2b661191f6031354cfb67 Mon Sep 17 00:00:00 2001 From: Fabian Isensee Date: Thu, 5 Nov 2026 23:01:47 +0100 Subject: [PATCH] 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