Repository: libgit2/git2go
Pull request #579 (2020-05-09 04:39:57)

Fix a potential use-after-free in DiffNotifyCallback
This change makes the DiffNotifyCallback always use an "unowned"
*git.Diff that does _not_ run the finalizer. Since the underlying
git_diff object is still owned by libgit2, we shouldn't be calling
Diff.Free() on it, even by accident, since that would cause a whole lot
of undefined behavior.

Now instead of storing a reference to a *git.Diff in the intermediate
state while the diff operation is being done, create a brand new
*git.Diff for every callback invocation, and only create a fully-owned
*git.Diff when the diff operation is done and the ownership is
transfered to Go.


Patch:
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 931cc09..5472f22 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -35,8 +35,8 @@ jobs:
       run: |
         git submodule update --init
         make build-libgit2-static
-        go get --tags "static" github.com/${{ github.repository }}/...
-        go build --tags "static" github.com/${{ github.repository }}/...
+        go get -tags static github.com/${{ github.repository }}/...
+        go build -tags static github.com/${{ github.repository }}/...
     - name: Test
       env:
         GOPATH: /home/runner/work/git2go
diff --git a/README.md b/README.md
index 6779c98..37c8923 100644
--- a/README.md
+++ b/README.md
@@ -60,6 +60,16 @@ Run `go get -d github.com/libgit2/git2go` to download the code and go to your `$
 
 will compile libgit2, link it into git2go and install it. The `master` branch is set up to follow the specific libgit2 version that is vendored, so trying dynamic linking may or may not work depending on the exact versions involved.
 
+In order to let Go pass the correct flags to `pkg-config`, `-tags static` needs to be passed to all `go` commands that build any binaries. For instance:
+
+    go build -tags static github.com/my/project/...
+    go test -tags static github.com/my/project/...
+    go install -tags static github.com/my/project/...
+
+One thing to take into account is that since Go expects the `pkg-config` file to be within the same directory where `make install-static` was called, so the `go.mod` file may need to have a [`replace` directive](https://github.com/golang/go/wiki/Modules#when-should-i-use-the-replace-directive) so that the correct setup is achieved. So if `git2go` is checked out at `$GOPATH/src/github.com/libgit2/git2go` and your project at `$GOPATH/src/github.com/my/project`, the `go.mod` file of `github.com/my/project` might need to have a line like
+
+    replace github.com/libgit2/git2go/v30 ../../libgit2/git2go
+
 Parallelism and network operations
 ----------------------------------
 
@@ -75,7 +85,7 @@ For the stable version, `go test` will work as usual. For the `master` branch, s
 Alternatively, you can build the library manually first and then run the tests
 
     ./script/build-libgit2-static.sh
-    go test -v --tags "static" ./...
+    go test -v -tags static ./...
 
 License
 -------
diff --git a/diff.go b/diff.go
index 0939435..e022b47 100644
--- a/diff.go
+++ b/diff.go
@@ -131,8 +131,9 @@ func diffLineFromC(line *C.git_diff_line) DiffLine {
 }
 
 type Diff struct {
-	ptr  *C.git_diff
-	repo *Repository
+	ptr          *C.git_diff
+	repo         *Repository
+	runFinalizer bool
 }
 
 func (diff *Diff) NumDeltas() (int, error) {
@@ -144,7 +145,7 @@ func (diff *Diff) NumDeltas() (int, error) {
 	return ret, nil
 }
 
-func (diff *Diff) GetDelta(index int) (DiffDelta, error) {
+func (diff *Diff) Delta(index int) (DiffDelta, error) {
 	if diff.ptr == nil {
 		return DiffDelta{}, ErrInvalid
 	}
@@ -154,14 +155,20 @@ func (diff *Diff) GetDelta(index int) (DiffDelta, error) {
 	return ret, nil
 }
 
+// deprecated: You should use `Diff.Delta()` instead.
+func (diff *Diff) GetDelta(index int) (DiffDelta, error) {
+	return diff.Delta(index)
+}
+
 func newDiffFromC(ptr *C.git_diff, repo *Repository) *Diff {
 	if ptr == nil {
 		return nil
 	}
 
 	diff := &Diff{
-		ptr:  ptr,
-		repo: repo,
+		ptr:          ptr,
+		repo:         repo,
+		runFinalizer: true,
 	}
 
 	runtime.SetFinalizer(diff, (*Diff).Free)
@@ -172,6 +179,11 @@ func (diff *Diff) Free() error {
 	if diff.ptr == nil {
 		return ErrInvalid
 	}
+	if !diff.runFinalizer {
+		// This is the case with the Diff objects that are involved in the DiffNotifyCallback.
+		diff.ptr = nil
+		return nil
+	}
 	runtime.SetFinalizer(diff, nil)
 	C.git_diff_free(diff.ptr)
 	diff.ptr = nil
@@ -574,9 +586,9 @@ var (
 )
 
 type diffNotifyData struct {
-	Callback DiffNotifyCallback
-	Diff     *Diff
-	Error    error
+	Callback   DiffNotifyCallback
+	Repository *Repository
+	Error      error
 }
 
 //export diffNotifyCb
@@ -590,11 +602,20 @@ func diffNotifyCb(_diff_so_far unsafe.Pointer, delta_to_add *C.git_diff_delta, m
 	}
 
 	if data != nil {
-		if data.Diff == nil {
-			data.Diff = newDiffFromC(diff_so_far, nil)
+		// We are not taking ownership of this diff pointer, so no finalizer is set.
+		diff := &Diff{
+			ptr:          diff_so_far,
+			repo:         data.Repository,
+			runFinalizer: false,
 		}
 
-		err := data.Callback(data.Diff, diffDeltaFromC(delta_to_add), C.GoString(matched_pathspec))
+		err := data.Callback(diff, diffDeltaFromC(delta_to_add), C.GoString(matched_pathspec))
+
+		// Since the callback could theoretically keep a reference to the diff
+		// (which could be freed by libgit2 if an error occurs later during the
+		// diffing process), this converts a use-after-free (terrible!) into a nil
+		// dereference ("just" pretty bad).
+		diff.ptr = nil
 
 		if err == ErrDeltaSkip {
 			return 1
@@ -608,11 +629,12 @@ func diffNotifyCb(_diff_so_far unsafe.Pointer, delta_to_add *C.git_diff_delta, m
 	return 0
 }
 
-func diffOptionsToC(opts *DiffOptions) (copts *C.git_diff_options, notifyData *diffNotifyData) {
+func diffOptionsToC(opts *DiffOptions, repo *Repository) (copts *C.git_diff_options) {
 	cpathspec := C.git_strarray{}
 	if opts != nil {
-		notifyData = &diffNotifyData{
-			Callback: opts.NotifyCallback,
+		notifyData := &diffNotifyData{
+			Callback:   opts.NotifyCallback,
+			Repository: repo,
 		}
 
 		if opts.Pathspec != nil {
@@ -665,7 +687,7 @@ func (v *Repository) DiffTreeToTree(oldTree, newTree *Tree, opts *DiffOptions) (
 		newPtr = newTree.cast_ptr
 	}
 
-	copts, notifyData := diffOptionsToC(opts)
+	copts := diffOptionsToC(opts, v)
 	defer freeDiffOptions(copts)
 
 	runtime.LockOSThread()
@@ -677,10 +699,6 @@ func (v *Repository) DiffTreeToTree(oldTree, newTree *Tree, opts *DiffOptions) (
 	if ecode < 0 {
 		return nil, MakeGitError(ecode)
 	}
-
-	if notifyData != nil && notifyData.Diff != nil {
-		return notifyData.Diff, nil
-	}
 	return newDiffFromC(diffPtr, v), nil
 }
 
@@ -692,7 +710,7 @@ func (v *Repository) DiffTreeToWorkdir(oldTree *Tree, opts *DiffOptions) (*Diff,
 		oldPtr = oldTree.cast_ptr
 	}
 
-	copts, notifyData := diffOptionsToC(opts)
+	copts := diffOptionsToC(opts, v)
 	defer freeDiffOptions(copts)
 
 	runtime.LockOSThread()
@@ -703,10 +721,6 @@ func (v *Repository) DiffTreeToWorkdir(oldTree *Tree, opts *DiffOptions) (*Diff,
 	if ecode < 0 {
 		return nil, MakeGitError(ecode)
 	}
-
-	if notifyData != nil && notifyData.Diff != nil {
-		return notifyData.Diff, nil
-	}
 	return newDiffFromC(diffPtr, v), nil
 }
 
@@ -723,7 +737,7 @@ func (v *Repository) DiffTreeToIndex(oldTree *Tree, index *Index, opts *DiffOpti
 		indexPtr = index.ptr
 	}
 
-	copts, notifyData := diffOptionsToC(opts)
+	copts := diffOptionsToC(opts, v)
 	defer freeDiffOptions(copts)
 
 	runtime.LockOSThread()
@@ -735,10 +749,6 @@ func (v *Repository) DiffTreeToIndex(oldTree *Tree, index *Index, opts *DiffOpti
 	if ecode < 0 {
 		return nil, MakeGitError(ecode)
 	}
-
-	if notifyData != nil && notifyData.Diff != nil {
-		return notifyData.Diff, nil
-	}
 	return newDiffFromC(diffPtr, v), nil
 }
 
@@ -750,7 +760,7 @@ func (v *Repository) DiffTreeToWorkdirWithIndex(oldTree *Tree, opts *DiffOptions
 		oldPtr = oldTree.cast_ptr
 	}
 
-	copts, notifyData := diffOptionsToC(opts)
+	copts := diffOptionsToC(opts, v)
 	defer freeDiffOptions(copts)
 
 	runtime.LockOSThread()
@@ -761,10 +771,6 @@ func (v *Repository) DiffTreeToWorkdirWithIndex(oldTree *Tree, opts *DiffOptions
 	if ecode < 0 {
 		return nil, MakeGitError(ecode)
 	}
-
-	if notifyData != nil && notifyData.Diff != nil {
-		return notifyData.Diff, nil
-	}
 	return newDiffFromC(diffPtr, v), nil
 }
 
@@ -776,7 +782,7 @@ func (v *Repository) DiffIndexToWorkdir(index *Index, opts *DiffOptions) (*Diff,
 		indexPtr = index.ptr
 	}
 
-	copts, notifyData := diffOptionsToC(opts)
+	copts := diffOptionsToC(opts, v)
 	defer freeDiffOptions(copts)
 
 	runtime.LockOSThread()
@@ -787,10 +793,6 @@ func (v *Repository) DiffIndexToWorkdir(index *Index, opts *DiffOptions) (*Diff,
 	if ecode < 0 {
 		return nil, MakeGitError(ecode)
 	}
-
-	if notifyData != nil && notifyData.Diff != nil {
-		return notifyData.Diff, nil
-	}
 	return newDiffFromC(diffPtr, v), nil
 }
 
@@ -814,12 +816,15 @@ func DiffBlobs(oldBlob *Blob, oldAsPath string, newBlob *Blob, newAsPath string,
 	handle := pointerHandles.Track(data)
 	defer pointerHandles.Untrack(handle)
 
+	var repo *Repository
 	var oldBlobPtr, newBlobPtr *C.git_blob
 	if oldBlob != nil {
 		oldBlobPtr = oldBlob.cast_ptr
+		repo = oldBlob.repo
 	}
 	if newBlob != nil {
 		newBlobPtr = newBlob.cast_ptr
+		repo = newBlob.repo
 	}
 
 	oldBlobPath := C.CString(oldAsPath)
@@ -827,7 +832,7 @@ func DiffBlobs(oldBlob *Blob, oldAsPath string, newBlob *Blob, newAsPath string,
 	newBlobPath := C.CString(newAsPath)
 	defer C.free(unsafe.Pointer(newBlobPath))
 
-	copts, _ := diffOptionsToC(opts)
+	copts := diffOptionsToC(opts, repo)
 	defer freeDiffOptions(copts)
 
 	runtime.LockOSThread()
diff --git a/git.go b/git.go
index 0459dde..bd01b6d 100644
--- a/git.go
+++ b/git.go
@@ -139,6 +139,16 @@ func init() {
 	C.git_openssl_set_locking()
 }
 
+// Shutdown frees all the resources acquired by libgit2. Make sure no
+// references to any git2go objects are live before calling this.
+// After this is called, invoking any function from this library will result in
+// undefined behavior, so make sure this is called carefully.
+func Shutdown() {
+	pointerHandles.Clear()
+
+	C.git_libgit2_shutdown()
+}
+
 // Oid represents the id for a Git object.
 type Oid [20]byte
 
diff --git a/handles.go b/handles.go
index d27d3c3..c0d4b3c 100644
--- a/handles.go
+++ b/handles.go
@@ -43,6 +43,16 @@ func (v *HandleList) Untrack(handle unsafe.Pointer) {
 	v.Unlock()
 }
 
+// Clear stops tracking all the managed pointers.
+func (v *HandleList) Clear() {
+	v.Lock()
+	for handle := range v.handles {
+		delete(v.handles, handle)
+		C.free(handle)
+	}
+	v.Unlock()
+}
+
 // Get retrieves the pointer from the given handle
 func (v *HandleList) Get(handle unsafe.Pointer) interface{} {
 	v.RLock()
diff --git a/index.go b/index.go
index 50538bd..150abf2 100644
--- a/index.go
+++ b/index.go
@@ -528,7 +528,7 @@ type IndexConflict struct {
 	Their    *IndexEntry
 }
 
-func (v *Index) GetConflict(path string) (IndexConflict, error) {
+func (v *Index) Conflict(path string) (IndexConflict, error) {
 
 	var cancestor *C.git_index_entry
 	var cour *C.git_index_entry
@@ -553,6 +553,11 @@ func (v *Index) GetConflict(path string) (IndexConflict, error) {
 	return ret, nil
 }
 
+// deprecated: You should use `Index.Conflict()` instead.
+func (v *Index) GetConflict(path string) (IndexConflict, error) {
+	return v.Conflict(path)
+}
+
 func (v *Index) RemoveConflict(path string) error {
 
 	cpath := C.CString(path)
diff --git a/merge.go b/merge.go
index 06e98a0..6ca818e 100644
--- a/merge.go
+++ b/merge.go
@@ -126,6 +126,14 @@ const (
 	// continue resolving conflicts.  The merge operation will fail with
 	// GIT_EMERGECONFLICT and no index will be returned.
 	MergeTreeFailOnConflict MergeTreeFlag = C.GIT_MERGE_FAIL_ON_CONFLICT
+	// MergeTreeSkipREUC specifies not to write the REUC extension on the
+	// generated index.
+	MergeTreeSkipREUC MergeTreeFlag = C.GIT_MERGE_SKIP_REUC
+	// MergeTreeNoRecursive specifies not to build a recursive merge base (by
+	// merging the multiple merge bases) if the commits being merged have
+	// multiple merge bases. Instead, the first base is used.
+	// This flag provides a similar merge base to `git-merge-resolve`.
+	MergeTreeNoRecursive MergeTreeFlag = C.GIT_MERGE_NO_RECURSIVE
 )
 
 type MergeOptions struct {
diff --git a/message.go b/message.go
new file mode 100644
index 0000000..255e8d7
--- /dev/null
+++ b/message.go
@@ -0,0 +1,42 @@
+package git
+
+/*
+#include <git2.h>
+*/
+import "C"
+import (
+	"runtime"
+	"unsafe"
+)
+
+// Trailer represents a single git message trailer.
+type Trailer struct {
+	Key   string
+	Value string
+}
+
+// MessageTrailers parses trailers out of a message, returning a slice of
+// Trailer structs. Trailers are key/value pairs in the last paragraph of a
+// message, not including any patches or conflicts that may be present.
+func MessageTrailers(message string) ([]Trailer, error) {
+	var trailersC C.git_message_trailer_array
+
+	messageC := C.CString(message)
+	defer C.free(unsafe.Pointer(messageC))
+
+	runtime.LockOSThread()
+	defer runtime.UnlockOSThread()
+
+	ecode := C.git_message_trailers(&trailersC, messageC)
+	if ecode < 0 {
+		return nil, MakeGitError(ecode)
+	}
+	defer C.git_message_trailer_array_free(&trailersC)
+	trailers := make([]Trailer, trailersC.count)
+	var trailer *C.git_message_trailer
+	for i, p := 0, uintptr(unsafe.Pointer(trailersC.trailers)); i < int(trailersC.count); i, p = i+1, p+unsafe.Sizeof(C.git_message_trailer{}) {
+		trailer = (*C.git_message_trailer)(unsafe.Pointer(p))
+		trailers[i] = Trailer{Key: C.GoString(trailer.key), Value: C.GoString(trailer.value)}
+	}
+	return trailers, nil
+}
diff --git a/patch.go b/patch.go
index 6a16b5f..4c6648e 100644
--- a/patch.go
+++ b/patch.go
@@ -77,7 +77,7 @@ func (v *Repository) PatchFromBuffers(oldPath, newPath string, oldBuf, newBuf []
 	cNewPath := C.CString(newPath)
 	defer C.free(unsafe.Pointer(cNewPath))
 
-	copts, _ := diffOptionsToC(opts)
+	copts := diffOptionsToC(opts, v)
 	defer freeDiffOptions(copts)
 
 	runtime.LockOSThread()
diff --git a/script/build-libgit2.sh b/script/build-libgit2.sh
index 839b8f3..acbc84a 100755
--- a/script/build-libgit2.sh
+++ b/script/build-libgit2.sh
@@ -41,8 +41,6 @@ cmake -DTHREADSAFE=ON \
       -DCMAKE_C_FLAGS=-fPIC \
       -DCMAKE_BUILD_TYPE="RelWithDebInfo" \
       -DCMAKE_INSTALL_PREFIX="${BUILD_PATH}/install" \
-      -DINCLUDE_INSTALL_DIR="${BUILD_PATH}/install/include" \
-      -DLIB_INSTALL_DIR="${BUILD_PATH}/install/lib" \
       "${VENDORED_PATH}" &&
 
 exec cmake --build . --target install
diff --git a/settings.go b/settings.go
index c6dfe1c..9f2ec02 100644
--- a/settings.go
+++ b/settings.go
@@ -93,10 +93,15 @@ func EnableCaching(enabled bool) error {
 	}
 }
 
-func GetCachedMemory() (current int, allowed int, err error) {
+func CachedMemory() (current int, allowed int, err error) {
 	return getSizetSizet(C.GIT_OPT_GET_CACHED_MEMORY)
 }
 
+// deprecated: You should use `CachedMemory()` instead.
+func GetCachedMemory() (current int, allowed int, err error) {
+	return CachedMemory()
+}
+
 func SetCacheMaxSize(maxSize int) error {
 	return setSizet(C.GIT_OPT_SET_CACHE_MAX_SIZE, maxSize)
 }


Test patch:
diff --git a/merge_test.go b/merge_test.go
index 7cf034f..319bef3 100644
--- a/merge_test.go
+++ b/merge_test.go
@@ -109,7 +109,7 @@ func TestMergeTreesWithoutAncestor(t *testing.T) {
 	if !index.HasConflicts() {
 		t.Fatal("expected conflicts in the index")
 	}
-	_, err = index.GetConflict("README")
+	_, err = index.Conflict("README")
 	checkFatal(t, err)
 
 }
diff --git a/message_test.go b/message_test.go
new file mode 100644
index 0000000..f33ccb7
--- /dev/null
+++ b/message_test.go
@@ -0,0 +1,42 @@
+package git
+
+import (
+	"fmt"
+	"reflect"
+	"testing"
+)
+
+func TestTrailers(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		input    string
+		expected []Trailer
+	}{
+		{
+			"commit with zero trailers\n",
+			[]Trailer{},
+		},
+		{
+			"commit with one trailer\n\nCo-authored-by: Alice <alice@example.com>\n",
+			[]Trailer{
+				Trailer{Key: "Co-authored-by", Value: "Alice <alice@example.com>"},
+			},
+		},
+		{
+			"commit with two trailers\n\nCo-authored-by: Alice <alice@example.com>\nSigned-off-by: Bob <bob@example.com>\n",
+			[]Trailer{
+				Trailer{Key: "Co-authored-by", Value: "Alice <alice@example.com>"},
+				Trailer{Key: "Signed-off-by", Value: "Bob <bob@example.com>"}},
+		},
+	}
+	for _, test := range tests {
+		fmt.Printf("%s", test.input)
+		actual, err := MessageTrailers(test.input)
+		if err != nil {
+			t.Errorf("Trailers returned an unexpected error: %v", err)
+		}
+		if !reflect.DeepEqual(test.expected, actual) {
+			t.Errorf("expecting %#v\ngot %#v", test.expected, actual)
+		}
+	}
+}
diff --git a/settings_test.go b/settings_test.go
index 4e45567..150ee7c 100644
--- a/settings_test.go
+++ b/settings_test.go
@@ -57,8 +57,8 @@ func TestEnableCaching(t *testing.T) {
 	checkFatal(t, err)
 }
 
-func TestGetCachedMemory(t *testing.T) {
-	current, allowed, err := GetCachedMemory()
+func TestCachedMemory(t *testing.T) {
+	current, allowed, err := CachedMemory()
 	checkFatal(t, err)
 
 	if current < 0 {