Cred type finalization can cause SIGSEGV #553

New Issue

2020-03-23T14:14:32-05:00

mstergianis commented

2020-03-23 14:14:32 -05:00

(Migrated from github.com)

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 172239021f/src/transports/ssh.c (L684-L685). However, git.Cred structs have a finalizer attached to them by default a32375a860/credentials.go (L28-L32).

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.

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 https://github.com/libgit2/git2go/blob/a32375a86063768507a01b32cc3d868f4c2ffa9c/credentials.go#L28-L32. ```golang 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 commented

2020-03-23 14:31:32 -05:00

(Migrated from github.com)

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

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!

huh, I think i have not seen this before since the fork i have been using in production uses a Golang implementation of SSH https://github.com/lhchavez/git2go/commit/aafbf47bbf238bb35759d1ac731cce44edc3c985 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 commented

2020-03-26 13:49:20 -05:00

(Migrated from github.com)

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?

3a2102638d/remote.go (L249)

the ownership of cred allocated above will be transferred to libgit on this line 3a2102638d/remote.go (L257) 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

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? https://github.com/libgit2/git2go/blob/3a2102638d64cd76e50f51577e8bb2b8f9c7035f/remote.go#L249 the ownership of `cred` allocated above will be transferred to libgit on this line https://github.com/libgit2/git2go/blob/3a2102638d64cd76e50f51577e8bb2b8f9c7035f/remote.go#L257 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 commented

2020-03-26 13:51:20 -05:00

(Migrated from github.com)

@lhchavez and a follow up question - what memory leaks you were trying to fix in https://github.com/libgit2/git2go/pull/478? Thanks.

lhchavez commented

2020-03-26 18:54:54 -05:00

(Migrated from github.com)

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

if git2go uses a manged transport (like in aafbf47bbf, 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.

> what memory leaks you were trying to fix in #478? Thanks. if git2go uses a manged transport (like in https://github.com/lhchavez/git2go/commit/aafbf47bbf238bb35759d1ac731cce44edc3c985, 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.

Sign in to join this conversation.