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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Jul 18 2022, Jeff King wrote:

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

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

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