Double free error when cloning repository

[c4milo]

I'm still getting familiar with git2go and libgit2, but I did some digging and it seems git_clone() already does deallocate git_repository: https://github.com/libgit2/libgit2/blob/development/src/clone.c#L425. So, having this finalizer seems unnecessary: https://github.com/libgit2/git2go/blob/make-static/clone.go#L53

[carlosmn]

It deallocates it on error, because it doesn't return it. If there's an error, it's something else.

[c4milo]

I see, What is interesting, though, is that my double free error went away when I commented the finalizer. I will keep digging more anyway.

[c4milo]

It also goes away if I change this:

func (v *Repository) Free() {
    runtime.SetFinalizer(v, nil)
    C.git_repository_free(v.ptr)
}

to

func (v *Repository) Free() {
    runtime.SetFinalizer(v, nil)
    if v.ptr != nil {
        C.git_repository_free(v.ptr)
    }
}

I agree the real culprit is still unknown. I'm going to check next if the finalizer is being called more than once by Go.

[c4milo]

After some more testing, it is now looking more like a race condition somewhere inside git2go. This is what I'm doing, I'm starting multiple goroutines at once, each one cloning a different repository. I wait for all of them to finish and return back the result. It sometimes finishes fine and sometimes it is not.

[c4milo]

Or maybe I'm misusing git2go? This is what I'm doing:

log.Info("Downloading buildpacks...")
    for _, bp := range bpacks {
        wg.Add(1)
        go func(url string) {
            log.Info("Cloning " + url + " into " + bpacksPath)

            defer wg.Done()
            parts := strings.Split(url, "#")

            branch := "master"
            if len(parts) == 2 {
                branch = parts[1]
            }

            dest := filepath.Join(bpacksPath, path.Base(url))

            options := &git.CloneOptions{
                CheckoutBranch: branch,
            }
            _, err := git.Clone(url, dest, options)
            if err != nil {
                log.Error(err.Error())
            }
        }(bp)
    }

    wg.Wait()

[carlosmn]

That definitely looks like it should work.

The check for v.ptr != nil suggests that we're returning a repository object when there was an error creating it.

[c4milo]

I also tested checking for v.ptr != nil in Repository.Free() but I still get the double free errors coming from inside git_clone()

[carlosmn]

That trace you posted says it goes through clone.go:48, but in master that's the defer call which wouldn't be causing this? Are you using something other than master?

[c4milo]

Sorry, I should have mentioned that before, I'm using your make-static branch.

[carlosmn]

Ok, that has the same clone code. I can't reproduce locally with code that should behave the same, but with more fixed data.

I had added a line to the imports, so of course line 48 is the actual git_clone() call which suggests it's an issue in libgit2 which is causing the double-free and the finalizer shouldn't have anything to do with it...

package main

import (
    "io/ioutil"
    "sync"
    "log"
    "github.com/libgit2/git2go"
    "runtime"
)

func main() {
    list := []string {
        "git://localhost/git/git-httpd",
        "git://localhost/git/git-httpd",
        "git://localhost/git/git-httpd",
        "git://localhost/git/git-httpd",
        "git://localhost/git/git-httpd",
        "git://localhost/git/git-httpd",
        "git://localhost/git/git-httpd",
        "git://localhost/git/git-httpd",
        "git://localhost/git/git-httpd",
        "git://localhost/git/git-httpd",
        "git://localhost/git/git-httpd",
        "git://localhost/git/git-httpd",
    }

    var wg sync.WaitGroup

    for _, repo := range list {
        wg.Add(1)
        go func(url string) {
            defer wg.Done()

            path, err := ioutil.TempDir("", "clone-test-clone")
            if err != nil {
                log.Fatal(":(", err)
            }

            opts := &git.CloneOptions{
                CheckoutBranch: "master",
            }
            _, err = git.Clone(url, path, opts)
            if err != nil {
                log.Println(" -- ", err)
            }

        }(repo)
    }

    wg.Wait()
    runtime.GC()
}

[c4milo]

These are the URLs I'm using:

var buildpacks = []string{
    //Heroku official buildpacks
    "https://github.com/heroku/heroku-buildpack-play.git",
    "https://github.com/heroku/heroku-buildpack-scala.git",
    "https://github.com/heroku/heroku-buildpack-java.git",
    "https://github.com/heroku/heroku-buildpack-ruby.git",
    "https://github.com/heroku/heroku-buildpack-nodejs.git",
    "https://github.com/heroku/heroku-buildpack-python.git",
    "https://github.com/heroku/heroku-buildpack-clojure.git",
    "https://github.com/heroku/heroku-buildpack-gradle.git",
    "https://github.com/heroku/heroku-buildpack-grails.git",
    "https://github.com/heroku/heroku-buildpack-php.git",

    //Community buildpacks
    "https://github.com/ddollar/heroku-buildpack-multi.git",
    "https://github.com/kr/heroku-buildpack-go.git",
    "https://github.com/jruby/heroku-buildpack-jruby.git",
    "https://github.com/archaelus/heroku-buildpack-erlang.git",
    "https://github.com/atris/heroku-buildpack-C.git",
    "https://github.com/miyagawa/heroku-buildpack-perl.git",
    "https://github.com/igrigorik/heroku-buildpack-dart.git",
    "https://github.com/ericfode/heroku-buildpack-rust.git",
    "https://github.com/oortcloud/heroku-buildpack-meteorite.git",
}

[c4milo]

As you mentioned, at this point I think it is clear the finalizer is not the culprit.

[carlosmn]

It looks like we don't really handle using SSL from multiple threads too well, so that explains why using git:// didn't fail.

[carlosmn]

This should be fixed with libgit2/libgit2#2421, could you try it out?

[c4milo]

I checked out your branch with the fix into the vendor folder, and re-installed libgit2. But, I still get the same errors: https://gist.github.com/c4milo/cd4d67ae5985a83ebc5d

[carlosmn]

Make sure you remove the old version from $GOPATH/pkg (e.g. rm $GOPATH/pkg/github.com/libgit2/git2go.a ) because the build tool won't notice when only libgit2 has changed and will no-op if no go files have changed.

[carlosmn]

If that doesn't fix it, could you attach gdb and show a backtrace?

[c4milo]

Removed git2go.a and I still get the errors. Regarding attaching GDB, I'm using OSX Mavericks and currently running into this issue: http://stackoverflow.com/questions/19745549/golang-cannot-get-gdb-working-for-go-programs-using-c-libraries

[carlosmn]

lldb should work just as well. I'll go test with my OSX machine in a bit.

[c4milo]

Oh nice. Yeah, I was able to use lldb just fine. Here is the backtrace https://gist.github.com/c4milo/357416303ae4ad9087e1

[carlosmn]

Turns out our handling of openssl with threading is completely broken, I'm working on fixing it properly.

[c4milo]

I'm glad you found the problem. 👯 Looking forward to your patch.

[carlosmn]

Alright, now it's fixed for real, can you test with the current version of cmn/ssl-init-once? It now passes for me on OSX too. I wonder if it's there's a difference in the scheduler, since all these calls down to C land make it use real threads for the goroutines.

[c4milo]

It seems to happen less often but I'm still getting errors.

[c4milo]

This is a new stack trace:

[carlosmn]

OpenSSL seems to be very picky about where it gets initialised and isn't explicit about it. In the branch I've moved to init it during the general library init, which is not great, but seems to be required. Could you take another look?

I used to be able to reproduce with the last few commits by setting a high GOMAXPROCS which seemed to make go happier to start up threads. With the 'wip' commit, that doesn't do it anymore.

[c4milo]

@carlosmn I'm not sure I understand what you are asking me to do.

[c4milo]

ah sorry, I get it now. I'm going to test again and let you know my findings.

[c4milo]

So far, I haven't been able to reproduce it again. I think that did it @carlosmn. It seems to be a bit slower than before but it could also be due to other factors. Nice work.

[carlosmn]

There is a printf on each lock and unlock which is going to slow it down quite a bit, but once the real commit is in it should barely affect the speed.

[c4milo]

Makes sense. Thank you for explaining :)

[carlosmn]

libgit2 was fixed a while ago, and we're using a fixed version in vendor/libgit2. Closing as fixed.