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