{"thread":{"id":"54191","subject":"[PATCH 1/2] push: release strbufs used for refspec formatting","startedAt":"2020-09-05T14:48:02Z","lastAt":"2020-09-05T14:50:07Z","messageCount":2,"participants":["René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"405061","messageId":"e0c7b847-b8c7-6880-748e-1e5b32934ed5@web.de","threadId":"54191","inReplyTo":null,"subject":"[PATCH 1/2] push: release strbufs used for refspec formatting","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-09-05T14:47:47Z","receivedAt":"2020-09-05T14:48:02Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"map_refspec() either returns the passed in ref string or a detached\nstrbuf.  This makes it hard for callers to release the possibly\nallocated memory, and set_refspecs() consequently leaks it.\n\nLet map_refspec() append any refspecs directly and release its own\nstrbufs after use.  Rename it to refspec_append_mapped() and don't\nreturn anything to reflect its increased responsibility.\n\nset_refspecs() also leaks its strbufs.  Do the same here and directly\ncall refspec_append() in each if branch instead of holding onto a\ndetached strbuf, then dispose of the allocated memory after use.  We\nneed to add an else branch for the final call because all the other\nconditional branches already add their formatted refspec now.\n\nsetup_push_upstream() and setup_push_current() forgot to release their\nstrbufs as well; plug these leaks, too, while at it.\n\nNone of these leaks were likely to impact users, because the number\nand sizes of refspecs are usually small and the allocations are only\ndone once per program run.  Clean them up nevertheless, as another\nstep on the long road towards zero memory leaks.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nPatch generated with --function-context for easier review.\n\n builtin/push.c | 34 ++++++++++++++++++++++------------\n 1 file changed, 22 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex bc94078e72..0f3c108c93 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -61,76 +61,84 @@ static struct refspec rs = REFSPEC_INIT_PUSH;\n\n static struct string_list push_options_config = STRING_LIST_INIT_DUP;\n\n-static const char *map_refspec(const char *ref,\n-\t\t\t       struct remote *remote, struct ref *local_refs)\n+static void refspec_append_mapped(struct refspec *refspec, const char *ref,\n+\t\t\t\t  struct remote *remote, struct ref *local_refs)\n {\n \tconst char *branch_name;\n \tstruct ref *matched = NULL;\n\n \t/* Does \"ref\" uniquely name our ref? */\n-\tif (count_refspec_match(ref, local_refs, &matched) != 1)\n-\t\treturn ref;\n+\tif (count_refspec_match(ref, local_refs, &matched) != 1) {\n+\t\trefspec_append(refspec, ref);\n+\t\treturn;\n+\t}\n\n \tif (remote->push.nr) {\n \t\tstruct refspec_item query;\n \t\tmemset(&query, 0, sizeof(struct refspec_item));\n \t\tquery.src = matched->name;\n \t\tif (!query_refspecs(&remote->push, &query) && query.dst) {\n \t\t\tstruct strbuf buf = STRBUF_INIT;\n \t\t\tstrbuf_addf(&buf, \"%s%s:%s\",\n \t\t\t\t    query.force ? \"+\" : \"\",\n \t\t\t\t    query.src, query.dst);\n-\t\t\treturn strbuf_detach(&buf, NULL);\n+\t\t\trefspec_append(refspec, buf.buf);\n+\t\t\tstrbuf_release(&buf);\n+\t\t\treturn;\n \t\t}\n \t}\n\n \tif (push_default == PUSH_DEFAULT_UPSTREAM &&\n \t    skip_prefix(matched->name, \"refs/heads/\", &branch_name)) {\n \t\tstruct branch *branch = branch_get(branch_name);\n \t\tif (branch->merge_nr == 1 && branch->merge[0]->src) {\n \t\t\tstruct strbuf buf = STRBUF_INIT;\n \t\t\tstrbuf_addf(&buf, \"%s:%s\",\n \t\t\t\t    ref, branch->merge[0]->src);\n-\t\t\treturn strbuf_detach(&buf, NULL);\n+\t\t\trefspec_append(refspec, buf.buf);\n+\t\t\tstrbuf_release(&buf);\n+\t\t\treturn;\n \t\t}\n \t}\n\n-\treturn ref;\n+\trefspec_append(refspec, ref);\n }\n\n static void set_refspecs(const char **refs, int nr, const char *repo)\n {\n \tstruct remote *remote = NULL;\n \tstruct ref *local_refs = NULL;\n \tint i;\n\n \tfor (i = 0; i < nr; i++) {\n \t\tconst char *ref = refs[i];\n \t\tif (!strcmp(\"tag\", ref)) {\n \t\t\tstruct strbuf tagref = STRBUF_INIT;\n \t\t\tif (nr <= ++i)\n \t\t\t\tdie(_(\"tag shorthand without <tag>\"));\n \t\t\tref = refs[i];\n \t\t\tif (deleterefs)\n \t\t\t\tstrbuf_addf(&tagref, \":refs/tags/%s\", ref);\n \t\t\telse\n \t\t\t\tstrbuf_addf(&tagref, \"refs/tags/%s\", ref);\n-\t\t\tref = strbuf_detach(&tagref, NULL);\n+\t\t\trefspec_append(&rs, tagref.buf);\n+\t\t\tstrbuf_release(&tagref);\n \t\t} else if (deleterefs) {\n \t\t\tstruct strbuf delref = STRBUF_INIT;\n \t\t\tif (strchr(ref, ':'))\n \t\t\t\tdie(_(\"--delete only accepts plain target ref names\"));\n \t\t\tstrbuf_addf(&delref, \":%s\", ref);\n-\t\t\tref = strbuf_detach(&delref, NULL);\n+\t\t\trefspec_append(&rs, delref.buf);\n+\t\t\tstrbuf_release(&delref);\n \t\t} else if (!strchr(ref, ':')) {\n \t\t\tif (!remote) {\n \t\t\t\t/* lazily grab remote and local_refs */\n \t\t\t\tremote = remote_get(repo);\n \t\t\t\tlocal_refs = get_local_heads();\n \t\t\t}\n-\t\t\tref = map_refspec(ref, remote, local_refs);\n-\t\t}\n-\t\trefspec_append(&rs, ref);\n+\t\t\trefspec_append_mapped(&rs, ref, remote, local_refs);\n+\t\t} else\n+\t\t\trefspec_append(&rs, ref);\n \t}\n }\n\n@@ -192,45 +200,47 @@ static const char message_detached_head_die[] =\n static void setup_push_upstream(struct remote *remote, struct branch *branch,\n \t\t\t\tint triangular, int simple)\n {\n \tstruct strbuf refspec = STRBUF_INIT;\n\n \tif (!branch)\n \t\tdie(_(message_detached_head_die), remote->name);\n \tif (!branch->merge_nr || !branch->merge || !branch->remote_name)\n \t\tdie(_(\"The current branch %s has no upstream branch.\\n\"\n \t\t    \"To push the current branch and set the remote as upstream, use\\n\"\n \t\t    \"\\n\"\n \t\t    \"    git push --set-upstream %s %s\\n\"),\n \t\t    branch->name,\n \t\t    remote->name,\n \t\t    branch->name);\n \tif (branch->merge_nr != 1)\n \t\tdie(_(\"The current branch %s has multiple upstream branches, \"\n \t\t    \"refusing to push.\"), branch->name);\n \tif (triangular)\n \t\tdie(_(\"You are pushing to remote '%s', which is not the upstream of\\n\"\n \t\t      \"your current branch '%s', without telling me what to push\\n\"\n \t\t      \"to update which remote branch.\"),\n \t\t    remote->name, branch->name);\n\n \tif (simple) {\n \t\t/* Additional safety */\n \t\tif (strcmp(branch->refname, branch->merge[0]->src))\n \t\t\tdie_push_simple(branch, remote);\n \t}\n\n \tstrbuf_addf(&refspec, \"%s:%s\", branch->refname, branch->merge[0]->src);\n \trefspec_append(&rs, refspec.buf);\n+\tstrbuf_release(&refspec);\n }\n\n static void setup_push_current(struct remote *remote, struct branch *branch)\n {\n \tstruct strbuf refspec = STRBUF_INIT;\n\n \tif (!branch)\n \t\tdie(_(message_detached_head_die), remote->name);\n \tstrbuf_addf(&refspec, \"%s:%s\", branch->refname, branch->refname);\n \trefspec_append(&rs, refspec.buf);\n+\tstrbuf_release(&refspec);\n }\n\n static int is_workflow_triangular(struct remote *remote)\n--\n2.28.0\n"},{"id":"405062","messageId":"e49140b8-25d6-0e5b-b1da-468a40c4a687@web.de","threadId":"54191","inReplyTo":"e0c7b847-b8c7-6880-748e-1e5b32934ed5@web.de","subject":"[PATCH 2/2] refspec: add and use refspec_appendf()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-09-05T14:49:30Z","receivedAt":"2020-09-05T14:50:07Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a function for building a refspec using printf-style formatting.  It\nfrees callers from managing their own buffer.  Use it throughout the\ntree to shorten and simplify its callers.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/clone.c |  7 ++-----\n builtin/fetch.c |  7 ++-----\n builtin/push.c  | 40 ++++++++++------------------------------\n refspec.c       | 18 ++++++++++++++++--\n refspec.h       |  2 ++\n remote.c        | 10 +++-------\n 6 files changed, 35 insertions(+), 49 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex b087ee40c2..fbfd6568cd 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -953,7 +953,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstruct ref *mapped_refs;\n \tconst struct ref *ref;\n \tstruct strbuf key = STRBUF_INIT;\n-\tstruct strbuf default_refspec = STRBUF_INIT;\n \tstruct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;\n \tstruct transport *transport = NULL;\n \tconst char *src_ref_prefix = \"refs/heads/\";\n@@ -1157,9 +1156,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n\n \tremote = remote_get(option_origin);\n\n-\tstrbuf_addf(&default_refspec, \"+%s*:%s*\", src_ref_prefix,\n-\t\t    branch_top.buf);\n-\trefspec_append(&remote->fetch, default_refspec.buf);\n+\trefspec_appendf(&remote->fetch, \"+%s*:%s*\", src_ref_prefix,\n+\t\t\tbranch_top.buf);\n\n \ttransport = transport_get(remote, remote->url[0]);\n \ttransport_set_verbosity(transport, option_verbosity, option_progress);\n@@ -1332,7 +1330,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&reflog_msg);\n \tstrbuf_release(&branch_top);\n \tstrbuf_release(&key);\n-\tstrbuf_release(&default_refspec);\n \tjunk_mode = JUNK_LEAVE_ALL;\n\n \tstrvec_clear(&ref_prefixes);\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a6d3268661..c555836937 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1738,15 +1738,12 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,\n\n \tfor (i = 0; i < argc; i++) {\n \t\tif (!strcmp(argv[i], \"tag\")) {\n-\t\t\tchar *tag;\n \t\t\ti++;\n \t\t\tif (i >= argc)\n \t\t\t\tdie(_(\"You need to specify a tag name.\"));\n\n-\t\t\ttag = xstrfmt(\"refs/tags/%s:refs/tags/%s\",\n-\t\t\t\t      argv[i], argv[i]);\n-\t\t\trefspec_append(&rs, tag);\n-\t\t\tfree(tag);\n+\t\t\trefspec_appendf(&rs, \"refs/tags/%s:refs/tags/%s\",\n+\t\t\t\t\targv[i], argv[i]);\n \t\t} else {\n \t\t\trefspec_append(&rs, argv[i]);\n \t\t}\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 0f3c108c93..0eeb2c8dd5 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -78,12 +78,9 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,\n \t\tmemset(&query, 0, sizeof(struct refspec_item));\n \t\tquery.src = matched->name;\n \t\tif (!query_refspecs(&remote->push, &query) && query.dst) {\n-\t\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\t\tstrbuf_addf(&buf, \"%s%s:%s\",\n-\t\t\t\t    query.force ? \"+\" : \"\",\n-\t\t\t\t    query.src, query.dst);\n-\t\t\trefspec_append(refspec, buf.buf);\n-\t\t\tstrbuf_release(&buf);\n+\t\t\trefspec_appendf(refspec, \"%s%s:%s\",\n+\t\t\t\t\tquery.force ? \"+\" : \"\",\n+\t\t\t\t\tquery.src, query.dst);\n \t\t\treturn;\n \t\t}\n \t}\n@@ -92,11 +89,8 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,\n \t    skip_prefix(matched->name, \"refs/heads/\", &branch_name)) {\n \t\tstruct branch *branch = branch_get(branch_name);\n \t\tif (branch->merge_nr == 1 && branch->merge[0]->src) {\n-\t\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\t\tstrbuf_addf(&buf, \"%s:%s\",\n-\t\t\t\t    ref, branch->merge[0]->src);\n-\t\t\trefspec_append(refspec, buf.buf);\n-\t\t\tstrbuf_release(&buf);\n+\t\t\trefspec_appendf(refspec, \"%s:%s\",\n+\t\t\t\t\tref, branch->merge[0]->src);\n \t\t\treturn;\n \t\t}\n \t}\n@@ -113,23 +107,17 @@ static void set_refspecs(const char **refs, int nr, const char *repo)\n \tfor (i = 0; i < nr; i++) {\n \t\tconst char *ref = refs[i];\n \t\tif (!strcmp(\"tag\", ref)) {\n-\t\t\tstruct strbuf tagref = STRBUF_INIT;\n \t\t\tif (nr <= ++i)\n \t\t\t\tdie(_(\"tag shorthand without <tag>\"));\n \t\t\tref = refs[i];\n \t\t\tif (deleterefs)\n-\t\t\t\tstrbuf_addf(&tagref, \":refs/tags/%s\", ref);\n+\t\t\t\trefspec_appendf(&rs, \":refs/tags/%s\", ref);\n \t\t\telse\n-\t\t\t\tstrbuf_addf(&tagref, \"refs/tags/%s\", ref);\n-\t\t\trefspec_append(&rs, tagref.buf);\n-\t\t\tstrbuf_release(&tagref);\n+\t\t\t\trefspec_appendf(&rs, \"refs/tags/%s\", ref);\n \t\t} else if (deleterefs) {\n-\t\t\tstruct strbuf delref = STRBUF_INIT;\n \t\t\tif (strchr(ref, ':'))\n \t\t\t\tdie(_(\"--delete only accepts plain target ref names\"));\n-\t\t\tstrbuf_addf(&delref, \":%s\", ref);\n-\t\t\trefspec_append(&rs, delref.buf);\n-\t\t\tstrbuf_release(&delref);\n+\t\t\trefspec_appendf(&rs, \":%s\", ref);\n \t\t} else if (!strchr(ref, ':')) {\n \t\t\tif (!remote) {\n \t\t\t\t/* lazily grab remote and local_refs */\n@@ -200,8 +188,6 @@ static const char message_detached_head_die[] =\n static void setup_push_upstream(struct remote *remote, struct branch *branch,\n \t\t\t\tint triangular, int simple)\n {\n-\tstruct strbuf refspec = STRBUF_INIT;\n-\n \tif (!branch)\n \t\tdie(_(message_detached_head_die), remote->name);\n \tif (!branch->merge_nr || !branch->merge || !branch->remote_name)\n@@ -227,20 +213,14 @@ static void setup_push_upstream(struct remote *remote, struct branch *branch,\n \t\t\tdie_push_simple(branch, remote);\n \t}\n\n-\tstrbuf_addf(&refspec, \"%s:%s\", branch->refname, branch->merge[0]->src);\n-\trefspec_append(&rs, refspec.buf);\n-\tstrbuf_release(&refspec);\n+\trefspec_appendf(&rs, \"%s:%s\", branch->refname, branch->merge[0]->src);\n }\n\n static void setup_push_current(struct remote *remote, struct branch *branch)\n {\n-\tstruct strbuf refspec = STRBUF_INIT;\n-\n \tif (!branch)\n \t\tdie(_(message_detached_head_die), remote->name);\n-\tstrbuf_addf(&refspec, \"%s:%s\", branch->refname, branch->refname);\n-\trefspec_append(&rs, refspec.buf);\n-\tstrbuf_release(&refspec);\n+\trefspec_appendf(&rs, \"%s:%s\", branch->refname, branch->refname);\n }\n\n static int is_workflow_triangular(struct remote *remote)\ndiff --git a/refspec.c b/refspec.c\nindex f10ef284ce..8d0affc34a 100644\n--- a/refspec.c\n+++ b/refspec.c\n@@ -153,7 +153,7 @@ void refspec_init(struct refspec *rs, int fetch)\n \trs->fetch = fetch;\n }\n\n-void refspec_append(struct refspec *rs, const char *refspec)\n+static void refspec_append_nodup(struct refspec *rs, char *refspec)\n {\n \tstruct refspec_item item;\n\n@@ -163,7 +163,21 @@ void refspec_append(struct refspec *rs, const char *refspec)\n \trs->items[rs->nr++] = item;\n\n \tALLOC_GROW(rs->raw, rs->raw_nr + 1, rs->raw_alloc);\n-\trs->raw[rs->raw_nr++] = xstrdup(refspec);\n+\trs->raw[rs->raw_nr++] = refspec;\n+}\n+\n+void refspec_append(struct refspec *rs, const char *refspec)\n+{\n+\trefspec_append_nodup(rs, xstrdup(refspec));\n+}\n+\n+void refspec_appendf(struct refspec *rs, const char *fmt, ...)\n+{\n+\tva_list ap;\n+\n+\tva_start(ap, fmt);\n+\trefspec_append_nodup(rs, xstrvfmt(fmt, ap));\n+\tva_end(ap);\n }\n\n void refspec_appendn(struct refspec *rs, const char **refspecs, int nr)\ndiff --git a/refspec.h b/refspec.h\nindex 8d654e3a3a..7569248d11 100644\n--- a/refspec.h\n+++ b/refspec.h\n@@ -56,6 +56,8 @@ void refspec_item_init_or_die(struct refspec_item *item, const char *refspec,\n void refspec_item_clear(struct refspec_item *item);\n void refspec_init(struct refspec *rs, int fetch);\n void refspec_append(struct refspec *rs, const char *refspec);\n+__attribute__((format (printf,2,3)))\n+void refspec_appendf(struct refspec *rs, const char *fmt, ...);\n void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);\n void refspec_clear(struct refspec *rs);\n\ndiff --git a/remote.c b/remote.c\nindex c5ed74f91c..5c04275342 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -287,19 +287,15 @@ static void read_branches_file(struct remote *remote)\n \t\tfrag = (char *)git_default_branch_name();\n\n \tadd_url_alias(remote, strbuf_detach(&buf, NULL));\n-\tstrbuf_addf(&buf, \"refs/heads/%s:refs/heads/%s\",\n-\t\t    frag, remote->name);\n-\trefspec_append(&remote->fetch, buf.buf);\n+\trefspec_appendf(&remote->fetch, \"refs/heads/%s:refs/heads/%s\",\n+\t\t\tfrag, remote->name);\n\n \t/*\n \t * Cogito compatible push: push current HEAD to remote #branch\n \t * (master if missing)\n \t */\n-\tstrbuf_reset(&buf);\n-\tstrbuf_addf(&buf, \"HEAD:refs/heads/%s\", frag);\n-\trefspec_append(&remote->push, buf.buf);\n+\trefspec_appendf(&remote->push, \"HEAD:refs/heads/%s\", frag);\n \tremote->fetch_tags = 1; /* always auto-follow */\n-\tstrbuf_release(&buf);\n }\n\n static int handle_config(const char *key, const char *value, void *cb)\n--\n2.28.0\n"}]}