Fix ownership and error handling bugs in refdb backend callbacks

pygit2_refdb_backend_write() wrapped libgit2's reference and
signature pointers directly, so the Python objects freed memory that
libgit2 frees again on return, a double free. Pass git_reference_dup()
and git_signature_dup() copies instead.

It also fed the _old OID pointer straight into git_oid_to_python(),
crashing when libgit2 passes NULL for newly created references; map
NULL to None, which RefdbBackend.write() already accepts.

pygit2_refdb_backend_lookup() treated a None result as a type error,
but None is how RefdbBackend.lookup() signals 'not found'; translate
it to GIT_ENOTFOUND.

Both lookup() and rename() leaked the Python Reference returned by
the callback, because libgit2 takes ownership of the underlying
git_reference while Reference_dealloc() would free it too. Detach
the pointer from the Python object before releasing it.

Add test_write_callback and test_write_callback_create, driving the
write callback through libgit2's git_reference_set_target() and
git_reference_create().

Follow-up to the fix for #1474

Assisted-by: Kimi Code

Author: jdavid

src/refdb_backend.c

@@ -190,18 +190,30 @@ pygit2_refdb_backend_lookup(git_reference **out,
     result = (Reference *)PyObject_CallObject(be->lookup, args);
     Py_DECREF(args);
 
-    if ((err = git_error_for_exc()) != 0)
-        goto out;
+    if ((err = git_error_for_exc()) != 0) {
+        Py_XDECREF(result);
+        return err;
+    }
+
+    // A lookup that finds nothing returns None, per RefdbBackend.lookup()
+    if ((PyObject *)result == Py_None) {
+        Py_DECREF(result);
+        return GIT_ENOTFOUND;
+    }
 
     if (!PyObject_IsInstance((PyObject *)result, (PyObject *)&ReferenceType)) {
         PyErr_SetString(PyExc_TypeError, "Expected object of type pygit2.Reference");
-        err = GIT_EUSER;
-        goto out;
+        Py_DECREF(result);
+        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;
-out:
-    return err;
+    result->reference = NULL;
+    Py_DECREF(result);
+    return 0;
 }
 
 static int
@@ -212,24 +224,46 @@ pygit2_refdb_backend_write(git_refdb_backend *_be,
 {
     struct pygit2_refdb_backend *be = (struct pygit2_refdb_backend *)_be;
 
-    PyObject *ref = wrap_reference((git_reference *)_ref, NULL);  // XXX: Drops const
+    // The Python objects take ownership of the reference and the signature,
+    // so pass them copies; _ref and _who belong to the caller (libgit2).
+    git_reference *reference;
+    int err = git_reference_dup(&reference, (git_reference *)_ref);  // XXX: Drops const
+    if (err != 0) {
+        return err;
+    }
+
+    PyObject *ref = wrap_reference(reference, NULL);
     if (ref == NULL) {
+        git_reference_free(reference);
         return GIT_EUSER;
     }
 
-    PyObject *who = build_signature(NULL, _who, "utf-8");
-    if (who == NULL) {
+    git_signature *signature;
+    err = git_signature_dup(&signature, _who);
+    if (err != 0) {
         Py_DECREF(ref);
-        return GIT_EUSER;
+        return err;
     }
 
-    PyObject *old = git_oid_to_python(_old);
-    if (old == NULL) {
+    PyObject *who = build_signature(NULL, signature, "utf-8");
+    if (who == NULL) {
         Py_DECREF(ref);
-        Py_DECREF(who);
         return GIT_EUSER;
     }
 
+    PyObject *old;
+    if (_old == NULL) {
+        old = Py_None;
+        Py_INCREF(old);
+    } else {
+        old = git_oid_to_python(_old);
+        if (old == NULL) {
+            Py_DECREF(ref);
+            Py_DECREF(who);
+            return GIT_EUSER;
+        }
+    }
+
     // Py_BuildValue takes ownership of ref, who and old (N format), even on failure, so
     // they must not be decref'd past this point.
     PyObject *args = Py_BuildValue("(NNNsNs)", ref, PyBool_FromLong(force), who, message,
@@ -239,7 +273,7 @@ pygit2_refdb_backend_write(git_refdb_backend *_be,
     }
 
     PyObject_CallObject(be->write, args);
-    int err = git_error_for_exc();
+    err = git_error_for_exc();
     Py_DECREF(args);
     return err;
 }
@@ -277,6 +311,7 @@ pygit2_refdb_backend_rename(git_reference **out, git_refdb_backend *_be,
 
     err = git_error_for_exc();
     if (err != 0) {
+        Py_XDECREF(ref);
         return err;
     }
 
@@ -287,9 +322,11 @@ pygit2_refdb_backend_rename(git_reference **out, git_refdb_backend *_be,
     }
 
     // Ownership of the underlying git_reference is transferred to libgit2,
-    // which sets its db field itself once the callback returns, so the Python
-    // object must not be decref'd; same as in pygit2_refdb_backend_lookup.
+    // 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;
 }
 

test/test_refdb_backend.py

@@ -136,6 +136,30 @@ def test_rename_callback(repo: Repository) -> None:
     assert repo.references['refs/heads/intl'].target == target
 
 
+def test_write_callback(repo: Repository) -> None:
+    # Exercise the custom backend's write callback through libgit2's
+    # git_reference_set_target; calling repo.backend.write() directly
+    # bypasses it.
+    refdb = pygit2.Refdb.new(repo)
+    refdb.set_backend(repo.backend)
+    repo.set_refdb(refdb)
+    master = repo.references['refs/heads/master']
+    i18n = repo.references['refs/heads/i18n']
+    i18n.set_target(master.target)
+    assert repo.references['refs/heads/i18n'].target == master.target
+
+
+def test_write_callback_create(repo: Repository) -> None:
+    # Exercise the custom backend's write callback through libgit2's
+    # git_reference_create, which passes old=NULL for new references.
+    refdb = pygit2.Refdb.new(repo)
+    refdb.set_backend(repo.backend)
+    repo.set_refdb(refdb)
+    master = repo.references['refs/heads/master']
+    repo.references.create('refs/heads/test-write', master.target)
+    assert repo.references['refs/heads/test-write'].target == master.target
+
+
 def test_delete(repo: Repository) -> None:
     old = repo.backend.lookup('refs/heads/i18n')
     repo.backend.delete('refs/heads/i18n', old.target, None)