memory leak in tree method "EntryByPath" #313

New Issue

2016-05-18T04:46:09-05:00

shinningstar commented

2016-05-18 04:46:09 -05:00

(Migrated from github.com)

We have found memory leak in our project when calling EntryByPath method. After reading source code of git2go and libgit2, C.git_tree_entry_bypath allocates memory but not free in this method. I think that's the root cause for memory leak.
By calling "defer C.git_tree_entry_free(entry)" before return is a solution for this problem. So What do you think? @carlosmn

    func (t Tree) EntryByPath(path string) (*TreeEntry, error) {
    cpath := C.CString(path)
    defer C.free(unsafe.Pointer(cpath))
    var entry *C.git_tree_entry

    runtime.LockOSThread()
    defer runtime.UnlockOSThread()

    ret := C.git_tree_entry_bypath(&entry, t.cast_ptr, cpath)
    if ret < 0 {
        return nil, MakeGitError(ret)
    }

    return newTreeEntry(entry), nil
}

We have found memory leak in our project when calling EntryByPath method. After reading source code of git2go and libgit2, C.git_tree_entry_bypath allocates memory but not free in this method. I think that's the root cause for memory leak. By calling "defer C.git_tree_entry_free(entry)" before return is a solution for this problem. So What do you think? @carlosmn ``` go func (t Tree) EntryByPath(path string) (*TreeEntry, error) { cpath := C.CString(path) defer C.free(unsafe.Pointer(cpath)) var entry *C.git_tree_entry runtime.LockOSThread() defer runtime.UnlockOSThread() ret := C.git_tree_entry_bypath(&entry, t.cast_ptr, cpath) if ret < 0 { return nil, MakeGitError(ret) } return newTreeEntry(entry), nil } ```

carlosmn commented

2016-05-18 10:51:50 -05:00

(Migrated from github.com)

Indeed, this leaks the git_tree_entry and we should be freeing it after creating the Go structure.

Indeed, this leaks the `git_tree_entry` and we should be freeing it after creating the Go structure.

shinningstar commented

2016-05-19 01:30:05 -05:00

(Migrated from github.com)

@carlosmn The Go structure is created by function newTreeEntry. Do you mean to free git_tree_entry at the end of that function?
There are some methods have called function newTreeEntry but only EntryByPath allocates extra memory because it used C.git_tree_entry_bypath . Other methods such as EntryById and EntryByName do not allocate C side memory. So we should not free their git_tree_entry.

So I think the appropriate solution is using C.git_tree_entry_free after called newTreeEntry and before EntryByPath returned, just like:

$ git diff
diff --git a/tree.go b/tree.go
index eba9f3d..1e67af3 100644
--- a/tree.go
+++ b/tree.go
@@ -87,7 +87,7 @@ func (t Tree) EntryByPath(path string) (*TreeEntry, error) {
        if ret < 0 {
                return nil, MakeGitError(ret)
        }
-
+       defer C.git_tree_entry_free(entry)
        return newTreeEntry(entry), nil
 }

@carlosmn The Go structure is created by function **newTreeEntry**. Do you mean to free git_tree_entry at the end of that function? There are some methods have called function **newTreeEntry** but only EntryByPath allocates extra memory because it used **C.git_tree_entry_bypath** . Other methods such as **EntryById** and **EntryByName** do not allocate C side memory. So we should not free their **git_tree_entry**. So I think the appropriate solution is using **C.git_tree_entry_free** after called newTreeEntry and before EntryByPath returned, just like: ``` $ git diff diff --git a/tree.go b/tree.go index eba9f3d..1e67af3 100644 --- a/tree.go +++ b/tree.go @@ -87,7 +87,7 @@ func (t Tree) EntryByPath(path string) (*TreeEntry, error) { if ret < 0 { return nil, MakeGitError(ret) } - + defer C.git_tree_entry_free(entry) return newTreeEntry(entry), nil } ```

carlosmn commented

2016-05-22 16:45:24 -05:00

(Migrated from github.com)

Yes, the caller owns the entry, as we cannot statically know whether the entry will be in the tree given as input. That code would work, though the defer seems overkill when we could have a local variable.

Sign in to join this conversation.