| Message ID | patch-v6-18.19-aa33f7e05c8-20230202T094704Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leak fixes: various simple leak fixes | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > The set_refspecs() caller of refspec_append_mapped() (added in [1]) > left open the question[2] of whether the "remote" we lazily fetch > might be NULL in the "[...]uniquely name our ref?" case, as > remote_get() can return NULL. > > If we got past the "[...]uniquely name our ref?" case we'd have > already segfaulted if we tried to dereference it as > "remote->push.nr". In these cases the config mechanism & previous > remote validation will have bailed out earlier. > > Let's refactor this code to clarify that, we'll now BUG() out if we > can't get a "remote", and will no longer retrieve it for these common > cases where we don't need it. Another thing this does, if I am not mistaken, and the above does not mention, is, that the old code called get_local_heads() for each and every ref from the command line, and the second and subsequent calls to the function and assignment to local_refs would leak the entire local_refs linked list. This step does not release the linked list at the end, but at least it stops leaking it in each iteration of the loop.
On Thu, Feb 02 2023, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> The set_refspecs() caller of refspec_append_mapped() (added in [1]) >> left open the question[2] of whether the "remote" we lazily fetch >> might be NULL in the "[...]uniquely name our ref?" case, as >> remote_get() can return NULL. >> >> If we got past the "[...]uniquely name our ref?" case we'd have >> already segfaulted if we tried to dereference it as >> "remote->push.nr". In these cases the config mechanism & previous >> remote validation will have bailed out earlier. >> >> Let's refactor this code to clarify that, we'll now BUG() out if we >> can't get a "remote", and will no longer retrieve it for these common >> cases where we don't need it. > > Another thing this does, if I am not mistaken, and the above does > not mention, is, that the old code called get_local_heads() for each > and every ref from the command line, and the second and subsequent > calls to the function and assignment to local_refs would leak the > entire local_refs linked list. This step does not release the > linked list at the end, but at least it stops leaking it in each > iteration of the loop. I think you're mistaken, or I'm misunderstanding you. For e.g. the 29th test in t4301-merge-tree-write-tree.sh where we do: git push read-only side1 side2 side3 We'll only invoke "get_local_heads()" once, not once for each of the refs. That's part of the reason for this refactoring: It *looks* as though we might do it N times, but in practice we always get hte "remote", so we only leak it once.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I think you're mistaken, or I'm misunderstanding you. For e.g. the 29th > test in t4301-merge-tree-write-tree.sh where we do: > > git push read-only side1 side2 side3 > > We'll only invoke "get_local_heads()" once, not once for each of the > refs. Ahh. get_local_heads() appears inside this conditional in the original. } else if (!strchr(ref, ':')) { if (!remote) { remote = remote_get(repo); local_refs = get_local_heads(); } refspec_append_mapped(...); } else ... So the original was using !remote as the switch to make both remote and local_refs assigned onnly once. The rewritten code makes them separately "find first non-null". OK.
diff --git a/builtin/push.c b/builtin/push.c index 60ac8017e52..97b35eca3ab 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -63,16 +63,9 @@ static struct refspec rs = REFSPEC_INIT_PUSH; static struct string_list push_options_config = STRING_LIST_INIT_DUP; static void refspec_append_mapped(struct refspec *refspec, const char *ref, - struct remote *remote, struct ref *local_refs) + struct remote *remote, struct ref *matched) { const char *branch_name; - struct ref *matched = NULL; - - /* Does "ref" uniquely name our ref? */ - if (count_refspec_match(ref, local_refs, &matched) != 1) { - refspec_append(refspec, ref); - return; - } if (remote->push.nr) { struct refspec_item query; @@ -120,12 +113,24 @@ static void set_refspecs(const char **refs, int nr, const char *repo) die(_("--delete only accepts plain target ref names")); refspec_appendf(&rs, ":%s", ref); } else if (!strchr(ref, ':')) { - if (!remote) { - /* lazily grab remote and local_refs */ - remote = remote_get(repo); + struct ref *matched = NULL; + + /* lazily grab local_refs */ + if (!local_refs) local_refs = get_local_heads(); + + /* Does "ref" uniquely name our ref? */ + if (count_refspec_match(ref, local_refs, &matched) != 1) { + refspec_append(&rs, ref); + } else { + /* lazily grab remote */ + if (!remote) + remote = remote_get(repo); + if (!remote) + BUG("must get a remote for repo '%s'", repo); + + refspec_append_mapped(&rs, ref, remote, matched); } - refspec_append_mapped(&rs, ref, remote, local_refs); } else refspec_append(&rs, ref); }
The set_refspecs() caller of refspec_append_mapped() (added in [1]) left open the question[2] of whether the "remote" we lazily fetch might be NULL in the "[...]uniquely name our ref?" case, as remote_get() can return NULL. If we got past the "[...]uniquely name our ref?" case we'd have already segfaulted if we tried to dereference it as "remote->push.nr". In these cases the config mechanism & previous remote validation will have bailed out earlier. Let's refactor this code to clarify that, we'll now BUG() out if we can't get a "remote", and will no longer retrieve it for these common cases where we don't need it. 1. ca02465b413 (push: use remote.$name.push as a refmap, 2013-12-03) 2. https://lore.kernel.org/git/c0c07b89-7eaf-21cd-748e-e14ea57f09fd@web.de/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/push.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)