Re: [PATCH v2 15/20] connected.c: free(new_pack) in check_connected()

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

On Fri, Dec 30 2022, René Scharfe wrote:

> Am 30.12.22 um 03:18 schrieb Ævar Arnfjörð Bjarmason:
>> Fix a memory leak that's been with us since this code was introduced
>> in c6807a40dcd (clone: open a shortcut for connectivity check,
>> 2013-05-26). We'd never free() the "new_pack" that we'd potentially
>> allocate.
>
> That's obviously not true because the patch removes free() calls for
> it, so at least some execution paths were already cleaning it up.
>
> Taking a closer look, though: Is there a leaking execution path
> without this patch at all?
>
>    $ git grep -n return connected.c
>    connected.c:41:		return err;
>    connected.c:89:		return 0;
>    connected.c:127:		return error(_("Could not run 'git rev-list'"));
>    connected.c:161:	return finish_command(&rev_list) || err;
>
> So there are four returns before.
>
>> Since it's initialized to "NULL" it's safe to call free()
>> here unconditionally.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  connected.c | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/connected.c b/connected.c
>> index b90fd61790c..e4d404e10b2 100644
>> --- a/connected.c
>> +++ b/connected.c
>> @@ -38,7 +38,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>>  	if (!oid) {
>>  		if (opt->err_fd)
>>  			close(opt->err_fd);
>> -		return err;
>> +		goto cleanup;
>
> Where are we?
>
>    $ git grep -n new_pack.= connected.c
>    connected.c:29:	struct packed_git *new_pack = NULL;
>    connected.c:53:		new_pack = add_packed_git(idx_file.buf, idx_file.len, 1);
>
> After new_pack is initialized to NULL, but before it is set to
> point to some actual pack object.  So no free() is needed here.
>
>>  	}
>>
>>  	if (transport && transport->smart_options &&
>> @@ -85,8 +85,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>>  promisor_pack_found:
>>  			;
>>  		} while ((oid = fn(cb_data)) != NULL);
>> -		free(new_pack);
>> -		return 0;
>> +		goto cleanup;
>
> free() removed, no leak before.
>
>>  	}
>>
>>  no_promisor_pack_found:
>> @@ -123,8 +122,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>>  		rev_list.no_stderr = opt->quiet;
>>
>>  	if (start_command(&rev_list)) {
>> -		free(new_pack);
>> -		return error(_("Could not run 'git rev-list'"));
>> +		err = error(_("Could not run 'git rev-list'"));
>> +		goto cleanup;
>
> Same here.
>
>>  	}
>>
>>  	sigchain_push(SIGPIPE, SIG_IGN);
>> @@ -157,6 +156,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>>  		err = error_errno(_("failed to close rev-list's stdin"));
>>
>>  	sigchain_pop(SIGPIPE);
>> +	err = finish_command(&rev_list) || err;
>> +cleanup:
>>  	free(new_pack);
>> -	return finish_command(&rev_list) || err;
>> +	return err;
>
> free() kept, no leak before.
>
> And that's all four returns.
>
> So there is no leak to begin with here, or am I missing something?

The commit message was just out of date, this was just the
post-refactoring for the already landed dd4143e7bf4 (connected.c: free
the "struct packed_git", 2022-11-08), but I forgot to update it.

I'll just drop this patch, as this series is meant to be leak fixes, not
such post-refactorings, sorry.