From 30035d9c66bc2a52352e3ad42b56047f06c20326 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sat, 5 Sep 2020 16:47:47 +0200 Subject: [PATCH 1/2] push: release strbufs used for refspec formatting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit map_refspec() either returns the passed in ref string or a detached strbuf. This makes it hard for callers to release the possibly allocated memory, and set_refspecs() consequently leaks it. Let map_refspec() append any refspecs directly and release its own strbufs after use. Rename it to refspec_append_mapped() and don't return anything to reflect its increased responsibility. set_refspecs() also leaks its strbufs. Do the same here and directly call refspec_append() in each if branch instead of holding onto a detached strbuf, then dispose of the allocated memory after use. We need to add an else branch for the final call because all the other conditional branches already add their formatted refspec now. setup_push_upstream() and setup_push_current() forgot to release their strbufs as well; plug these leaks, too, while at it. None of these leaks were likely to impact users, because the number and sizes of refspecs are usually small and the allocations are only done once per program run. Clean them up nevertheless, as another step on the long road towards zero memory leaks. Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano --- builtin/push.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index bc94078e72..0f3c108c93 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -61,15 +61,17 @@ static struct refspec rs = REFSPEC_INIT_PUSH; static struct string_list push_options_config = STRING_LIST_INIT_DUP; -static const char *map_refspec(const char *ref, - struct remote *remote, struct ref *local_refs) +static void refspec_append_mapped(struct refspec *refspec, const char *ref, + struct remote *remote, struct ref *local_refs) { const char *branch_name; struct ref *matched = NULL; /* Does "ref" uniquely name our ref? */ - if (count_refspec_match(ref, local_refs, &matched) != 1) - return ref; + if (count_refspec_match(ref, local_refs, &matched) != 1) { + refspec_append(refspec, ref); + return; + } if (remote->push.nr) { struct refspec_item query; @@ -80,7 +82,9 @@ static const char *map_refspec(const char *ref, strbuf_addf(&buf, "%s%s:%s", query.force ? "+" : "", query.src, query.dst); - return strbuf_detach(&buf, NULL); + refspec_append(refspec, buf.buf); + strbuf_release(&buf); + return; } } @@ -91,11 +95,13 @@ static const char *map_refspec(const char *ref, struct strbuf buf = STRBUF_INIT; strbuf_addf(&buf, "%s:%s", ref, branch->merge[0]->src); - return strbuf_detach(&buf, NULL); + refspec_append(refspec, buf.buf); + strbuf_release(&buf); + return; } } - return ref; + refspec_append(refspec, ref); } static void set_refspecs(const char **refs, int nr, const char *repo) @@ -115,22 +121,24 @@ static void set_refspecs(const char **refs, int nr, const char *repo) strbuf_addf(&tagref, ":refs/tags/%s", ref); else strbuf_addf(&tagref, "refs/tags/%s", ref); - ref = strbuf_detach(&tagref, NULL); + refspec_append(&rs, tagref.buf); + strbuf_release(&tagref); } else if (deleterefs) { struct strbuf delref = STRBUF_INIT; if (strchr(ref, ':')) die(_("--delete only accepts plain target ref names")); strbuf_addf(&delref, ":%s", ref); - ref = strbuf_detach(&delref, NULL); + refspec_append(&rs, delref.buf); + strbuf_release(&delref); } else if (!strchr(ref, ':')) { if (!remote) { /* lazily grab remote and local_refs */ remote = remote_get(repo); local_refs = get_local_heads(); } - ref = map_refspec(ref, remote, local_refs); - } - refspec_append(&rs, ref); + refspec_append_mapped(&rs, ref, remote, local_refs); + } else + refspec_append(&rs, ref); } } @@ -221,6 +229,7 @@ static void setup_push_upstream(struct remote *remote, struct branch *branch, strbuf_addf(&refspec, "%s:%s", branch->refname, branch->merge[0]->src); refspec_append(&rs, refspec.buf); + strbuf_release(&refspec); } static void setup_push_current(struct remote *remote, struct branch *branch) @@ -231,6 +240,7 @@ static void setup_push_current(struct remote *remote, struct branch *branch) die(_(message_detached_head_die), remote->name); strbuf_addf(&refspec, "%s:%s", branch->refname, branch->refname); refspec_append(&rs, refspec.buf); + strbuf_release(&refspec); } static int is_workflow_triangular(struct remote *remote) From 1af8b8c0a570ee0b12a19fdd920a3ea09fb22a75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sat, 5 Sep 2020 16:49:30 +0200 Subject: [PATCH 2/2] refspec: add and use refspec_appendf() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a function for building a refspec using printf-style formatting. It frees callers from managing their own buffer. Use it throughout the tree to shorten and simplify its callers. Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano --- builtin/clone.c | 7 ++----- builtin/fetch.c | 7 ++----- builtin/push.c | 40 ++++++++++------------------------------ refspec.c | 18 ++++++++++++++++-- refspec.h | 2 ++ remote.c | 10 +++------- 6 files changed, 35 insertions(+), 49 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index b087ee40c2..fbfd6568cd 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -953,7 +953,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix) struct ref *mapped_refs; const struct ref *ref; struct strbuf key = STRBUF_INIT; - struct strbuf default_refspec = STRBUF_INIT; struct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT; struct transport *transport = NULL; const char *src_ref_prefix = "refs/heads/"; @@ -1157,9 +1156,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix) remote = remote_get(option_origin); - strbuf_addf(&default_refspec, "+%s*:%s*", src_ref_prefix, - branch_top.buf); - refspec_append(&remote->fetch, default_refspec.buf); + refspec_appendf(&remote->fetch, "+%s*:%s*", src_ref_prefix, + branch_top.buf); transport = transport_get(remote, remote->url[0]); transport_set_verbosity(transport, option_verbosity, option_progress); @@ -1332,7 +1330,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix) strbuf_release(&reflog_msg); strbuf_release(&branch_top); strbuf_release(&key); - strbuf_release(&default_refspec); junk_mode = JUNK_LEAVE_ALL; strvec_clear(&ref_prefixes); diff --git a/builtin/fetch.c b/builtin/fetch.c index a6d3268661..c555836937 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1738,15 +1738,12 @@ static int fetch_one(struct remote *remote, int argc, const char **argv, for (i = 0; i < argc; i++) { if (!strcmp(argv[i], "tag")) { - char *tag; i++; if (i >= argc) die(_("You need to specify a tag name.")); - tag = xstrfmt("refs/tags/%s:refs/tags/%s", - argv[i], argv[i]); - refspec_append(&rs, tag); - free(tag); + refspec_appendf(&rs, "refs/tags/%s:refs/tags/%s", + argv[i], argv[i]); } else { refspec_append(&rs, argv[i]); } diff --git a/builtin/push.c b/builtin/push.c index 0f3c108c93..0eeb2c8dd5 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -78,12 +78,9 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref, memset(&query, 0, sizeof(struct refspec_item)); query.src = matched->name; if (!query_refspecs(&remote->push, &query) && query.dst) { - struct strbuf buf = STRBUF_INIT; - strbuf_addf(&buf, "%s%s:%s", - query.force ? "+" : "", - query.src, query.dst); - refspec_append(refspec, buf.buf); - strbuf_release(&buf); + refspec_appendf(refspec, "%s%s:%s", + query.force ? "+" : "", + query.src, query.dst); return; } } @@ -92,11 +89,8 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref, skip_prefix(matched->name, "refs/heads/", &branch_name)) { struct branch *branch = branch_get(branch_name); if (branch->merge_nr == 1 && branch->merge[0]->src) { - struct strbuf buf = STRBUF_INIT; - strbuf_addf(&buf, "%s:%s", - ref, branch->merge[0]->src); - refspec_append(refspec, buf.buf); - strbuf_release(&buf); + refspec_appendf(refspec, "%s:%s", + ref, branch->merge[0]->src); return; } } @@ -113,23 +107,17 @@ static void set_refspecs(const char **refs, int nr, const char *repo) for (i = 0; i < nr; i++) { const char *ref = refs[i]; if (!strcmp("tag", ref)) { - struct strbuf tagref = STRBUF_INIT; if (nr <= ++i) die(_("tag shorthand without ")); ref = refs[i]; if (deleterefs) - strbuf_addf(&tagref, ":refs/tags/%s", ref); + refspec_appendf(&rs, ":refs/tags/%s", ref); else - strbuf_addf(&tagref, "refs/tags/%s", ref); - refspec_append(&rs, tagref.buf); - strbuf_release(&tagref); + refspec_appendf(&rs, "refs/tags/%s", ref); } else if (deleterefs) { - struct strbuf delref = STRBUF_INIT; if (strchr(ref, ':')) die(_("--delete only accepts plain target ref names")); - strbuf_addf(&delref, ":%s", ref); - refspec_append(&rs, delref.buf); - strbuf_release(&delref); + refspec_appendf(&rs, ":%s", ref); } else if (!strchr(ref, ':')) { if (!remote) { /* lazily grab remote and local_refs */ @@ -200,8 +188,6 @@ static const char message_detached_head_die[] = static void setup_push_upstream(struct remote *remote, struct branch *branch, int triangular, int simple) { - struct strbuf refspec = STRBUF_INIT; - if (!branch) die(_(message_detached_head_die), remote->name); if (!branch->merge_nr || !branch->merge || !branch->remote_name) @@ -227,20 +213,14 @@ static void setup_push_upstream(struct remote *remote, struct branch *branch, die_push_simple(branch, remote); } - strbuf_addf(&refspec, "%s:%s", branch->refname, branch->merge[0]->src); - refspec_append(&rs, refspec.buf); - strbuf_release(&refspec); + refspec_appendf(&rs, "%s:%s", branch->refname, branch->merge[0]->src); } static void setup_push_current(struct remote *remote, struct branch *branch) { - struct strbuf refspec = STRBUF_INIT; - if (!branch) die(_(message_detached_head_die), remote->name); - strbuf_addf(&refspec, "%s:%s", branch->refname, branch->refname); - refspec_append(&rs, refspec.buf); - strbuf_release(&refspec); + refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname); } static int is_workflow_triangular(struct remote *remote) diff --git a/refspec.c b/refspec.c index f10ef284ce..8d0affc34a 100644 --- a/refspec.c +++ b/refspec.c @@ -153,7 +153,7 @@ void refspec_init(struct refspec *rs, int fetch) rs->fetch = fetch; } -void refspec_append(struct refspec *rs, const char *refspec) +static void refspec_append_nodup(struct refspec *rs, char *refspec) { struct refspec_item item; @@ -163,7 +163,21 @@ void refspec_append(struct refspec *rs, const char *refspec) rs->items[rs->nr++] = item; ALLOC_GROW(rs->raw, rs->raw_nr + 1, rs->raw_alloc); - rs->raw[rs->raw_nr++] = xstrdup(refspec); + rs->raw[rs->raw_nr++] = refspec; +} + +void refspec_append(struct refspec *rs, const char *refspec) +{ + refspec_append_nodup(rs, xstrdup(refspec)); +} + +void refspec_appendf(struct refspec *rs, const char *fmt, ...) +{ + va_list ap; + + va_start(ap, fmt); + refspec_append_nodup(rs, xstrvfmt(fmt, ap)); + va_end(ap); } void refspec_appendn(struct refspec *rs, const char **refspecs, int nr) diff --git a/refspec.h b/refspec.h index 8d654e3a3a..7569248d11 100644 --- a/refspec.h +++ b/refspec.h @@ -56,6 +56,8 @@ void refspec_item_init_or_die(struct refspec_item *item, const char *refspec, void refspec_item_clear(struct refspec_item *item); void refspec_init(struct refspec *rs, int fetch); void refspec_append(struct refspec *rs, const char *refspec); +__attribute__((format (printf,2,3))) +void refspec_appendf(struct refspec *rs, const char *fmt, ...); void refspec_appendn(struct refspec *rs, const char **refspecs, int nr); void refspec_clear(struct refspec *rs); diff --git a/remote.c b/remote.c index c5ed74f91c..5c04275342 100644 --- a/remote.c +++ b/remote.c @@ -287,19 +287,15 @@ static void read_branches_file(struct remote *remote) frag = (char *)git_default_branch_name(); add_url_alias(remote, strbuf_detach(&buf, NULL)); - strbuf_addf(&buf, "refs/heads/%s:refs/heads/%s", - frag, remote->name); - refspec_append(&remote->fetch, buf.buf); + refspec_appendf(&remote->fetch, "refs/heads/%s:refs/heads/%s", + frag, remote->name); /* * Cogito compatible push: push current HEAD to remote #branch * (master if missing) */ - strbuf_reset(&buf); - strbuf_addf(&buf, "HEAD:refs/heads/%s", frag); - refspec_append(&remote->push, buf.buf); + refspec_appendf(&remote->push, "HEAD:refs/heads/%s", frag); remote->fetch_tags = 1; /* always auto-follow */ - strbuf_release(&buf); } static int handle_config(const char *key, const char *value, void *cb)