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?
> }
