Skip to content

Commit

Permalink
Fix use-after-free when patch outlives diff
Browse files Browse the repository at this point in the history
And added test that would trigger the bug in Valgrind.
  • Loading branch information
garious committed Dec 18, 2014
1 parent beff871 commit 1ddb55e
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 3 deletions.
5 changes: 2 additions & 3 deletions src/diff.c
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ wrap_patch(git_patch *patch)
git_patch_line_stats(NULL, &additions, &deletions, patch);
py_patch->additions = additions;
py_patch->deletions = deletions;
py_patch->patch = patch;

hunk_amounts = git_patch_num_hunks(patch);
py_patch->hunks = PyList_New(hunk_amounts);
Expand Down Expand Up @@ -129,7 +130,6 @@ wrap_patch(git_patch *patch)
}
}
}
git_patch_free(patch);

return (PyObject*) py_patch;
}
Expand All @@ -153,8 +153,7 @@ Patch_dealloc(Patch *self)
Py_CLEAR(self->hunks);
Py_CLEAR(self->old_id);
Py_CLEAR(self->new_id);
/* We do not have to free old_file_path and new_file_path, they will
* be freed by git_diff_list_free in Diff_dealloc */
git_patch_free(self->patch);
PyObject_Del(self);
}

Expand Down
1 change: 1 addition & 0 deletions src/types.h
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ typedef struct {
unsigned additions;
unsigned deletions;
unsigned flags;
git_patch* patch;
} Patch;

typedef struct {
Expand Down
16 changes: 16 additions & 0 deletions test/test_diff.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
from pygit2 import GIT_DIFF_IGNORE_WHITESPACE, GIT_DIFF_IGNORE_WHITESPACE_EOL
from . import utils
from itertools import chain
import gc


COMMIT_SHA1_1 = '5fe808e8953c12735680c257f56600cb0de44b10'
Expand Down Expand Up @@ -199,6 +200,21 @@ def get_context_for_lines(diff):
self.assertAll(lambda x: commit_a.tree[x], entries)
self.assertAll(lambda x: '+' == x, get_context_for_lines(diff_swaped))

def test_diff_free_diff_before_patch(self):
commit_a = self.repo[COMMIT_SHA1_1]
commit_b = self.repo[COMMIT_SHA1_2]
diff = commit_a.tree.diff_to_tree(commit_b.tree)
patch = diff[0]

# Free diff.
diff = None
gc.collect()

# If the underlying git_patch* is freed too early, the
# following reads will trigger errors in Valgrind.
self.assertEqual(patch.old_file_path, 'a')
self.assertEqual(patch.new_file_path, 'a')

def test_diff_revparse(self):
diff = self.repo.diff('HEAD', 'HEAD~6')
self.assertEqual(type(diff), pygit2.Diff)
Expand Down

0 comments on commit 1ddb55e

Please sign in to comment.