Fix a potential use-after-free in DiffNotifyCallback (libgit2#579)

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.

Author: lhchavez

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) {
@@ -165,8 +166,9 @@ func newDiffFromC(ptr *C.git_diff, repo *Repository) *Diff {
 	}
 
 	diff := &Diff{
-		ptr:  ptr,
-		repo: repo,
+		ptr:          ptr,
+		repo:         repo,
+		runFinalizer: true,
 	}
 
 	runtime.SetFinalizer(diff, (*Diff).Free)
@@ -177,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
@@ -579,9 +586,9 @@ var (
 )
 
 type diffNotifyData struct {
-	Callback DiffNotifyCallback
-	Diff     *Diff
-	Error    error
+	Callback   DiffNotifyCallback
+	Repository *Repository
+	Error      error
 }
 
 //export diffNotifyCb
@@ -595,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
@@ -613,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 {
@@ -670,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()
@@ -682,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
 }
 
@@ -697,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()
@@ -708,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
 }
 
@@ -728,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()
@@ -740,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
 }
 
@@ -755,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()
@@ -766,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
 }
 
@@ -781,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()
@@ -792,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
 }
 
@@ -819,20 +816,23 @@ 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)
 	defer C.free(unsafe.Pointer(oldBlobPath))
 	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()