From 120421c2d7bf4eab40bc1e26027e2b7f10422e80 Mon Sep 17 00:00:00 2001 From: lhchavez Date: Fri, 8 May 2020 21:04:43 -0700 Subject: [PATCH] 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. --- diff.go | 78 ++++++++++++++++++++++++++++---------------------------- patch.go | 2 +- 2 files changed, 40 insertions(+), 40 deletions(-) diff --git a/diff.go b/diff.go index 0939435..309baa1 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) { @@ -160,8 +161,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) @@ -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 @@ -574,9 +581,9 @@ var ( ) type diffNotifyData struct { - Callback DiffNotifyCallback - Diff *Diff - Error error + Callback DiffNotifyCallback + Repository *Repository + Error error } //export diffNotifyCb @@ -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{ - Callback: opts.NotifyCallback, + 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() 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() -- 2.45.2