| Message ID | patch-v2-15.20-d5210017cab-20221230T020341Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leak fixes: various simple leak fixes | expand |
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? > }
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.
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; } 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; } 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; } 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; }
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. 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(-)