garbage collector found invalid heap pointer: Go 1.4 about git2go HOT 25 CLOSED

The newer releases of Go (at least that's where I've noticed it) can make the GC free memory from a variable in the middle of a function if the variable isn't in use in the Go code anymore. It's possible that there are some missing defensive copies.

from git2go.

It looks like there's a timing aspect to this. Removing the prints lets the program run without crashing.

from git2go.

I'm getting same panic, EVEN WITH empty callback, without any prints or anything else. Like this:

WalkCallback := func(dirPath string, ent *git.TreeEntry) int {
    return 0
}

rootTree.Walk(WalkCallback)

But my tracebacks are always pointing to this: https://github.com/libgit2/git2go/blob/master/tree.go#L109

err := C._go_git_treewalk(
    t.cast_ptr,
    C.GIT_TREEWALK_PRE,
    unsafe.Pointer(&callback),
)

from git2go.

@carlosmn

It looks like there's a timing aspect to this.

No it's not timing, it's actual problem with git2go code. In 1.4 invalid (contains int for example) go pointers will cause panic during GC. What we are doing here is instructing Go to treat pointer from C (with int) as it was Go's pointer (unsafe.Pointer) which is wrong. We was able get away with this in 1.3 series, but 1.4 is actually have this fixed. This is by design. If we want to operate foreign pointers uintptr should be used.

Here golang/go#9191 (comment) is explained better.

from git2go.

@kron4eg

I got the same issue like your comments. my go version is "go version go1.4.1 linux/amd64"
How shall I fix this?

Thanks in advance, it is urgent!!!!

from git2go.

@b35li use go 1.3.3, it's OK there.

from git2go.

@kron4eg
thanks for your comments.
Is there any other way, this module is one part of big project, it is hard to say switch the go version directly for me.

I am confused if the problem is with git2go, why don't we fix in git2go, while we downgrad the go version to suit it?

from git2go.

I think the problem is how git2go handle C pointers. Maybe I'm wrong, but I don't have time to fix it myself :) So I just downgraded.

from git2go.

@kron4eg Got it. thanks very much. I will downgrade it and have a try.

@carlosmn Any comments?

from git2go.

@kron4eg if we are using ints as pointers, that's definitely a bug, but I don't know which code you're saying that is doing this.

from git2go.

@carlosmn I can't say exactly (in other case I'd send PR with fix), but looks like an int pointer returned from libgit2 itself at some point. And Go1.4 runtime see it and crash.

from git2go.

@kron4eg Exactly, I have no idea what root cause it is , however, when I downgrade to 1.3.3 it indeed works. wonderful.

@carlosmn if you need any feedback from me, just @ me. Thanks.

from git2go.

I am using 1.4.2 for Tree.Walk testing and it always runtime panic for "nil pointer reference" error.
After printing some log, it shows the callback function pointer(as the payload of git_tree_walk) becomes invalid after a while. It seems that the memory of use provided callback function is recycled by GC.

func CallbackGitTreeWalk(_root *C.char, _entry *C.git_tree_entry, ptr unsafe.Pointer) C.int {
    root := C.GoString(_root)
    entry := _entry
    callback := *(*TreeWalkCallback)(ptr)
    fmt.Println("callback func %v", callback)
    return C.int(callback(root, newTreeEntry(entry)))
}

callback func 0x488c30
ext/pkg/linux_amd64/golang.org/x/tools/refactor/importgraph.a
callback func 0x488c30
ext/pkg/linux_amd64/golang.org/x/tools/refactor/lexical.a
callback func 0x488c30
ext/pkg/linux_amd64/golang.org/x/tools/refactor/rename.a
callback func <nil>
--- FAIL: TestWalk (5.86s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x5251d5]

from git2go.

I think the problem is that GC has freed the pointer of callback since it doesn't known that it is referenced by C code. So to prevent GC on the pointer of callback, I've add it into a map and delete it after C._go_git_treewalk call finished. The changed code on tree.go is here:

func (t Tree) Walk(callback TreeWalkCallback) error {
    runtime.LockOSThread()
    defer runtime.UnlockOSThread()
    cbmap := make(map[unsafe.Pointer]interface{}, 1)
    ptr := unsafe.Pointer(&callback)
    cbmap[ptr] = callback
    defer func() { delete(cbmap, ptr) }() 
    err := C._go_git_treewalk(
        t.cast_ptr,
        C.GIT_TREEWALK_PRE,
        ptr,
    )   

    if err < 0 {
        return MakeGitError(err)
    }   

    return nil 
}

@carlosmn Do you have any comments on this?
@kron4eg Could you help to test this code on your situation?

from git2go.

By the way, to reproduce the runtime panic on tree_walk, you should walk a larger git repository to make GC happened during walking. Maybe to reduce the percentage of GC by debug.SetGCPercent() is another way.

from git2go.

It's easy to reproduce the error when walking a rather big tree when starting off the GC via a goroutine. The following example crashes reliably for me on the Linux kernel repository:

package main

import (
    "fmt"
    git "github.com/libgit2/git2go"
    "os"
    "runtime"
    "time"
)

func main() {
    path := os.Args[1]

    repo, _ := git.OpenRepository(path)
    ref, _ := repo.Head()
    commit, _ := repo.LookupCommit(ref.Target())
    tree, _ := commit.Tree()

    go func() {
        time.Sleep(100 * time.Millisecond)
        runtime.GC()
    }()

    if err := tree.Walk(func(dir string, entry *git.TreeEntry) int {
        fmt.Println(entry.Name)
        return 0
    }); err != nil {
        panic(err)
    }
}

from git2go.

And yes, the error does not occur anymore after the workaround by @shinningstar is applied. Unfortunately it seems as if we don't have any way of doing what we want to do that is completly reliable, as the GC of Go is allowed to copy stacks around, thus causing pointers to become invalid. So if we hand over a Go pointer to a C function and the GC kicks in, causing pointers to become invalid, we're screwed up. As of Go 1.4 handing over Go pointers is not supported (see golang/go#8310).

So I guess the only thing we can do atm is to use the proposed workaround, as it at least seems to make the issue go away, even though it might not prove reliable.

from git2go.

If the runtime is going to arbitrarily make all of our pointers invalid, we'll have to write an indirection layer, as we keep our pointers long-term.

Whether this is due to the stack moving or overeager GC, we will need a more comprehensive method for converting pointers between libgit2 and the Go code.

from git2go.

I'm currently unable to trigger this error, even when running against the linux repo. I have however created a branch cmn/pointer-indirection which should work around both an eager GC and the stack moving issue for this particular code. The rest of the callbacks should also get this treatment.

from git2go.

@carlosmn You could see issue#10303 in golang, maybe it could help.

from git2go.

@carlosmn: I've modified the code to use your pointer indirection branch (see pks-t/git2go/pointer-indirection. Seems to work after fixing a bug in the NewHandleList function, at least my code snippet posted above does not error out anymore.

If this is the way we want to go I'll also convert other functions that use Go callbacks in C code.

from git2go.

@pks-t @carlosmn How about using the empty(NOP) function to escape callback to heap, just like go runtime team using? It seems that cgo team will use this method in Go1.5 to escape Go world pointer when passing to C world.

//go:noescape
func use(unsafe.Pointer) {}

func (t Tree) Walk(callback TreeWalkCallback) error {
    runtime.LockOSThread()
    defer runtime.UnlockOSThread()

    err := C._go_git_treewalk(
        t.cast_ptr,
        C.GIT_TREEWALK_PRE,
        unsafe.Pointer(&callback),
    )   
    use(unsafe.Pointer(&callback))

    if err < 0 {
        return MakeGitError(err)
    }

    return nil 
}

from git2go.

Well, for my part I wouldn't really want to rely on workarounds that are not officially documented. For now it is explicitly stated that no Go pointers should be passed into the C world, so I'd like to adhere to that and use the pointer indirection branch of @carlosmn insetad.

from git2go.

@pks-t OK, the pointer indirection is fine. I could patch my project locally and wish it merged into main branch soon.

from git2go.

@shinningstar I've already converted most of the code to use pointer indirection, see #196.

from git2go.