Cred type finalization can cause `SIGSEGV`

[mstergianis]

Hi there,

In using the library I was able to cause a panic in the go runtime. It appears as though a Cred is being freed twice. It appears as though when you use a cred for ssh, libgit2 cleans up the cred that it creates, for example see https://github.com/libgit2/libgit2/blob/172239021f7ba04fe7327647b213799853a9eb89/src/transports/ssh.c#L684-L685. However, git.Cred structs have a finalizer attached to them by default

git2go/credentials.go

Lines 28 to 32 in a32375a

	func newCred() *Cred {
	cred := &Cred{}
	runtime.SetFinalizer(cred, (*Cred).Free)
	return cred
	}

.

package main

import (
	"errors"
	"io/ioutil"
	"log"
	"runtime"

	git "github.com/libgit2/git2go/v29"
)

func main() {
	cloneOptions := &git.CloneOptions{
		FetchOptions: &git.FetchOptions{
			RemoteCallbacks: git.RemoteCallbacks{
				CredentialsCallback:      credCallbackGenerator(false), // change the bool to true to prevent panic
				CertificateCheckCallback: certificateCheckCallback,
			},
		},
	}

	tempDir, err := ioutil.TempDir("./", "cloned-repo-")
	must(err)

	repo, err := git.Clone("git@github.com:githubtraining/hellogitworld.git", tempDir, cloneOptions)
	must(err)

	runtime.GC()
	log.Println(repo)
}

func must(err error) {
	if err != nil {
		panic(err)
	}
}

func credCallbackGenerator(shouldUnsetFinalize bool) func(url string, username string, allowedTypes git.CredType) (*git.Cred, error) {
	return func(url string, username string, allowedTypes git.CredType) (cred *git.Cred, err error) {
		if (allowedTypes & git.CredTypeSshKey) == git.CredTypeSshKey {
			cred, err = git.NewCredSshKeyFromAgent(username)
		} else {
			cred, err = nil, errors.New("only unsupported allowed types")
		}

		if shouldUnsetFinalize && cred != nil {
			runtime.SetFinalizer(cred, nil)
		}

		return cred, err
	}
}

func certificateCheckCallback(cert *git.Certificate, valid bool, hostname string) git.ErrorCode {
	return 0
}

On my machine, this exhibits the behaviour I am describing.

[lhchavez]

huh, I think i have not seen this before since the fork i have been using in production uses a Golang implementation of SSH lhchavez@aafbf47

but we still need to be able to distinguish between instances that come from within libgit2 and instances that are allocated from Go and set the finalizer accordingly.

thanks for reporting this!

[vladimir-buzuev]

same for plaintext creds. @lhchavez, what do you mean by 'distinguish between instances that come from within libgit2 and instances that are allocated from Go' what are the cases when the finalizer is actually needed?

git2go/remote.go

Line 249 in 3a21026

cred, err := callbacks.CredentialsCallback(url, username_from_url, (CredType)(allowed_types))

the ownership of cred allocated above will be transferred to libgit on this line

git2go/remote.go

Line 257 in 3a21026

*_cred = cred.ptr

and libgit will eventually free it. But the finalizer for cred will do the same. And actually, the finalizer can do it even earlier as from go perspective, cred is already garbage after L257

[vladimir-buzuev]

@lhchavez and a follow up question - what memory leaks you were trying to fix in #478? Thanks.

[lhchavez]

what memory leaks you were trying to fix in #478? Thanks.

if git2go uses a manged transport (like in lhchavez@aafbf47, where libgit2's ssh implementation is not involved, which by the way needs to be upstreamed at some point), there's nobody that frees the Cred object, and you get a leak.