Fix ownership and lifetime bugs in refdb backend callbacks

The refdb backend callbacks that hand a reference to libgit2 all had
the same bug class: they leaked the Python Reference returned or
yielded by user code, and borrowed the inner git_reference pointer
from it, so releasing the object without leaking would leave libgit2
with a dangling pointer.

git_reference_dup() cannot be used to make a safe copy, because it
requires the db field to be set, which libgit2 only does after the
callback returns. Instead, detach the pointer when the callback
returned a fresh object (refcount 1), and copy it with
git_reference__alloc()/git_reference__alloc_symbolic() when it
returned a shared object, e.g. one the backend caches and returns on
every call; the copy loses the cached peel, which is only a hint.

Fixed callbacks:

- lookup() and rename() now handle a cached Reference instead of
  invalidating it on first use (the second lookup handed libgit2 a
  NULL pointer).

- The iterator next() callback leaked every yielded Reference and
  borrowed its pointer, a double free waiting for the object to be
  collected; the glob filter also leaked every non-matching yield.
  The next_name() callback leaked as well, and now keeps the last
  yielded Reference alive so the borrowed name stays valid until the
  next call.

- The iterator constructor crashed on a NULL glob, which is exactly
  what git_reference_iterator_new() passes; it had never been
  exercised through libgit2 before.

Also fix Reference(name, oid, None) passing an uninitialized peel OID
to git_reference__alloc(), so every backend-constructed direct
reference carried a garbage peel into libgit2.

Add test_lookup_cached_callback, test_iterator_callback and
test_iterator_callback_no_leak, driving cached Reference objects
through libgit2; each crashes or fails against the old code.

Fixes #1475

Assisted-by: Kimi Code

Author: jdavid

CHANGELOG.md

@@ -19,9 +19,10 @@
   bound to the `DIFF3` constant
   [#1483](https://github.com/libgit2/pygit2/pull/1483)
 
-- Fix memory issues
+- Fix memory issues in refdb backend
   [#1471](https://github.com/libgit2/pygit2/issues/1471)
   [#1474](https://github.com/libgit2/pygit2/issues/1474)
+  [#1475](https://github.com/libgit2/pygit2/issues/1475)
 
 - Update wheels to libgit2 1.9.6 and OpenSSL 3.5.7
 

src/refdb_backend.c

@@ -36,6 +36,7 @@
 #include "wildmatch.h"
 #include <git2/refdb.h>
 #include <git2/sys/refdb_backend.h>
+#include <git2/sys/refs.h>
 #include <git2/sys/errors.h>
 
 extern PyTypeObject ReferenceType;
@@ -66,21 +67,82 @@ struct pygit2_refdb_backend
 struct pygit2_refdb_iterator {
     struct git_reference_iterator base;
     PyObject *iterator;
+    PyObject *current; /* keeps the last Reference yielded by next_name alive */
     char *glob;
 };
 
+// Copy a reference that does not belong to a refdb yet. git_reference_dup()
+// cannot be used here because it requires the db field to be set, which
+// libgit2 only does after the backend callback returns. The copy does not
+// preserve the cached peel of direct references, which is only a hint.
+static git_reference *
+copy_reference(const git_reference *ref)
+{
+    if (git_reference_type(ref) == GIT_REF_SYMBOLIC) {
+        return git_reference__alloc_symbolic(git_reference_name(ref),
+                                             git_reference_symbolic_target(ref));
+    }
+    return git_reference__alloc(git_reference_name(ref),
+                                git_reference_target(ref), NULL);
+}
+
+// Transfer the reference returned by a backend callback to libgit2, which
+// takes ownership and sets its db field itself once the callback returns.
+// If the callback returned a fresh object (refcount 1), detach the pointer
+// from the Python object before releasing it; if it returned a shared
+// object (e.g. one the backend caches and returns on every call), leave it
+// intact and give libgit2 a copy instead.
+// Consumes the reference to result; returns 0 or a libgit2 error code.
+static int
+transfer_reference(git_reference **out, Reference *result)
+{
+    if (result->reference == NULL) {
+        PyErr_SetString(PyExc_ValueError, "Reference object is no longer valid");
+        Py_DECREF(result);
+        return GIT_EUSER;
+    }
+
+    if (Py_REFCNT((PyObject *)result) == 1) {
+        *out = result->reference;
+        result->reference = NULL;
+        Py_DECREF(result);
+        return 0;
+    }
+
+    *out = copy_reference(result->reference);
+    Py_DECREF(result);
+    if (*out == NULL) {
+        git_error_set(GIT_ERROR_NOMEMORY, "out of memory");
+        return GIT_ERROR;
+    }
+    return 0;
+}
+
+// Returns the next valid Reference from the Python iterator, or NULL when
+// the iterator is exhausted or an error occurred; check PyErr_Occurred()
+// to tell the two cases apart.
 static Reference *
 iterator_get_next(struct pygit2_refdb_iterator *iter)
 {
     Reference *ref;
     while ((ref = (Reference *)PyIter_Next(iter->iterator)) != NULL) {
-        if (!iter->glob) {
-            return ref;
+        if (!PyObject_IsInstance((PyObject *)ref, (PyObject *)&ReferenceType)) {
+            PyErr_SetString(PyExc_TypeError,
+                            "RefdbBackend iterator must yield References");
+            Py_DECREF(ref);
+            return NULL;
+        }
+        if (ref->reference == NULL) {
+            PyErr_SetString(PyExc_ValueError,
+                            "Reference object is no longer valid");
+            Py_DECREF(ref);
+            return NULL;
         }
-        const char *name = git_reference_name(ref->reference);
-        if (wildmatch(iter->glob, name, 0) != WM_NOMATCH) {
+        if (!iter->glob ||
+            wildmatch(iter->glob, git_reference_name(ref->reference), 0) != WM_NOMATCH) {
             return ref;
         }
+        Py_DECREF(ref);
     }
     return NULL;
 }
@@ -91,16 +153,12 @@ pygit2_refdb_iterator_next(git_reference **out, git_reference_iterator *_iter)
     struct pygit2_refdb_iterator *iter = (struct pygit2_refdb_iterator *)_iter;
     Reference *ref = iterator_get_next(iter);
     if (ref == NULL) {
+        if (PyErr_Occurred())
+            return GIT_EUSER;
         *out = NULL;
         return GIT_ITEROVER;
     }
-    if (!PyObject_IsInstance((PyObject *)ref, (PyObject *)&ReferenceType)) {
-        PyErr_SetString(PyExc_TypeError,
-                        "RefdbBackend iterator must yield References");
-        return GIT_EUSER;
-    }
-    *out = ref->reference;
-    return 0;
+    return transfer_reference(out, ref);
 }
 
 static int
@@ -109,14 +167,15 @@ pygit2_refdb_iterator_next_name(const char **ref_name, git_reference_iterator *_
     struct pygit2_refdb_iterator *iter = (struct pygit2_refdb_iterator *)_iter;
     Reference *ref = iterator_get_next(iter);
     if (ref == NULL) {
+        if (PyErr_Occurred())
+            return GIT_EUSER;
         *ref_name = NULL;
         return GIT_ITEROVER;
     }
-    if (!PyObject_IsInstance((PyObject *)ref, (PyObject *)&ReferenceType)) {
-        PyErr_SetString(PyExc_TypeError,
-                        "RefdbBackend iterator must yield References");
-        return GIT_EUSER;
-    }
+    // The name is borrowed from the Reference; keep the object alive until
+    // the next call or until the iterator is freed.
+    Py_XDECREF(iter->current);
+    iter->current = (PyObject *)ref;
     *ref_name = git_reference_name(ref->reference);
     return 0;
 }
@@ -125,6 +184,7 @@ static void
 pygit2_refdb_iterator_free(git_reference_iterator *_iter)
 {
     struct pygit2_refdb_iterator *iter = (struct pygit2_refdb_iterator *)_iter;
+    Py_CLEAR(iter->current);
     Py_DECREF(iter->iterator);
     free(iter->glob);
 }
@@ -144,12 +204,17 @@ pygit2_refdb_backend_iterator(git_reference_iterator **iter,
         git_error_set(GIT_ERROR_NOMEMORY, "out of memory");
         return GIT_ERROR;
     }
+    if (glob && (pyiter->glob = strdup(glob)) == NULL) {
+        Py_DECREF(iterator);
+        free(pyiter);
+        git_error_set(GIT_ERROR_NOMEMORY, "out of memory");
+        return GIT_ERROR;
+    }
     *iter = (git_reference_iterator *)pyiter;
     pyiter->iterator = iterator;
     pyiter->base.next = pygit2_refdb_iterator_next;
     pyiter->base.next_name = pygit2_refdb_iterator_next_name;
     pyiter->base.free = pygit2_refdb_iterator_free;
-    pyiter->glob = strdup(glob);
     return 0;
 }
 
@@ -207,13 +272,7 @@ pygit2_refdb_backend_lookup(git_reference **out,
         return GIT_EUSER;
     }
 
-    // Ownership of the underlying git_reference is transferred to libgit2,
-    // which sets its db field itself once the callback returns; detach it
-    // from the Python object before releasing the object.
-    *out = result->reference;
-    result->reference = NULL;
-    Py_DECREF(result);
-    return 0;
+    return transfer_reference(out, result);
 }
 
 static int
@@ -321,13 +380,7 @@ pygit2_refdb_backend_rename(git_reference **out, git_refdb_backend *_be,
         return GIT_EUSER;
     }
 
-    // Ownership of the underlying git_reference is transferred to libgit2,
-    // which sets its db field itself once the callback returns; detach it
-    // from the Python object before releasing the object.
-    *out = ref->reference;
-    ref->reference = NULL;
-    Py_DECREF(ref);
-    return 0;
+    return transfer_reference(out, ref);
 }
 
 static int

src/reference.c

@@ -170,7 +170,8 @@ Reference_init(Reference *self, PyObject *args, PyObject *kwds)
         py_oid_to_git_oid(py_peel, &peel);
     }
 
-    self->reference = git_reference__alloc(name, &oid, &peel);
+    self->reference = git_reference__alloc(name, &oid,
+                                           py_peel == Py_None ? NULL : &peel);
     return 0;
 }
 

test/test_refdb_backend.py

@@ -25,14 +25,17 @@
 
 """Tests for Refdb objects."""
 
-from collections.abc import Generator
+import sys
+from collections.abc import Generator, Iterator
 from pathlib import Path
 
 import pytest
 
 import pygit2
 from pygit2 import Commit, Oid, Reference, Repository, Signature
 
+from . import utils
+
 
 # Note: the refdb abstraction from libgit2 is meant to provide information
 # which libgit2 transforms into something more useful, and in general YMMV by
@@ -96,6 +99,41 @@ def repo(testrepo: Repository) -> Generator[Repository, None, None]:
     yield testrepo
 
 
+class CachedRefdbBackend(ProxyRefdbBackend):
+    """A backend that caches and reuses the Reference objects it returns."""
+
+    def __init__(self, source: pygit2.RefdbBackend) -> None:
+        super().__init__(source)
+        self.cache: dict[str, Reference] = {}
+
+    def lookup(self, ref: str) -> Reference:
+        if ref not in self.cache:
+            self.cache[ref] = self.source.lookup(ref)
+        return self.cache[ref]
+
+
+class IterRefdbBackend(ProxyRefdbBackend):
+    """A backend whose iterator yields cached Reference objects."""
+
+    def __init__(self, source: pygit2.RefdbBackend) -> None:
+        super().__init__(source)
+        self.cache: list[Reference] | None = None
+        self.refs: Iterator[Reference] = iter([])
+
+    def __iter__(self) -> 'IterRefdbBackend':
+        if self.cache is None:
+            self.cache = [
+                self.source.lookup('refs/heads/master'),
+                self.source.lookup('refs/heads/i18n'),
+                Reference('refs/heads/symbolic', 'refs/heads/master'),
+            ]
+        self.refs = iter(self.cache)
+        return self
+
+    def __next__(self) -> Reference:
+        return next(self.refs)
+
+
 def test_exists(repo: Repository) -> None:
     assert not repo.backend.exists('refs/heads/does-not-exist')
     assert repo.backend.exists('refs/heads/master')
@@ -106,6 +144,55 @@ def test_lookup(repo: Repository) -> None:
     assert repo.backend.lookup('refs/heads/master').name == 'refs/heads/master'
 
 
+def test_lookup_cached_callback(testrepo: Repository) -> None:
+    # Regression test: a backend may cache and return the same Reference
+    # object on every lookup; the callback must not invalidate it, and
+    # repeated lookups through libgit2 must keep working.
+    backend = CachedRefdbBackend(pygit2.RefdbFsBackend(testrepo))
+    refdb = pygit2.Refdb.new(testrepo)
+    refdb.set_backend(backend)
+    testrepo.set_refdb(refdb)
+
+    target = testrepo.references['refs/heads/master'].target
+    assert testrepo.references['refs/heads/master'].target == target
+    assert backend.cache['refs/heads/master'].name == 'refs/heads/master'
+
+
+def test_iterator_callback(testrepo: Repository) -> None:
+    # Exercise the custom backend's iterator callback through libgit2's
+    # git_reference_iterator; the Python attribute alone doesn't install it.
+    backend = IterRefdbBackend(pygit2.RefdbFsBackend(testrepo))
+    refdb = pygit2.Refdb.new(testrepo)
+    refdb.set_backend(backend)
+    testrepo.set_refdb(refdb)
+
+    names = sorted(ref.name for ref in testrepo.references.iterator())
+    assert names == ['refs/heads/i18n', 'refs/heads/master', 'refs/heads/symbolic']
+
+    # The backend's cached objects must still be usable after iteration.
+    assert backend.cache is not None
+    assert [ref.name for ref in backend.cache] == [
+        'refs/heads/master',
+        'refs/heads/i18n',
+        'refs/heads/symbolic',
+    ]
+
+
+@utils.requires_refcount
+def test_iterator_callback_no_leak(testrepo: Repository) -> None:
+    # Iterating must not leak the Reference objects the backend yields.
+    backend = IterRefdbBackend(pygit2.RefdbFsBackend(testrepo))
+    refdb = pygit2.Refdb.new(testrepo)
+    refdb.set_backend(backend)
+    testrepo.set_refdb(refdb)
+
+    list(testrepo.references.iterator())
+    assert backend.cache is not None
+    refcount = sys.getrefcount(backend.cache[0])
+    list(testrepo.references.iterator())
+    assert sys.getrefcount(backend.cache[0]) == refcount
+
+
 def test_write(repo: Repository) -> None:
     master = repo.backend.lookup('refs/heads/master')
     commit = repo[master.target]