[PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()

Use string_list_append_nodup() instead of string_list_append() to hand
over ownership of a detached strbuf and thus avoid leaking its memory.

Signed-off-by: Rene Scharfe <redacted>
---
 builtin/fmt-merge-msg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 22034f87e7..8e8a15ea4a 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c

@@ -377,7 +377,8 @@ static void shortlog(const char *name,
 			string_list_append(&subjects,
 					   oid_to_hex(&commit->object.oid));
 		else
-			string_list_append(&subjects, strbuf_detach(&sb, NULL));
+			string_list_append_nodup(&subjects,
+						 strbuf_detach(&sb, NULL));
 	}
 
 	if (opts->credit_people)

-- 
2.15.1

On Thu, Dec 07, 2017 at 09:22:49PM +0100, René Scharfe wrote:

Use string_list_append_nodup() instead of string_list_append() to hand
over ownership of a detached strbuf and thus avoid leaking its memory.

Looks obviously correct (though one thing missing from the diff context
is whether "subjects" is set to DUP -- it is, which is good).

Grepping for "list_append.*detach" shows a few other possible cases in
transport-helper.c, which I think are leaks.

I wondered if it would be possible to write a coccinelle rule for this,
but I think it's not possible. Whether this is right depends on the
strdup_strings flag, which could change at runtime (though in practice
it doesn't).

-Peff

Am 07.12.2017 um 22:27 schrieb Jeff King:

Grepping for "list_append.*detach" shows a few other possible cases in
transport-helper.c, which I think are leaks.

-- >8 --
Subject: [PATCH] transport-helper: plug strbuf and string_list leaks

Transfer ownership of detached strbufs to string_lists of the
duplicating variety by calling string_list_append_nodup() instead of
string_list_append() to avoid duplicating and then leaking the buffer.

While at it make sure to release the string_list when done;
push_refs_with_export() already does that.

Reported-by: Jeff King <redacted>
Signed-off-by: Rene Scharfe <redacted>
---
 transport-helper.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index bf05a2dcf1..f682e7c534 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -882,7 +882,8 @@ static int push_refs_with_push(struct transport *transport,
 			struct strbuf cas = STRBUF_INIT;
 			strbuf_addf(&cas, "%s:%s",
 				    ref->name, oid_to_hex(&ref->old_oid_expect));
-			string_list_append(&cas_options, strbuf_detach(&cas, NULL));
+			string_list_append_nodup(&cas_options,
+						 strbuf_detach(&cas, NULL));
 		}
 	}
 	if (buf.len == 0) {

@@ -897,6 +898,7 @@ static int push_refs_with_push(struct transport *transport,
 	strbuf_addch(&buf, '\n');
 	sendline(data, &buf);
 	strbuf_release(&buf);
+	string_list_release(&cas_options, 0);
 
 	return push_update_refs_status(data, remote_refs, flags);
 }

@@ -930,7 +932,8 @@ static int push_refs_with_export(struct transport *transport,
 		private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);
 		if (private && !get_oid(private, &oid)) {
 			strbuf_addf(&buf, "^%s", private);
-			string_list_append(&revlist_args, strbuf_detach(&buf, NULL));
+			string_list_append_nodup(&revlist_args,
+						 strbuf_detach(&buf, NULL));
 			oidcpy(&ref->old_oid, &oid);
 		}
 		free(private);

-- 
2.15.1

On Fri, Dec 08, 2017 at 06:29:31PM +0100, René Scharfe wrote:

Am 07.12.2017 um 22:27 schrieb Jeff King:

quoted

Grepping for "list_append.*detach" shows a few other possible cases in
transport-helper.c, which I think are leaks.

-- >8 --
Subject: [PATCH] transport-helper: plug strbuf and string_list leaks

Transfer ownership of detached strbufs to string_lists of the
duplicating variety by calling string_list_append_nodup() instead of
string_list_append() to avoid duplicating and then leaking the buffer.

Thanks, this part looks obviously correct.

While at it make sure to release the string_list when done;
push_refs_with_export() already does that.

This one takes a bit more digging. I've been bitten before in Git's code
by freeing what appeared to be a leak only to find out that we had
passed the pointers off to some other data structure which expected them
to persist.

Here we feed them to set_helper_option(), which passes them to
quote_c_style(), which makes a copy into a strbuf. So I think all is
well.

-Peff