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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 18, 2022, 15:08 UTC
Message-ID: <220718.864jzexxn6.gmgdl@evledraar.gmail.com>
In-Reply-To: <YtV0xB9d6g/XwkLk@coredump.intra.peff.net>

On Mon, Jul 18 2022, Jeff King wrote:

Show 23 quoted lines

> 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.

Thanks, I'll mull that over, and didn't have time to do it now. FWIW I was aiming to end up with the least bit of invasiveness into the private bits of revision.[ch], i.e. we know we can fiddle with rev.pending independently of other members, but it's rather nasty to assume that. So restoring the previous state and having release_revisions() handle it is a bit less nasty.

Of course we assume things about its internals here anyway, but this way it's only for the narrow scope of when we're clobbering it for "show".

Show 14 quoted lines

> 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. ;)

*nod*

Show 26 quoted lines

>> 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.

I can add an explanation for this, this too passes, note that it's "TEST_FAILS..." not "TEST_PASSES...".

I..e. in an earlier series I whitelisted all of lib-git-svn.sh, unless that variable was specificed, which was less churn than adding "TEST_PASSES_SANITIZE_LEAK=true" to all of them (as more than 1/2 of them passed already).

So there's no new leaks here, and only new passing tests.