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 New Product Announcement Letter Sample Assisted-by: Kimi Code

Author: WordPress Blog Post Ideas

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)