From 249a147f38470701ed103c8592f2508869067cfb Mon Sep 17 00:00:00 2001 From: Erik Welch Date: Fri, 31 Jul 2026 11:53:51 -0500 Subject: [PATCH 1/2] Hold the owner of a borrowed GraphBLAS handle in TypedOpBase `SelectOp._from_indexunary` reuses the IndexUnaryOp's `GrB_IndexUnaryOp` rather than allocating a second one, and marked the borrowing typed op `_owns_gb_obj_inst = False` so that only one side would free it. Nothing kept the owner alive, though: `register_anonymous` drops the IndexUnaryOp on the way out, so its typed op is collected and frees the handle while the SelectOp is still pointing at it. The result is a dangling pointer rather than a NULL one. `__del__` frees through a synthesized cell, `ffi.new(f"{c_type_name}*", gb_obj)`, so `GrB_*_free` clears that temporary and leaves the op's own `gb_obj` holding the old address. Reproduced at 6f1eb02: def _ne_thunk(x, i, j, thunk): return x != thunk sel = SelectOp.register_anonymous(_ne_thunk) gc.collect() Vector.from_coo([0, 1, 2], [1, 5, 9]).select(sel, 5).new() -> UninitializedObject Whether it raises depends on whether anything happens to retain the IndexUnaryOp. With `x > thunk` it survives, because a traceback caught inside the per-dtype compile loop keeps a frame that references it. Replace the per-instance opt-out with `_gb_obj_owner`, naming the owning typed op. That suppresses our free and keeps the handle alive for as long as we can reach it; disclaiming ownership without naming an owner is what left the handle dangling. The opt-out being replaced was added in 6f1eb02 to stop a double free, so the sequence is leak, double free, opt-out, dangling handle, and now an owned reference. --- graphblas/core/operator/base.py | 28 +++++++++++++++------------- graphblas/core/operator/select.py | 11 +++++++---- graphblas/tests/test_op.py | 21 +++++++++++++++++++++ 3 files changed, 43 insertions(+), 17 deletions(-) diff --git a/graphblas/core/operator/base.py b/graphblas/core/operator/base.py index bf685ba5c..a893ba3ad 100644 --- a/graphblas/core/operator/base.py +++ b/graphblas/core/operator/base.py @@ -534,16 +534,13 @@ class TypedOpBase: "gb_name", "_type2", "_jit_c_info", - "_owns_gb_obj_inst", + "_gb_obj_owner", "__weakref__", ) # Subclasses whose ``gb_obj`` was allocated via ``GrB__new`` / # ``GxB__new`` (TypedUser*Op, _BoundIndexBinaryOp) override this so # ``__del__`` frees the SuiteSparse handle. Built-in typed ops point at # SuiteSparse's permanent built-in singletons and must never free. - # Specific instances can override via ``_owns_gb_obj_inst`` (set by - # the constructor); ``SelectOp._from_indexunary`` aliases an existing - # ``GrB_IndexUnaryOp`` and must clear ownership to avoid a double free. _owns_gb_obj = False def __init__(self, parent, name, type_, return_type, gb_obj, gb_name, dtype2=None): @@ -558,10 +555,12 @@ def __init__(self, parent, name, type_, return_type, gb_obj, gb_name, dtype2=Non # for this typed op; ``None`` for built-in ops and for UDT ops with # no JIT path. self._jit_c_info = None - # Per-instance ownership override; defaults to the class attribute. - # ``SelectOp._from_indexunary`` flips this to ``False`` on aliasing - # TypedUserSelectOps so only the IndexUnaryOp frees the handle. - self._owns_gb_obj_inst = type(self)._owns_gb_obj + # Set when ``gb_obj`` is borrowed from another typed op rather than + # allocated here (``SelectOp._from_indexunary``). Storing the owner + # both suppresses our free and keeps the handle alive as long as we + # point at it; disclaiming ownership without naming an owner would + # leave this op holding a dangling handle. + self._gb_obj_owner = None @property def jit_c_name(self): @@ -594,11 +593,14 @@ def __reduce__(self): def __del__(self): # Free the SuiteSparse handle we allocated. Built-in typed ops alias - # SuiteSparse's permanent built-in singletons and must never free, so - # gate on the per-instance owns flag (defaults to the class - # attribute; the alias case overrides to False). Mirrors the - # ``Matrix.__del__`` / ``Vector.__del__`` pattern. - if not getattr(self, "_owns_gb_obj_inst", False): + # SuiteSparse's permanent built-in singletons and must never free. + # Mirrors the ``Matrix.__del__`` / ``Vector.__del__`` pattern. + if not type(self)._owns_gb_obj: + return + # A borrowed handle belongs to ``_gb_obj_owner``, which we keep alive. + # ``getattr`` guards the case where ``__init__`` raised before the slot + # was set. + if getattr(self, "_gb_obj_owner", None) is not None: return gb_obj = getattr(self, "gb_obj", None) if gb_obj is None or lib is None or ffi is None: diff --git a/graphblas/core/operator/select.py b/graphblas/core/operator/select.py index 32a2509a5..7164694e4 100644 --- a/graphblas/core/operator/select.py +++ b/graphblas/core/operator/select.py @@ -97,10 +97,13 @@ def _from_indexunary(cls, iop): t.return_type, t.gb_obj, ) - # Aliases the IndexUnaryOp's allocation. The IndexUnaryOp - # owns the free; clearing here prevents a double free when - # both ends are GC'd. - op._owns_gb_obj_inst = False + # Borrow the IndexUnaryOp's allocation instead of making a + # second one. Holding ``t`` keeps that handle alive for as long + # as this SelectOp can use it: ``iop`` is a temporary in + # ``register_anonymous``, so without this the handle is freed + # the moment it is collected and every call raises + # UninitializedObject. + op._gb_obj_owner = t else: op = cls._typed_class( obj, diff --git a/graphblas/tests/test_op.py b/graphblas/tests/test_op.py index ef5227f07..e439fc149 100644 --- a/graphblas/tests/test_op.py +++ b/graphblas/tests/test_op.py @@ -1359,6 +1359,27 @@ def _this_or_that(val, idx, _, thunk): # pragma: no cover (numba) assert result.isequal(w) +@pytest.mark.skipif("not supports_udfs") +def test_select_op_outlives_source_indexunary(): + """A SelectOp keeps alive the IndexUnaryOp whose GraphBLAS handle it borrows. + + ``SelectOp._from_indexunary`` reuses the IndexUnaryOp's ``gb_obj`` rather + than allocating a second one, and ``register_anonymous`` drops that + IndexUnaryOp on the way out. Without an explicit reference the handle is + freed as soon as it is collected, and every use of the SelectOp raises + ``UninitializedObject``. + """ + import gc + + def _ne_thunk(x, i, j, thunk): # pragma: no cover (numba) + return x != thunk + + sel = SelectOp.register_anonymous(_ne_thunk) + gc.collect() + v = Vector.from_coo([0, 1, 2], [1, 5, 9]) + assert v.select(sel, 5).new().isequal(Vector.from_coo([0, 2], [1, 9], size=3)) + + @pytest.mark.skipif("not supports_udfs") @pytest.mark.slow def test_udt_tuple_return_binaryop(record_udt): From 3ba0f361d7277cac105c152d0902aaf13337f074 Mon Sep 17 00:00:00 2001 From: Erik Welch Date: Thu, 17 Sep 2026 22:41:28 -0500 Subject: [PATCH 2/2] Note the borrowed-handle exception on TypedUserSelectOp._owns_gb_obj Read on its own, `_owns_gb_obj = True` says every TypedUserSelectOp frees its handle. The ones `_from_indexunary` builds do not: they borrow the IndexUnaryOp's `GrB_IndexUnaryOp` and set `_gb_obj_owner`, which suppresses the free. Worth a comment because the mistake is silent. Freeing from both sides is a use-after-free, and a freed handle usually still reads as valid: with the ownership removed, select kept returning correct results even after 30 fresh allocations churned the allocator. No test would catch it. --- graphblas/core/operator/select.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/graphblas/core/operator/select.py b/graphblas/core/operator/select.py index 7164694e4..3cc506d84 100644 --- a/graphblas/core/operator/select.py +++ b/graphblas/core/operator/select.py @@ -25,7 +25,11 @@ def __call__(self, val, thunk=None): class TypedUserSelectOp(TypedOpBase): __slots__ = () opclass = "SelectOp" - _owns_gb_obj = True # underlying object is a GrB_IndexUnaryOp + # The underlying object is a ``GrB_IndexUnaryOp``. Instances built by + # ``_from_indexunary`` borrow that handle and set ``_gb_obj_owner``, which + # suppresses the free here. Freeing from both sides is a use-after-free that + # no test catches: the freed handle usually still reads as valid. + _owns_gb_obj = True def __init__(self, parent, name, type_, return_type, gb_obj, dtype2=None): super().__init__(parent, name, type_, return_type, gb_obj, f"{name}_{type_}", dtype2=dtype2)