Re: [PATCH v2 15/20] connected.c: free(new_pack) in check_connected()
- From
René Scharfe <l.s.r@web.de>
- Date
- Dec 30, 2022, 16:17 UTC
- Message-ID
- <755d84d5-62f1-4f4e-ff31-4604c0dca0a9@web.de>
- In-Reply-To
- <patch-v2-15.20-d5210017cab-20221230T020341Z-avarab@gmail.com>
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.
Show 18 quoted lines
> 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.
Show 10 quoted lines
> } > > 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.
Show 11 quoted lines
> }
>
> 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.
Show 12 quoted lines
> }
>
> 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?
> }