Ensure that no pointer handles leak during the test (#712) (#716)

This change makes sure that pointer handles are correctly cleaned up
during tests.

(cherry picked from commit e28cce8)

Co-authored-by: lhchavez <[email protected]>

Author: github-actions[bot]

clone.go

@@ -55,38 +55,36 @@ func Clone(url string, path string, options *CloneOptions) (*Repository, error)
 
 //export remoteCreateCallback
 func remoteCreateCallback(
-	cremote unsafe.Pointer,
-	crepo unsafe.Pointer,
+	out **C.git_remote,
+	crepo *C.git_repository,
 	cname, curl *C.char,
-	payload unsafe.Pointer,
+	handle unsafe.Pointer,
 ) C.int {
 	name := C.GoString(cname)
 	url := C.GoString(curl)
-	repo := newRepositoryFromC((*C.git_repository)(crepo))
-	// We don't own this repository, so make sure we don't try to free it
-	runtime.SetFinalizer(repo, nil)
+	repo := newRepositoryFromC(crepo)
+	repo.weak = true
+	defer repo.Free()
 
-	data, ok := pointerHandles.Get(payload).(*cloneCallbackData)
+	data, ok := pointerHandles.Get(handle).(*cloneCallbackData)
 	if !ok {
 		panic("invalid remote create callback")
 	}
 
 	remote, ret := data.options.RemoteCreateCallback(repo, name, url)
-	// clear finalizer as the calling C function will
-	// free the remote itself
-	runtime.SetFinalizer(remote, nil)
-
 	if ret < 0 {
 		*data.errorTarget = errors.New(ErrorCode(ret).String())
 		return C.int(ErrorCodeUser)
 	}
-
 	if remote == nil {
 		panic("no remote created by callback")
 	}
 
-	cptr := (**C.git_remote)(cremote)
-	*cptr = remote.ptr
+	*out = remote.ptr
+
+	// clear finalizer as the calling C function will
+	// free the remote itself
+	runtime.SetFinalizer(remote, nil)
 
 	return C.int(ErrorCodeOK)
 }

clone_test.go

@@ -74,4 +74,5 @@ func TestCloneWithCallback(t *testing.T) {
 	if err != nil || remote == nil {
 		t.Fatal("Remote was not created properly")
 	}
+	defer remote.Free()
 }

git_test.go

@@ -1,13 +1,33 @@
 package git
 
 import (
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path"
+	"reflect"
 	"testing"
 	"time"
 )
 
+func TestMain(m *testing.M) {
+	ret := m.Run()
+
+	// Ensure that we are not leaking any pointer handles.
+	pointerHandles.Lock()
+	if len(pointerHandles.handles) > 0 {
+		for h, ptr := range pointerHandles.handles {
+			fmt.Printf("%016p: %v %+v\n", h, reflect.TypeOf(ptr), ptr)
+		}
+		panic("pointer handle list not empty")
+	}
+	pointerHandles.Unlock()
+
+	Shutdown()
+
+	os.Exit(ret)
+}
+
 func cleanupTestRepo(t *testing.T, r *Repository) {
 	var err error
 	if r.IsBare() {

object.go

@@ -77,14 +77,14 @@ func (o *Object) Type() ObjectType {
 	return ret
 }
 
-// Owner returns a weak reference to the repository which owns this
-// object. This won't keep the underlying repository alive.
+// Owner returns a weak reference to the repository which owns this object.
+// This won't keep the underlying repository alive, but it should still be
+// Freed.
 func (o *Object) Owner() *Repository {
-	ret := &Repository{
-		ptr: C.git_object_owner(o.ptr),
-	}
+	repo := newRepositoryFromC(C.git_object_owner(o.ptr))
 	runtime.KeepAlive(o)
-	return ret
+	repo.weak = true
+	return repo
 }
 
 func dupObject(obj *Object, kind ObjectType) (*C.git_object, error) {

push_test.go

@@ -14,15 +14,18 @@ func TestRemotePush(t *testing.T) {
 
 	remote, err := localRepo.Remotes.Create("test_push", repo.Path())
 	checkFatal(t, err)
+	defer remote.Free()
 
 	seedTestRepo(t, localRepo)
 
 	err = remote.Push([]string{"refs/heads/master"}, nil)
 	checkFatal(t, err)
 
-	_, err = localRepo.References.Lookup("refs/remotes/test_push/master")
+	ref, err := localRepo.References.Lookup("refs/remotes/test_push/master")
 	checkFatal(t, err)
+	defer ref.Free()
 
-	_, err = repo.References.Lookup("refs/heads/master")
+	ref, err = repo.References.Lookup("refs/heads/master")
 	checkFatal(t, err)
+	defer ref.Free()
 }

reference.go

@@ -293,12 +293,14 @@ func (v *Reference) Peel(t ObjectType) (*Object, error) {
 	return allocObject(cobj, v.repo), nil
 }
 
-// Owner returns a weak reference to the repository which owns this
-// reference.
+// Owner returns a weak reference to the repository which owns this reference.
+// This won't keep the underlying repository alive, but it should still be
+// Freed.
 func (v *Reference) Owner() *Repository {
-	return &Repository{
-		ptr: C.git_reference_owner(v.ptr),
-	}
+	repo := newRepositoryFromC(C.git_reference_owner(v.ptr))
+	runtime.KeepAlive(v)
+	repo.weak = true
+	return repo
 }
 
 // Cmp compares v to ref2. It returns 0 on equality, otherwise a

remote.go

@@ -432,10 +432,12 @@ func RemoteIsValidName(name string) bool {
 	return C.git_remote_is_valid_name(cname) == 1
 }
 
+// Free releases the resources of the Remote.
 func (r *Remote) Free() {
 	runtime.SetFinalizer(r, nil)
 	C.git_remote_free(r.ptr)
 	r.ptr = nil
+	r.repo = nil
 }
 
 type RemoteCollection struct {

remote_test.go

@@ -23,9 +23,9 @@ func TestListRemotes(t *testing.T) {
 	repo := createTestRepo(t)
 	defer cleanupTestRepo(t, repo)
 
-	_, err := repo.Remotes.Create("test", "git://foo/bar")
-
+	remote, err := repo.Remotes.Create("test", "git://foo/bar")
 	checkFatal(t, err)
+	defer remote.Free()
 
 	expected := []string{
 		"test",
@@ -53,6 +53,7 @@ func TestCertificateCheck(t *testing.T) {
 
 	remote, err := repo.Remotes.Create("origin", "https://github.com/libgit2/TestGitRepository")
 	checkFatal(t, err)
+	defer remote.Free()
 
 	options := FetchOptions{
 		RemoteCallbacks: RemoteCallbacks{
@@ -73,6 +74,7 @@ func TestRemoteConnect(t *testing.T) {
 
 	remote, err := repo.Remotes.Create("origin", "https://github.com/libgit2/TestGitRepository")
 	checkFatal(t, err)
+	defer remote.Free()
 
 	err = remote.ConnectFetch(nil, nil, nil)
 	checkFatal(t, err)
@@ -85,6 +87,7 @@ func TestRemoteLs(t *testing.T) {
 
 	remote, err := repo.Remotes.Create("origin", "https://github.com/libgit2/TestGitRepository")
 	checkFatal(t, err)
+	defer remote.Free()
 
 	err = remote.ConnectFetch(nil, nil, nil)
 	checkFatal(t, err)
@@ -104,6 +107,7 @@ func TestRemoteLsFiltering(t *testing.T) {
 
 	remote, err := repo.Remotes.Create("origin", "https://github.com/libgit2/TestGitRepository")
 	checkFatal(t, err)
+	defer remote.Free()
 
 	err = remote.ConnectFetch(nil, nil, nil)
 	checkFatal(t, err)
@@ -136,11 +140,13 @@ func TestRemotePruneRefs(t *testing.T) {
 	err = config.SetBool("remote.origin.prune", true)
 	checkFatal(t, err)
 
-	_, err = repo.Remotes.Create("origin", "https://github.com/libgit2/TestGitRepository")
+	remote, err := repo.Remotes.Create("origin", "https://github.com/libgit2/TestGitRepository")
 	checkFatal(t, err)
+	defer remote.Free()
 
-	remote, err := repo.Remotes.Lookup("origin")
+	remote, err = repo.Remotes.Lookup("origin")
 	checkFatal(t, err)
+	defer remote.Free()
 
 	if !remote.PruneRefs() {
 		t.Fatal("Expected remote to be configured to prune references")
@@ -159,6 +165,7 @@ func TestRemotePrune(t *testing.T) {
 
 	remoteRef, err := remoteRepo.CreateBranch("test-prune", commit, true)
 	checkFatal(t, err)
+	defer remoteRef.Free()
 
 	repo := createTestRepo(t)
 	defer cleanupTestRepo(t, repo)
@@ -170,12 +177,14 @@ func TestRemotePrune(t *testing.T) {
 	remoteUrl := fmt.Sprintf("file://%s", remoteRepo.Workdir())
 	remote, err := repo.Remotes.Create("origin", remoteUrl)
 	checkFatal(t, err)
+	defer remote.Free()
 
 	err = remote.Fetch([]string{"test-prune"}, nil, "")
 	checkFatal(t, err)
 
-	_, err = repo.References.Create("refs/remotes/origin/test-prune", head, true, "remote reference")
+	ref, err := repo.References.Create("refs/remotes/origin/test-prune", head, true, "remote reference")
 	checkFatal(t, err)
+	defer ref.Free()
 
 	err = remoteRef.Delete()
 	checkFatal(t, err)
@@ -185,15 +194,17 @@ func TestRemotePrune(t *testing.T) {
 
 	rr, err := repo.Remotes.Lookup("origin")
 	checkFatal(t, err)
+	defer rr.Free()
 
 	err = rr.ConnectFetch(nil, nil, nil)
 	checkFatal(t, err)
 
 	err = rr.Prune(nil)
 	checkFatal(t, err)
 
-	_, err = repo.References.Lookup("refs/remotes/origin/test-prune")
+	ref, err = repo.References.Lookup("refs/remotes/origin/test-prune")
 	if err == nil {
+		ref.Free()
 		t.Fatal("Expected error getting a pruned reference")
 	}
 }

repository.go

@@ -35,6 +35,10 @@ type Repository struct {
 	// Stashes represents the collection of stashes and can be used to
 	// save, apply and iterate over stash states in this repository.
 	Stashes StashCollection
+
+	// weak indicates that a repository is a weak pointer and should not be
+	// freed.
+	weak bool
 }
 
 func newRepositoryFromC(ptr *C.git_repository) *Repository {
@@ -136,8 +140,13 @@ func (v *Repository) SetRefdb(refdb *Refdb) {
 }
 
 func (v *Repository) Free() {
+	ptr := v.ptr
+	v.ptr = nil
 	runtime.SetFinalizer(v, nil)
-	C.git_repository_free(v.ptr)
+	if v.weak {
+		return
+	}
+	C.git_repository_free(ptr)
 }
 
 func (v *Repository) Config() (*Config, error) {