Skip to content

Memory allocation fixes for diff generator#5214

Merged
ethomson merged 4 commits into
libgit2:masterfrom
pks-t:pks/diff-iterator-allocation-fixes
Sep 9, 2019
Merged

Memory allocation fixes for diff generator#5214
ethomson merged 4 commits into
libgit2:masterfrom
pks-t:pks/diff-iterator-allocation-fixes

Conversation

@pks-t

@pks-t pks-t commented Aug 27, 2019

Copy link
Copy Markdown
Member

Some more fixes inspired by recent crashes that have been reported

pks-t added 4 commits August 27, 2019 10:47
While the `DIFF_FROM_ITERATORS` does make it shorter to implement the
various `git_diff_foo_to_bar` functions, it is a complex and unreadable
beast that implicitly assumes certain local variable names. This is not
something desirable to have at all and obstructs understanding and more
importantly debugging the code by quite a bit.

The `DIFF_FROM_ITERATORS` macro basically removed the burden of having
to derive the options for both iterators from a pair of iterator flags
and the diff options. This patch introduces a new function that does the
that exact and refactors all callers to manage the iterators by
themselves.

As we potentially need to allocate a shared prefix for the
iterator, we need to tell the caller to allocate that prefix as soon as
the options aren't required anymore. Thus, the function has a `char
**prefix` out pointer that will get set to the allocated string and
subsequently be free'd by the caller.

While this patch increases the line count, I personally deem this to an
acceptable tradeoff for increased readbiblity.
When preparing options for the two iterators that are about to be
diffed, we allocate a common prefix for both iterators depending on
the options passed by the user. We do not check whether the allocation
was successful, though. In fact, this isn't much of a problem, as using
a `NULL` prefix is perfectly fine. But in the end, we probably want to
detect that the system doesn't have any memory left, as we're unlikely
to be able to continue afterwards anyway.

While the issue is being fixed in the newly created function
`diff_prepare_iterator_opts`, it has been previously existing in the
previous macro `DIFF_FROM_ITERATORS` already.
When allocating tree iterator entries, we use GIT_ERROR_ALLOC_CHECK` to
check whether the allocation has failed. The macro will cause the
function to immediately return, though, leaving behind a partially
initialized iterator frame.

Fix the issue by manually checking for memory allocation errors and
using `goto done` in case of an error, popping the iterator frame.
When allocating new tree iterator frames, we zero out the allocated
memory twice. Remove one of the `memset` calls.
@ethomson

Copy link
Copy Markdown
Member

Happy to see that macro go away. I need to meditate upon that commit but the other three look obviously correct.

@ethomson ethomson merged commit 4d3392d into libgit2:master Sep 9, 2019
@pks-t pks-t deleted the pks/diff-iterator-allocation-fixes branch September 12, 2019 07:50
@pks-t

pks-t commented Sep 12, 2019

Copy link
Copy Markdown
Member Author

Thanks Ed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants