From 14e29a3390cf38a6c596abfa71caa6765d36b86f Mon Sep 17 00:00:00 2001 From: lhchavez Date: Sun, 21 Jun 2020 06:45:39 -0700 Subject: [PATCH] Fix a potential use-after-free in DiffNotifyCallback (#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. (cherry picked from commit c78ae57de63857d53f05ed088f308699622360fe) --- diff.go | 78 ++++++++++++++++++++++++++++---------------------------- patch.go | 2 +- 2 files changed, 40 insertions(+), 40 deletions(-) diff --git a/diff.go b/diff.go index 7a41f86..7f824fa 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) { @@ -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,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) @@ -832,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/patch.go b/patch.go index 7e6f71d..1c4d633 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()