From cce14aa58b746b0669d9a35e9812a41124f9114e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 12 Aug 2015 10:10:18 +0200 Subject: branch: fix memory leaks related to CStrings --- branch.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/branch.go b/branch.go index 42e1216..8cf73b6 100644 --- a/branch.go +++ b/branch.go @@ -94,6 +94,7 @@ func (repo *Repository) CreateBranch(branchName string, target *Commit, force bo var ptr *C.git_reference cBranchName := C.CString(branchName) + defer C.free(unsafe.Pointer(cBranchName)) cForce := cbool(force) cSignature, err := signature.toC() @@ -134,6 +135,7 @@ func (b *Branch) Delete() error { func (b *Branch) Move(newBranchName string, force bool, signature *Signature, msg string) (*Branch, error) { var ptr *C.git_reference cNewBranchName := C.CString(newBranchName) + defer C.free(unsafe.Pointer(cNewBranchName)) cForce := cbool(force) cSignature, err := signature.toC() @@ -180,6 +182,7 @@ func (repo *Repository) LookupBranch(branchName string, bt BranchType) (*Branch, var ptr *C.git_reference cName := C.CString(branchName) + defer C.free(unsafe.Pointer(cName)) runtime.LockOSThread() defer runtime.UnlockOSThread() @@ -208,6 +211,7 @@ func (b *Branch) Name() (string, error) { func (repo *Repository) RemoteName(canonicalBranchName string) (string, error) { cName := C.CString(canonicalBranchName) + defer C.free(unsafe.Pointer(cName)) nameBuf := C.git_buf{} @@ -225,6 +229,7 @@ func (repo *Repository) RemoteName(canonicalBranchName string) (string, error) { func (b *Branch) SetUpstream(upstreamName string) error { cName := C.CString(upstreamName) + defer C.free(unsafe.Pointer(cName)) runtime.LockOSThread() defer runtime.UnlockOSThread() @@ -251,6 +256,7 @@ func (b *Branch) Upstream() (*Reference, error) { func (repo *Repository) UpstreamName(canonicalBranchName string) (string, error) { cName := C.CString(canonicalBranchName) + defer C.free(unsafe.Pointer(cName)) nameBuf := C.git_buf{} -- cgit v1.2.3 From 37bb1a6025eac8b1e689686f8830a20a266394cf Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 12 Aug 2015 10:21:49 +0200 Subject: merge: fix memory leak related to merge file opts --- merge.go | 1 + 1 file changed, 1 insertion(+) diff --git a/merge.go b/merge.go index 8c87391..183305c 100644 --- a/merge.go +++ b/merge.go @@ -394,6 +394,7 @@ func MergeFile(ancestor MergeFileInput, ours MergeFileInput, theirs MergeFileInp return nil, MakeGitError(ecode) } populateCMergeFileOptions(copts, *options) + defer freeCMergeFileOptions(copts) } runtime.LockOSThread() -- cgit v1.2.3 From 0b530c15cfff492e61c7afae55888fe1eeffe214 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 12 Aug 2015 12:44:58 +0200 Subject: clone: improve handling of remote create callback The clone options contain fields for ae remote create callback and its payload, which can be used to override the behavior when the default remote is being created for newly cloned repositories. Currently we only accept a C function as callback, though, making it overly complicated to use it. We also unconditionally `free` the payload if its address is non-`nil`, which may cause the program to segfault when the memory is not dynamically allocated. Instead, we want callers to provide a Go function that is subsequently being called by us. To do this, we introduce an indirection such that we are able to extract the provided function and payload when being called by `git_clone` and handle the return values of the user-provided function. --- clone.go | 59 ++++++++++++++++++++++++++++++++++++++++++++++++----------- clone_test.go | 45 ++++++++++++++++++++++++++++++++++++++++++++- wrapper.c | 5 +++++ 3 files changed, 97 insertions(+), 12 deletions(-) diff --git a/clone.go b/clone.go index 4de4aea..1265d7f 100644 --- a/clone.go +++ b/clone.go @@ -3,6 +3,7 @@ package git /* #include +extern void _go_git_populate_remote_cb(git_clone_options *opts); */ import "C" import ( @@ -10,13 +11,14 @@ import ( "unsafe" ) +type RemoteCreateCallback func(repo Repository, name, url string) (*Remote, ErrorCode) + type CloneOptions struct { *CheckoutOpts *RemoteCallbacks Bare bool CheckoutBranch string - RemoteCreateCallback C.git_remote_create_cb - RemoteCreatePayload unsafe.Pointer + RemoteCreateCallback RemoteCreateCallback } func Clone(url string, path string, options *CloneOptions) (*Repository, error) { @@ -30,6 +32,7 @@ func Clone(url string, path string, options *CloneOptions) (*Repository, error) copts := (*C.git_clone_options)(C.calloc(1, C.size_t(unsafe.Sizeof(C.git_clone_options{})))) populateCloneOptions(copts, options) + defer freeCloneOptions(copts) if len(options.CheckoutBranch) != 0 { copts.checkout_branch = C.CString(options.CheckoutBranch) @@ -38,9 +41,6 @@ func Clone(url string, path string, options *CloneOptions) (*Repository, error) runtime.LockOSThread() defer runtime.UnlockOSThread() ret := C.git_clone(&repo.ptr, curl, cpath, copts) - freeCheckoutOpts(&copts.checkout_opts) - C.free(unsafe.Pointer(copts.checkout_branch)) - C.free(unsafe.Pointer(copts)) if ret < 0 { return nil, MakeGitError(ret) @@ -50,6 +50,32 @@ func Clone(url string, path string, options *CloneOptions) (*Repository, error) return repo, nil } +//export remoteCreateCallback +func remoteCreateCallback(cremote unsafe.Pointer, crepo unsafe.Pointer, cname, curl *C.char, payload unsafe.Pointer) C.int { + name := C.GoString(cname) + url := C.GoString(curl) + repo := Repository{(*C.git_repository)(crepo)} + + if opts, ok := pointerHandles.Get(payload).(CloneOptions); ok { + remote, err := opts.RemoteCreateCallback(repo, name, url) + + if err == ErrOk && remote != nil { + // clear finalizer as the calling C function will + // free the remote itself + runtime.SetFinalizer(remote, nil) + + cptr := (**C.git_remote)(cremote) + *cptr = remote.ptr + } else if err == ErrOk && remote == nil { + panic("no remote created by callback") + } + + return C.int(err) + } else { + panic("invalid remote create callback") + } +} + func populateCloneOptions(ptr *C.git_clone_options, opts *CloneOptions) { C.git_clone_init_options(ptr, C.GIT_CLONE_OPTIONS_VERSION) @@ -61,12 +87,23 @@ func populateCloneOptions(ptr *C.git_clone_options, opts *CloneOptions) { ptr.bare = cbool(opts.Bare) if opts.RemoteCreateCallback != nil { - ptr.remote_cb = opts.RemoteCreateCallback - defer C.free(unsafe.Pointer(opts.RemoteCreateCallback)) + // Go v1.1 does not allow to assign a C function pointer + C._go_git_populate_remote_cb(ptr) + ptr.remote_cb_payload = pointerHandles.Track(*opts) + } +} - if opts.RemoteCreatePayload != nil { - ptr.remote_cb_payload = opts.RemoteCreatePayload - defer C.free(opts.RemoteCreatePayload) - } +func freeCloneOptions(ptr *C.git_clone_options) { + if ptr == nil { + return } + + freeCheckoutOpts(&ptr.checkout_opts) + + if ptr.remote_cb_payload != nil { + pointerHandles.Untrack(ptr.remote_cb_payload) + } + + C.free(unsafe.Pointer(ptr.checkout_branch)) + C.free(unsafe.Pointer(ptr)) } diff --git a/clone_test.go b/clone_test.go index fd83fec..86fced8 100644 --- a/clone_test.go +++ b/clone_test.go @@ -5,8 +5,11 @@ import ( "testing" ) -func TestClone(t *testing.T) { +const ( + REMOTENAME = "testremote" +) +func TestClone(t *testing.T) { repo := createTestRepo(t) defer cleanupTestRepo(t, repo) @@ -20,3 +23,43 @@ func TestClone(t *testing.T) { checkFatal(t, err) } + +func TestCloneWithCallback(t *testing.T) { + testPayload := 0 + + repo := createTestRepo(t) + defer cleanupTestRepo(t, repo) + + seedTestRepo(t, repo) + + path, err := ioutil.TempDir("", "git2go") + checkFatal(t, err) + + opts := CloneOptions{ + Bare: true, + RemoteCreateCallback: func(r Repository, name, url string) (*Remote, ErrorCode) { + testPayload += 1 + + remote, err := r.CreateRemote(REMOTENAME, url) + if err != nil { + return nil, ErrGeneric + } + + return remote, ErrOk + }, + } + + repo2, err := Clone(repo.Path(), path, &opts) + defer cleanupTestRepo(t, repo2) + + checkFatal(t, err) + + if testPayload != 1 { + t.Fatal("Payload's value has not been changed") + } + + remote, err := repo2.LookupRemote(REMOTENAME) + if err != nil || remote == nil { + t.Fatal("Remote was not created properly") + } +} diff --git a/wrapper.c b/wrapper.c index 017168d..3b88f93 100644 --- a/wrapper.c +++ b/wrapper.c @@ -5,6 +5,11 @@ typedef int (*gogit_submodule_cbk)(git_submodule *sm, const char *name, void *payload); +void _go_git_populate_remote_cb(git_clone_options *opts) +{ + opts->remote_cb = (git_remote_create_cb)remoteCreateCallback; +} + int _go_git_visit_submodule(git_repository *repo, void *fct) { return git_submodule_foreach(repo, (gogit_submodule_cbk)&SubmoduleVisitor, fct); -- cgit v1.2.3 From 157593f38da780c4f6cb6dc61275b9b36a3327bf Mon Sep 17 00:00:00 2001 From: Carlos Martín Nieto Date: Mon, 31 Aug 2015 13:13:27 +0200 Subject: Don't trat a revwalk's ITEROVER as an error --- walk.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/walk.go b/walk.go index d02044a..c314f60 100644 --- a/walk.go +++ b/walk.go @@ -173,6 +173,10 @@ func (v *RevWalk) Iterate(fun RevWalkIterator) (err error) { return nil } if err != nil { + if err.(GitError).Code == ErrIterOver { + err = nil + } + return err } -- cgit v1.2.3 From 337f25d47e9841ea9c5e1cf6bc25720c08538a9f Mon Sep 17 00:00:00 2001 From: Carlos Martín Nieto Date: Mon, 31 Aug 2015 16:05:48 +0200 Subject: Remove the vendored libgit2 submodule This is a left-over from the merge from 'next'. --- vendor/libgit2 | 1 - 1 file changed, 1 deletion(-) delete mode 160000 vendor/libgit2 diff --git a/vendor/libgit2 b/vendor/libgit2 deleted file mode 160000 index fb84cde..0000000 --- a/vendor/libgit2 +++ /dev/null @@ -1 +0,0 @@ -Subproject commit fb84cde81e11947add4ff8bb9b4084f7d76e6567 -- cgit v1.2.3 From 1ea99658246673770b21f8848e257132a29e78d1 Mon Sep 17 00:00:00 2001 From: Carlos Martín Nieto Date: Mon, 31 Aug 2015 14:01:06 +0200 Subject: Install v23 on Travis --- script/install-libgit2.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/script/install-libgit2.sh b/script/install-libgit2.sh index a6c3202..9bf6b37 100755 --- a/script/install-libgit2.sh +++ b/script/install-libgit2.sh @@ -13,9 +13,9 @@ if [ "x$TRAVIS_BRANCH" = "xnext" ]; then fi cd "${HOME}" -wget -O libgit2-0.22.3.tar.gz https://github.com/libgit2/libgit2/archive/v0.22.1.tar.gz -tar -xzvf libgit2-0.22.3.tar.gz -cd libgit2-0.22.1 && mkdir build && cd build +wget -O libgit2-0.23.1.tar.gz https://github.com/libgit2/libgit2/archive/v0.23.1.tar.gz +tar -xzvf libgit2-0.23.1.tar.gz +cd libgit2-0.23.1 && mkdir build && cd build cmake -DTHREADSAFE=ON -DBUILD_CLAR=OFF -DCMAKE_BUILD_TYPE="RelWithDebInfo" .. && make && sudo make install sudo ldconfig cd "${TRAVIS_BUILD_DIR}" -- cgit v1.2.3 From 2743bbfca3963e2a1cc21513b7ab9e8e8af70bf2 Mon Sep 17 00:00:00 2001 From: Carlos Martín Nieto Date: Mon, 31 Aug 2015 15:36:20 +0200 Subject: Test against Go 1.5 --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index 209d89f..f796389 100644 --- a/.travis.yml +++ b/.travis.yml @@ -9,6 +9,7 @@ go: - 1.2 - 1.3 - 1.4 + - 1.5 - tip matrix: -- cgit v1.2.3 From 4090c401c8bf3f062e898f75bde01e2ef27b3911 Mon Sep 17 00:00:00 2001 From: Carlos Martín Nieto Date: Mon, 31 Aug 2015 16:05:54 +0200 Subject: Don't call the finalizer on a borrowed repository When libgit2 gives us the repository for us to create the remote in, we do not own it, so we must make sure we don't try to free it. --- clone.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/clone.go b/clone.go index b8579b5..e80d14d 100644 --- a/clone.go +++ b/clone.go @@ -55,15 +55,16 @@ func remoteCreateCallback(cremote unsafe.Pointer, crepo unsafe.Pointer, cname, c name := C.GoString(cname) url := C.GoString(curl) repo := newRepositoryFromC((*C.git_repository)(crepo)) + // We don't own this repository, so make sure we don't try to free it + runtime.SetFinalizer(repo, nil) if opts, ok := pointerHandles.Get(payload).(CloneOptions); ok { remote, err := opts.RemoteCreateCallback(repo, name, url) + // clear finalizer as the calling C function will + // free the remote itself + runtime.SetFinalizer(remote, nil) if err == ErrOk && remote != nil { - // clear finalizer as the calling C function will - // free the remote itself - runtime.SetFinalizer(remote, nil) - cptr := (**C.git_remote)(cremote) *cptr = remote.ptr } else if err == ErrOk && remote == nil { -- cgit v1.2.3 From 34fb7e03ecfd3fbe7000ecfac4e30ea719d9879e Mon Sep 17 00:00:00 2001 From: Calin Seciu Date: Thu, 17 Sep 2015 17:07:34 +0300 Subject: Fix crash when using Pathspec in StatusOptions Using `StatusOptions.Pathspec` results in a fatal error panic with the message 'unexpected signal during runtime execution'. This is because the `&cpathspec` C.git_strarray gets freed in `*StatusOptions.toC()` before being passed to `C.git_status_init_options()` in `*Repository.StatusList()` (see https://github.com/libgit2/git2go/blob/b3e7705c48f038ef335204a2a9e1ee829784c30e/status.go#L138) The relevant panic trace is: ``` fatal error: unexpected signal during runtime execution [signal 0xb code=0x1 addr=0xb01dfacedebac1e pc=0x4062609] runtime stack: runtime.throw(0x469a080, 0x2a) /usr/local/Cellar/go/1.5.1/libexec/src/runtime/panic.go:527 +0x90 runtime.sigpanic() /usr/local/Cellar/go/1.5.1/libexec/src/runtime/sigpanic_unix.go:12 +0x5a goroutine 71 [syscall, locked to thread]: runtime.cgocall(0x400a720, 0xc8204e9998, 0x0) /usr/local/Cellar/go/1.5.1/libexec/src/runtime/cgocall.go:120 +0x11b fp=0xc8204e9968 sp=0xc8204e9938 github.com/libgit2/git2go._Cfunc_git_status_list_new(0xc8204c39c8, 0x5e17780, 0xc820478c40, 0xc800000000) ??:0 +0x39 fp=0xc8204e9998 sp=0xc8204e9968 github.com/libgit2/git2go.(*Repository).StatusList(0xc820013290, 0xc8204e9b58, 0x0, 0x0, 0x0) /Users/calin/go/src/github.com/libgit2/git2go/status.go:168 +0x11d fp=0xc8204e99e8 sp=0xc8204e9998 ``` --- status.go | 36 +++++++++++++----------------------- 1 file changed, 13 insertions(+), 23 deletions(-) diff --git a/status.go b/status.go index 3f5a06d..068a474 100644 --- a/status.go +++ b/status.go @@ -126,34 +126,24 @@ type StatusOptions struct { Pathspec []string } -func (opts *StatusOptions) toC() *C.git_status_options { - if opts == nil { - return nil - } - - cpathspec := C.git_strarray{} - if opts.Pathspec != nil { - cpathspec.count = C.size_t(len(opts.Pathspec)) - cpathspec.strings = makeCStringsFromStrings(opts.Pathspec) - defer freeStrarray(&cpathspec) - } - - copts := &C.git_status_options{ - version: C.GIT_STATUS_OPTIONS_VERSION, - show: C.git_status_show_t(opts.Show), - flags: C.uint(opts.Flags), - pathspec: cpathspec, - } - - return copts -} - func (v *Repository) StatusList(opts *StatusOptions) (*StatusList, error) { var ptr *C.git_status_list var copts *C.git_status_options if opts != nil { - copts = opts.toC() + cpathspec := C.git_strarray{} + if opts.Pathspec != nil { + cpathspec.count = C.size_t(len(opts.Pathspec)) + cpathspec.strings = makeCStringsFromStrings(opts.Pathspec) + defer freeStrarray(&cpathspec) + } + + copts = &C.git_status_options{ + version: C.GIT_STATUS_OPTIONS_VERSION, + show: C.git_status_show_t(opts.Show), + flags: C.uint(opts.Flags), + pathspec: cpathspec, + } } else { copts = &C.git_status_options{} ret := C.git_status_init_options(copts, C.GIT_STATUS_OPTIONS_VERSION) -- cgit v1.2.3