Re: [PATCH 4/6] log: fix common "rev.pending" memory leak in "git show"

[Jeff King <peff@peff.net>]

On Wed, Jul 13, 2022 at 03:10:33PM +0200, Ævar Arnfjörð Bjarmason wrote:

> diff --git a/builtin/log.c b/builtin/log.c
> index e0f40798d45..77ec256a8ae 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -747,6 +747,8 @@ int cmd_show(int argc, const char **argv, const char *prefix)
>  			memcpy(&rev.pending, &blank, sizeof(rev.pending));
>  			add_object_array(o, name, &rev.pending);
>  			ret = cmd_log_walk_no_free(&rev);
> +			object_array_clear(&rev.pending);
> +			memcpy(&rev.pending, &cp, sizeof(rev.pending));
>  			break;

OK, so we clear the fake one-entry pending array we just created. That
make sense.

And then we have to restore rev.pending, because we don't otherwise
clean up cp. That makes sense in the context of your previous patch, but
if you take my suggestion there to separate "cp" from rev.pending
entirely, and clean it up explicitly, then this second memcpy() goes
away. IMHO that leaves the resulting code much easier to follow, as the
lifetimes are clearly distinct.

Two alternatives I briefly considered that don't work:

  - you can't just leave the one-item rev.pending in place to get
    cleaned up by release_revisions(), because we may show multiple
    commits. We have to clean each one as we go.

  - you can't just object_array_clear(&rev.pending) _before_ clearing
    it, because in the initial state it's still a copy of "cp", and thus
    would hose the array we're iterating over.

This whole "reuse rev, but tweak its pending array" feels a bit sketchy
to me in general. But it has been this way since 2006, and anyway is
completely out of scope for your series, so let's hold our nose and
continue. ;)

> diff --git a/t/t7007-show.sh b/t/t7007-show.sh
> index d6cc69e0f2c..f908a4d1abc 100755
> --- a/t/t7007-show.sh
> +++ b/t/t7007-show.sh
> @@ -2,6 +2,7 @@
>  
>  test_description='git show'
>  
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh

Lots of stuff now passes, which is good, but....

> diff --git a/t/t9122-git-svn-author.sh b/t/t9122-git-svn-author.sh
> index 527ba3d2932..0fc289ae0f0 100755
> --- a/t/t9122-git-svn-author.sh
> +++ b/t/t9122-git-svn-author.sh
> @@ -2,7 +2,6 @@
>  
>  test_description='git svn authorship'
>  
> -TEST_FAILS_SANITIZE_LEAK=true
>  . ./lib-git-svn.sh

...this one (and t9162) don't anymore? Are these hunks a mistake? If
not, this feels like something that needs to go into the commit message.

-Peff