Re: [PATCH v3 3/6] log: fix a memory leak in "git show <revision>..."

[Jeff King <peff@peff.net>]

On Tue, Aug 02, 2022 at 05:33:13PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Let's fix this memory leak. Now when we encounter an OBJ_COMMIT we
> save away the "rev.pending" before clearing it. We then add a single
> commit to it, which our indirect invocation of prepare_revision_walk()
> will remove. After that we restore the "rev.pending".
> 
> Our "rev.pending" will then get free'd by the release_revisions()
> added in f6bfea0ad01 (revisions API users: use release_revisions() in
> builtin/log.c, 2022-04-13)

OK, I agree this is doing the correct thing. I actually found the
earlier "let's dissociate rev.pending from rev entirely" approach easier
to reason about, though.

> diff --git a/builtin/log.c b/builtin/log.c
> index 88a5e98875a..b4b1d974617 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -743,11 +743,17 @@ int cmd_show(int argc, const char **argv, const char *prefix)
>  			rev.shown_one = 1;
>  			break;
>  		case OBJ_COMMIT:
> +		{
> +			struct object_array old;
> +
> +			memcpy(&old, &rev.pending, sizeof(old));
>  			rev.pending.nr = rev.pending.alloc = 0;
>  			rev.pending.objects = NULL;
>  			add_object_array(o, name, &rev.pending);
>  			ret = cmd_log_walk_no_free(&rev);
> +			memcpy(&rev.pending, &old, sizeof(rev.pending));
>  			break;
> +		}

Here we overwrite the one-item rev.pending without freeing it, but just
immediately after instead of before. It's a little subtle, but your
comment in the commit message:

  [...] and only free the new "rev.pending" in the "OBJ_COMMIT" case arm
  as prepare_revision_walk() would draw it down.

covers that. IMHO that could be spelled out a bit more (particularly
that this only works for OBJ_COMMIT, but that's OK because that's the
only type we're adding), but I can live with it.

-Peff