diff.go

@@ -133,6 +133,7 @@ func diffLineFromC(line *C.git_diff_line) DiffLine {
 type Diff struct {
 	ptr          *C.git_diff
 	repo         *Repository
+	runFinalizer bool
 }
 
 func (diff *Diff) NumDeltas() (int, error) {
@@ -162,6 +163,7 @@ func newDiffFromC(ptr *C.git_diff, repo *Repository) *Diff {
 	diff := &Diff{
 		ptr:          ptr,
 		repo:         repo,
+		runFinalizer: true,
 	}
 
 	runtime.SetFinalizer(diff, (*Diff).Free)
@@ -172,6 +174,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
@@ -575,7 +582,7 @@ var (
 
 type diffNotifyData struct {
 	Callback   DiffNotifyCallback
-	Diff     *Diff
+	Repository *Repository
 	Error      error
 }
 
@@ -590,11 +597,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 +624,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{
+		notifyData := &diffNotifyData{
 			Callback:   opts.NotifyCallback,
+			Repository: repo,
 		}
 
 		if opts.Pathspec != nil {
@@ -665,7 +682,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 +694,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 +705,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 +716,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 +732,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 +744,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 +755,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 +766,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 +777,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 +788,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 +811,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 +827,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()

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()