threads / patch / 45815

patch, 3 partsfix memory leak in git-am

Subject: [PATCH 0/3] fix memory leak in git-am

## tl;dr

5 messages between Apr 27, 2017 and Apr 27, 2017. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Jeff King· Apr 27, 2017, 03:25 UTC · lore

This was noticed by Coverity. The leak isn't new, but I think was "re-found" by Coverity because some nearby code did an unrelated s/sha1/oid/ change, throwing off its heuristics.

I also checked whether this was in Dscho's big pack of Coverity fixups from earlier today, and it's not.

The first one is the actual fix. The second one is pure cleanup, but I think worth doing. The final one is arguably just churn, and maybe people even like the end result less. I'm OK to drop it.

  [1/3]: am: fix commit buffer leak in get_commit_info()
  [2/3]: am: simplify allocations in get_commit_info()
  [3/3]: am: shorten ident_split variable name in get_commit_info()
 builtin/am.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)
-Peff
Jeff King· Apr 27, 2017, 03:25 UTC · re: Jeff King · lore

[PATCH 1/3] am: fix commit buffer leak in get_commit_info()

Calling logmsg_reencode() may allocate a buffer for the commit message (because we need to load it from disk, or because it needs re-encoded). We must "unuse" it afterwards to free it.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/am.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to builtin/am.c +1 −0
diff --git a/builtin/am.c b/builtin/am.c
index 805f56cec..baab951ed 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1421,6 +1421,7 @@ static void get_commit_info(struct am_state *state, struct commit *commit)
 		die(_("unable to parse commit %s"), oid_to_hex(&commit->object.oid));
 	state->msg = xstrdup(msg + 2);
 	state->msg_len = strlen(state->msg);
+	unuse_commit_buffer(commit, buffer);
 }
 
 /**
-- 
2.13.0.rc0.465.ga1e654229
Jeff King· Apr 27, 2017, 03:27 UTC · re: Jeff King · lore

[PATCH 2/3] am: simplify allocations in get_commit_info()

After we call split_ident_line(), we have several begin/end pairs for various parts of the ident. We then copy each into a strbuf to create a single string, and then detach that string. We can instead skip the strbuf entirely and just duplicate the strings directly.

This is shorter, and it makes it more obvious that we are not leaking the strbuf (we were not before, because every code path either died or hit a strbuf_detach).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/am.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)
Show changes to builtin/am.c +10 −15
diff --git a/builtin/am.c b/builtin/am.c
index baab951ed..e08f03326 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1380,40 +1380,35 @@ static int get_mail_commit_oid(struct object_id *commit_id, const char *mail)
  */
 static void get_commit_info(struct am_state *state, struct commit *commit)
 {
-	const char *buffer, *ident_line, *author_date, *msg;
+	const char *buffer, *ident_line, *msg;
 	size_t ident_len;
 	struct ident_split ident_split;
-	struct strbuf sb = STRBUF_INIT;
 
 	buffer = logmsg_reencode(commit, NULL, get_commit_output_encoding());
 
 	ident_line = find_commit_header(buffer, "author", &ident_len);
 
-	if (split_ident_line(&ident_split, ident_line, ident_len) < 0) {
-		strbuf_add(&sb, ident_line, ident_len);
-		die(_("invalid ident line: %s"), sb.buf);
-	}
+	if (split_ident_line(&ident_split, ident_line, ident_len) < 0)
+		die(_("invalid ident line: %.*s"), (int)ident_len, ident_line);
 
 	assert(!state->author_name);
 	if (ident_split.name_begin) {
-		strbuf_add(&sb, ident_split.name_begin,
-			ident_split.name_end - ident_split.name_begin);
-		state->author_name = strbuf_detach(&sb, NULL);
+		state->author_name =
+			xmemdupz(ident_split.name_begin,
+				 ident_split.name_end - ident_split.name_begin);
 	} else
 		state->author_name = xstrdup("");
 
 	assert(!state->author_email);
 	if (ident_split.mail_begin) {
-		strbuf_add(&sb, ident_split.mail_begin,
-			ident_split.mail_end - ident_split.mail_begin);
-		state->author_email = strbuf_detach(&sb, NULL);
+		state->author_email =
+			xmemdupz(ident_split.mail_begin,
+				 ident_split.mail_end - ident_split.mail_begin);
 	} else
 		state->author_email = xstrdup("");
 
-	author_date = show_ident_date(&ident_split, DATE_MODE(NORMAL));
-	strbuf_addstr(&sb, author_date);
 	assert(!state->author_date);
-	state->author_date = strbuf_detach(&sb, NULL);
+	state->author_date = xstrdup(show_ident_date(&ident_split, DATE_MODE(NORMAL)));
 
 	assert(!state->msg);
 	msg = strstr(buffer, "\n\n");
-- 
2.13.0.rc0.465.ga1e654229
Jeff King· Apr 27, 2017, 03:28 UTC · re: Jeff King · lore

[PATCH 3/3] am: shorten ident_split variable name in get_commit_info()

The local ident_split variable is often mentioned three times per line when dealing with its begin/end pointer pairs. Let's use a shorter name which lets us get rid of some long lines. Since this is a short self-contained function, readability doesn't suffer.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/am.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)
Show changes to builtin/am.c +9 −11
diff --git a/builtin/am.c b/builtin/am.c
index e08f03326..2aed3ef01 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1382,33 +1382,31 @@ static void get_commit_info(struct am_state *state, struct commit *commit)
 {
 	const char *buffer, *ident_line, *msg;
 	size_t ident_len;
-	struct ident_split ident_split;
+	struct ident_split id;
 
 	buffer = logmsg_reencode(commit, NULL, get_commit_output_encoding());
 
 	ident_line = find_commit_header(buffer, "author", &ident_len);
 
-	if (split_ident_line(&ident_split, ident_line, ident_len) < 0)
+	if (split_ident_line(&id, ident_line, ident_len) < 0)
 		die(_("invalid ident line: %.*s"), (int)ident_len, ident_line);
 
 	assert(!state->author_name);
-	if (ident_split.name_begin) {
+	if (id.name_begin)
 		state->author_name =
-			xmemdupz(ident_split.name_begin,
-				 ident_split.name_end - ident_split.name_begin);
-	} else
+			xmemdupz(id.name_begin, id.name_end - id.name_begin);
+	else
 		state->author_name = xstrdup("");
 
 	assert(!state->author_email);
-	if (ident_split.mail_begin) {
+	if (id.mail_begin)
 		state->author_email =
-			xmemdupz(ident_split.mail_begin,
-				 ident_split.mail_end - ident_split.mail_begin);
-	} else
+			xmemdupz(id.mail_begin, id.mail_end - id.mail_begin);
+	else
 		state->author_email = xstrdup("");
 
 	assert(!state->author_date);
-	state->author_date = xstrdup(show_ident_date(&ident_split, DATE_MODE(NORMAL)));
+	state->author_date = xstrdup(show_ident_date(&id, DATE_MODE(NORMAL)));
 
 	assert(!state->msg);
 	msg = strstr(buffer, "\n\n");
-- 
2.13.0.rc0.465.ga1e654229
Junio C Hamano· Apr 27, 2017, 05:41 UTC · re: Jeff King · lore

Re: [PATCH 0/3] fix memory leak in git-am

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> This was noticed by Coverity. The leak isn't new, but I think was
> "re-found" by Coverity because some nearby code did an unrelated
> s/sha1/oid/ change, throwing off its heuristics.
>
> I also checked whether this was in Dscho's big pack of Coverity fixups
> from earlier today, and it's not.
>
> The first one is the actual fix. The second one is pure cleanup, but I
> think worth doing. The final one is arguably just churn, and maybe
> people even like the end result less. I'm OK to drop it.
>
>   [1/3]: am: fix commit buffer leak in get_commit_info()
>   [2/3]: am: simplify allocations in get_commit_info()
>   [3/3]: am: shorten ident_split variable name in get_commit_info()
All looked sensible.  Thanks.

← back to recent threads