pull: plug minor memory leak after using is_descendant_of()

Commit Message

René Scharfe June 19, 2020, 1:14 p.m. UTC

cmd_pull() builds a commit_list to pass a single potential ancestor to
is_descendant_of().  The latter leaves the list intact.  Release the
allocated memory after the call.

Leaking in cmd_*() isn't a big deal, but sets a bad example for other
users of is_descendant_of().

Signed-off-by: René Scharfe <[e-mail]>
---
Patch generated with -U15 for easier review; only the pre-context is
interesting, though.  --function-context would add even more noise.
A --block-context option might be nice (include surrounding lines up to
and including the previous and next lines with lower indentation). :)
Or perhaps it's a sign that the function should be split up..

 builtin/pull.c | 1 +
 1 file changed, 1 insertion(+)

--
2.27.0

Comments

Jeff King June 19, 2020, 1:35 p.m. UTC | #1

On Fri, Jun 19, 2020 at 03:14:19PM +0200, René Scharfe wrote:

> cmd_pull() builds a commit_list to pass a single potential ancestor to
> is_descendant_of().  The latter leaves the list intact.  Release the
> allocated memory after the call.
> 
> Leaking in cmd_*() isn't a big deal, but sets a bad example for other
> users of is_descendant_of().

This also looks good to me.

> ---
> Patch generated with -U15 for easier review; only the pre-context is
> interesting, though.  --function-context would add even more noise.
> A --block-context option might be nice (include surrounding lines up to
> and including the previous and next lines with lower indentation). :)
> Or perhaps it's a sign that the function should be split up..

I wondered how:

  git -c diff.cpp.funcname='.*{' show --function-context

but think it always goes to the "^}" line, which is more than we want
(plus the start of the block is in the hunk header, which makes the
indentation look funky).

-Peff

Patch

diff --git a/builtin/pull.c b/builtin/pull.c
index 00e5857a8d..8e6572d305 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1019,30 +1019,31 @@  int cmd_pull(int argc, const char **argv, const char *prefix)
 		if (!autostash) {
 			struct commit_list *list = NULL;
 			struct commit *merge_head, *head;

 			head = lookup_commit_reference(the_repository,
 						       &orig_head);
 			commit_list_insert(head, &list);
 			merge_head = lookup_commit_reference(the_repository,
 							     &merge_heads.oid[0]);
 			if (is_descendant_of(merge_head, list)) {
 				/* we can fast-forward this without invoking rebase */
 				opt_ff = "--ff-only";
 				ran_ff = 1;
 				ret = run_merge();
 			}
+			free_commit_list(list);
 		}
 		if (!ran_ff)
 			ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);

 		if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
 			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
 			ret = rebase_submodules();

 		return ret;
 	} else {
 		int ret = run_merge();
 		if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
 			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
 			ret = update_submodules();
 		return ret;