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