[PATCH 00/34] plug strbuf memory leaks

[PATCH 00/34] plug strbuf memory leaks

[Rene Scharfe]

Release allocated strbufs in functions that are at least potentionally
library-like; cmd_*() functions are out of scope because the process
ends with them and the OS cleans up for us anyway.  The patches are
split by function and were generated with --function-context for easier
reviewing.

  am: release strbufs after use in detect_patch_format()
  am: release strbuf on error return in hg_patch_to_mail()
  am: release strbuf after use in safe_to_abort()
  check-ref-format: release strbuf after use in check_ref_format_branch()
  clean: release strbuf after use in remove_dirs()
  clone: release strbuf after use in remove_junk()
  commit: release strbuf on error return in commit_tree_extended()
  connect: release strbuf on error return in git_connect()
  convert: release strbuf on error return in filter_buffer_or_fd()
  diff: release strbuf after use in diff_summary()
  diff: release strbuf after use in show_rename_copy()
  diff: release strbuf after use in show_stats()
  help: release strbuf on error return in exec_man_konqueror()
  help: release strbuf on error return in exec_man_man()
  help: release strbuf on error return in exec_woman_emacs()
  mailinfo: release strbuf after use in handle_from()
  mailinfo: release strbuf on error return in handle_boundary()
  merge: release strbuf after use in save_state()
  merge: release strbuf after use in write_merge_heads()
  notes: release strbuf after use in notes_copy_from_stdin()
  refs: release strbuf on error return in write_pseudoref()
  remote: release strbuf after use in read_remote_branches()
  remote: release strbuf after use in migrate_file()
  remote: release strbuf after use in set_url()
  send-pack: release strbuf on error return in send_pack()
  sha1_file: release strbuf on error return in index_path()
  shortlog: release strbuf after use in insert_one_record()
  sequencer: release strbuf after use in save_head()
  transport-helper: release strbuf after use in process_connect_service()
  userdiff: release strbuf after use in userdiff_get_textconv()
  utf8: release strbuf on error return in strbuf_utf8_replace()
  vcs-svn: release strbuf after use in end_revision()
  wt-status: release strbuf after use in read_rebase_todolist()
  wt-status: release strbuf after use in wt_longstatus_print_tracking()

 builtin/am.c               | 34 ++++++++++++++++++++++------------
 builtin/check-ref-format.c |  1 +
 builtin/clean.c            |  7 +++++--
 builtin/clone.c            |  2 +-
 builtin/help.c             |  3 +++
 builtin/merge.c            |  9 +++++++--
 builtin/notes.c            |  1 +
 builtin/remote.c           |  8 +++++---
 builtin/shortlog.c         |  1 +
 commit.c                   |  7 +++++--
 connect.c                  |  4 +++-
 convert.c                  |  4 +++-
 diff.c                     |  3 +++
 mailinfo.c                 | 10 +++++-----
 refs.c                     |  2 +-
 send-pack.c                |  5 ++++-
 sequencer.c                |  5 ++++-
 sha1_file.c                |  6 +++---
 transport-helper.c         |  1 +
 userdiff.c                 |  1 +
 utf8.c                     |  3 ++-
 vcs-svn/svndump.c          |  1 +
 wt-status.c                |  2 ++
 23 files changed, 84 insertions(+), 36 deletions(-)

-- 
2.14.1

[PATCH 01/34] am: release strbufs after use in detect_patch_format()

[Rene Scharfe]

Don't reset the strbufs l2 and l3 before use as if they were static, but
release them at the end instead.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/am.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index c369dd1dce..3c50b03faa 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -633,73 +633,73 @@ static int is_mail(FILE *fp)
 static int detect_patch_format(const char **paths)
 {
 	enum patch_format ret = PATCH_FORMAT_UNKNOWN;
 	struct strbuf l1 = STRBUF_INIT;
 	struct strbuf l2 = STRBUF_INIT;
 	struct strbuf l3 = STRBUF_INIT;
 	FILE *fp;
 
 	/*
 	 * We default to mbox format if input is from stdin and for directories
 	 */
 	if (!*paths || !strcmp(*paths, "-") || is_directory(*paths))
 		return PATCH_FORMAT_MBOX;
 
 	/*
 	 * Otherwise, check the first few lines of the first patch, starting
 	 * from the first non-blank line, to try to detect its format.
 	 */
 
 	fp = xfopen(*paths, "r");
 
 	while (!strbuf_getline(&l1, fp)) {
 		if (l1.len)
 			break;
 	}
 
 	if (starts_with(l1.buf, "From ") || starts_with(l1.buf, "From: ")) {
 		ret = PATCH_FORMAT_MBOX;
 		goto done;
 	}
 
 	if (starts_with(l1.buf, "# This series applies on GIT commit")) {
 		ret = PATCH_FORMAT_STGIT_SERIES;
 		goto done;
 	}
 
 	if (!strcmp(l1.buf, "# HG changeset patch")) {
 		ret = PATCH_FORMAT_HG;
 		goto done;
 	}
 
-	strbuf_reset(&l2);
 	strbuf_getline(&l2, fp);
-	strbuf_reset(&l3);
 	strbuf_getline(&l3, fp);
 
 	/*
 	 * If the second line is empty and the third is a From, Author or Date
 	 * entry, this is likely an StGit patch.
 	 */
 	if (l1.len && !l2.len &&
 		(starts_with(l3.buf, "From:") ||
 		 starts_with(l3.buf, "Author:") ||
 		 starts_with(l3.buf, "Date:"))) {
 		ret = PATCH_FORMAT_STGIT;
 		goto done;
 	}
 
 	if (l1.len && is_mail(fp)) {
 		ret = PATCH_FORMAT_MBOX;
 		goto done;
 	}
 
 done:
 	fclose(fp);
 	strbuf_release(&l1);
+	strbuf_release(&l2);
+	strbuf_release(&l3);
 	return ret;
 }
 
 /**
  * Splits out individual email patches from `paths`, where each path is either
  * a mbox file or a Maildir. Returns 0 on success, -1 on failure.
  */
-- 
2.14.1

Re: [PATCH 01/34] am: release strbufs after use in detect_patch_format()

[Stefan Beller]

On Wed, Aug 30, 2017 at 10:49 AM, Rene Scharfe <l.s.r@web.de> wrote:
> Don't reset the strbufs l2 and l3 before use as if they were static, but
> release them at the end instead.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>

This was introduced with 5ae41c79b8 (builtin-am: support and
auto-detect StGit patches, 2015-08-04), trying to sift through old emails
I did not find a discussion if that was static in an earlier round of the
patches.

Thanks and
Reviewed-by: Stefan Beller <sbeller@google.com>

[PATCH 02/34] am: release strbuf on error return in hg_patch_to_mail()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/am.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 3c50b03faa..3d38b3fe9f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -881,75 +881,84 @@ static int split_mail_stgit_series(struct am_state *state, const char **paths,
 static int hg_patch_to_mail(FILE *out, FILE *in, int keep_cr)
 {
 	struct strbuf sb = STRBUF_INIT;
+	int rc = 0;
 
 	while (!strbuf_getline_lf(&sb, in)) {
 		const char *str;
 
 		if (skip_prefix(sb.buf, "# User ", &str))
 			fprintf(out, "From: %s\n", str);
 		else if (skip_prefix(sb.buf, "# Date ", &str)) {
 			timestamp_t timestamp;
 			long tz, tz2;
 			char *end;
 
 			errno = 0;
 			timestamp = parse_timestamp(str, &end, 10);
-			if (errno)
-				return error(_("invalid timestamp"));
+			if (errno) {
+				rc = error(_("invalid timestamp"));
+				goto exit;
+			}
 
-			if (!skip_prefix(end, " ", &str))
-				return error(_("invalid Date line"));
+			if (!skip_prefix(end, " ", &str)) {
+				rc = error(_("invalid Date line"));
+				goto exit;
+			}
 
 			errno = 0;
 			tz = strtol(str, &end, 10);
-			if (errno)
-				return error(_("invalid timezone offset"));
+			if (errno) {
+				rc = error(_("invalid timezone offset"));
+				goto exit;
+			}
 
-			if (*end)
-				return error(_("invalid Date line"));
+			if (*end) {
+				rc = error(_("invalid Date line"));
+				goto exit;
+			}
 
 			/*
 			 * mercurial's timezone is in seconds west of UTC,
 			 * however git's timezone is in hours + minutes east of
 			 * UTC. Convert it.
 			 */
 			tz2 = labs(tz) / 3600 * 100 + labs(tz) % 3600 / 60;
 			if (tz > 0)
 				tz2 = -tz2;
 
 			fprintf(out, "Date: %s\n", show_date(timestamp, tz2, DATE_MODE(RFC2822)));
 		} else if (starts_with(sb.buf, "# ")) {
 			continue;
 		} else {
 			fprintf(out, "\n%s\n", sb.buf);
 			break;
 		}
 	}
 
 	strbuf_reset(&sb);
 	while (strbuf_fread(&sb, 8192, in) > 0) {
 		fwrite(sb.buf, 1, sb.len, out);
 		strbuf_reset(&sb);
 	}
-
+exit:
 	strbuf_release(&sb);
-	return 0;
+	return rc;
 }
 
 /**
  * Splits a list of files/directories into individual email patches. Each path
  * in `paths` must be a file/directory that is formatted according to
  * `patch_format`.
  *
  * Once split out, the individual email patches will be stored in the state
  * directory, with each patch's filename being its index, padded to state->prec
  * digits.
  *
  * state->cur will be set to the index of the first mail, and state->last will
  * be set to the index of the last mail.
  *
  * Set keep_cr to 0 to convert all lines ending with \r\n to end with \n, 1
  * to disable this behavior, -1 to use the default configured setting.
  *
  * Returns 0 on success, -1 on failure.
  */
-- 
2.14.1

[PATCH 03/34] am: release strbuf after use in safe_to_abort()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/am.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/am.c b/builtin/am.c
index 3d38b3fe9f..d7513f5375 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2095,29 +2095,30 @@ static void am_skip(struct am_state *state)
 static int safe_to_abort(const struct am_state *state)
 {
 	struct strbuf sb = STRBUF_INIT;
 	struct object_id abort_safety, head;
 
 	if (file_exists(am_path(state, "dirtyindex")))
 		return 0;
 
 	if (read_state_file(&sb, state, "abort-safety", 1) > 0) {
 		if (get_oid_hex(sb.buf, &abort_safety))
 			die(_("could not parse %s"), am_path(state, "abort-safety"));
 	} else
 		oidclr(&abort_safety);
+	strbuf_release(&sb);
 
 	if (get_oid("HEAD", &head))
 		oidclr(&head);
 
 	if (!oidcmp(&head, &abort_safety))
 		return 1;
 
 	warning(_("You seem to have moved HEAD since the last 'am' failure.\n"
 		"Not rewinding to ORIG_HEAD"));
 
 	return 0;
 }
 
 /**
  * Aborts the current am session if it is safe to do so.
  */
-- 
2.14.1

[PATCH 04/34] check-ref-format: release strbuf after use in check_ref_format_branch()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/check-ref-format.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index eac499450f..6c40ff110b 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -39,12 +39,13 @@ static char *collapse_slashes(const char *refname)
 static int check_ref_format_branch(const char *arg)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int nongit;
 
 	setup_git_directory_gently(&nongit);
 	if (strbuf_check_branch_ref(&sb, arg))
 		die("'%s' is not a valid branch name", arg);
 	printf("%s\n", sb.buf + 11);
+	strbuf_release(&sb);
 	return 0;
 }
 
-- 
2.14.1

[PATCH 05/34] clean: release strbuf after use in remove_dirs()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/clean.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 21a7a32994..733b6d3745 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -151,104 +151,107 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)
 static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 		int dry_run, int quiet, int *dir_gone)
 {
 	DIR *dir;
 	struct strbuf quoted = STRBUF_INIT;
 	struct dirent *e;
 	int res = 0, ret = 0, gone = 1, original_len = path->len, len;
 	struct string_list dels = STRING_LIST_INIT_DUP;
 
 	*dir_gone = 1;
 
 	if ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) && is_nonbare_repository_dir(path)) {
 		if (!quiet) {
 			quote_path_relative(path->buf, prefix, &quoted);
 			printf(dry_run ?  _(msg_would_skip_git_dir) : _(msg_skip_git_dir),
 					quoted.buf);
 		}
 
 		*dir_gone = 0;
-		return 0;
+		goto out;
 	}
 
 	dir = opendir(path->buf);
 	if (!dir) {
 		/* an empty dir could be removed even if it is unreadble */
 		res = dry_run ? 0 : rmdir(path->buf);
 		if (res) {
 			int saved_errno = errno;
 			quote_path_relative(path->buf, prefix, &quoted);
 			errno = saved_errno;
 			warning_errno(_(msg_warn_remove_failed), quoted.buf);
 			*dir_gone = 0;
 		}
-		return res;
+		ret = res;
+		goto out;
 	}
 
 	strbuf_complete(path, '/');
 
 	len = path->len;
 	while ((e = readdir(dir)) != NULL) {
 		struct stat st;
 		if (is_dot_or_dotdot(e->d_name))
 			continue;
 
 		strbuf_setlen(path, len);
 		strbuf_addstr(path, e->d_name);
 		if (lstat(path->buf, &st))
 			; /* fall thru */
 		else if (S_ISDIR(st.st_mode)) {
 			if (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))
 				ret = 1;
 			if (gone) {
 				quote_path_relative(path->buf, prefix, &quoted);
 				string_list_append(&dels, quoted.buf);
 			} else
 				*dir_gone = 0;
 			continue;
 		} else {
 			res = dry_run ? 0 : unlink(path->buf);
 			if (!res) {
 				quote_path_relative(path->buf, prefix, &quoted);
 				string_list_append(&dels, quoted.buf);
 			} else {
 				int saved_errno = errno;
 				quote_path_relative(path->buf, prefix, &quoted);
 				errno = saved_errno;
 				warning_errno(_(msg_warn_remove_failed), quoted.buf);
 				*dir_gone = 0;
 				ret = 1;
 			}
 			continue;
 		}
 
 		/* path too long, stat fails, or non-directory still exists */
 		*dir_gone = 0;
 		ret = 1;
 		break;
 	}
 	closedir(dir);
 
 	strbuf_setlen(path, original_len);
 
 	if (*dir_gone) {
 		res = dry_run ? 0 : rmdir(path->buf);
 		if (!res)
 			*dir_gone = 1;
 		else {
 			int saved_errno = errno;
 			quote_path_relative(path->buf, prefix, &quoted);
 			errno = saved_errno;
 			warning_errno(_(msg_warn_remove_failed), quoted.buf);
 			*dir_gone = 0;
 			ret = 1;
 		}
 	}
 
 	if (!*dir_gone && !quiet) {
 		int i;
 		for (i = 0; i < dels.nr; i++)
 			printf(dry_run ?  _(msg_would_remove) : _(msg_remove), dels.items[i].string);
 	}
+out:
+	strbuf_release(&quoted);
 	string_list_clear(&dels, 0);
 	return ret;
 }
-- 
2.14.1

[PATCH 06/34] clone: release strbuf after use in remove_junk()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 8d11b570a1..dbddd98f80 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -487,28 +487,28 @@ N_("Clone succeeded, but checkout failed.\n"
 static void remove_junk(void)
 {
 	struct strbuf sb = STRBUF_INIT;
 
 	switch (junk_mode) {
 	case JUNK_LEAVE_REPO:
 		warning("%s", _(junk_leave_repo_msg));
 		/* fall-through */
 	case JUNK_LEAVE_ALL:
 		return;
 	default:
 		/* proceed to removal */
 		break;
 	}
 
 	if (junk_git_dir) {
 		strbuf_addstr(&sb, junk_git_dir);
 		remove_dir_recursively(&sb, 0);
 		strbuf_reset(&sb);
 	}
 	if (junk_work_tree) {
 		strbuf_addstr(&sb, junk_work_tree);
 		remove_dir_recursively(&sb, 0);
-		strbuf_reset(&sb);
 	}
+	strbuf_release(&sb);
 }
 
 static void remove_junk_on_signal(int signo)
-- 
2.14.1

Re: [PATCH 06/34] clone: release strbuf after use in remove_junk()

[Junio C Hamano]

Rene Scharfe <l.s.r@web.de> writes:

> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  builtin/clone.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 8d11b570a1..dbddd98f80 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -487,28 +487,28 @@ N_("Clone succeeded, but checkout failed.\n"
> ...
>  	if (junk_git_dir) {
>  		strbuf_addstr(&sb, junk_git_dir);
>  		remove_dir_recursively(&sb, 0);
>  		strbuf_reset(&sb);
>  	}
>  	if (junk_work_tree) {
>  		strbuf_addstr(&sb, junk_work_tree);
>  		remove_dir_recursively(&sb, 0);
> -		strbuf_reset(&sb);
>  	}
> +	strbuf_release(&sb);
>  }

The code definitely needs a _release() at the end, but I feel
lukewarm about the "if we are about to _release(), do not bother to
_reset()" micro-optimization.  Keeping the existing two users that
use sb as a (shared and reused) temporary similar would help those
who add the third one or reuse the pattern in their code elsewhere.

We could flip the "be nice to the next user by clearing after use"
pattern to "clear any potential cruft before you use", i.e.

	if (...) {
		strbuf_reset(&sb);
		strbuf_addstr(&sb, ...);
	}
	if (...) {
		strbuf_reset(&sb);
		strbuf_addstr(&sb, ...);
	}
	strbuf_release(&sb);

but that still tempts us for the same micro-optimization at the
beginning where sb hasn't been used since STRBUF_INIT, so it is not
a real "solution".

So, I dunno.

Re: [PATCH 06/34] clone: release strbuf after use in remove_junk()

[René Scharfe]

Am 06.09.2017 um 21:51 schrieb Junio C Hamano:
> Rene Scharfe <l.s.r@web.de> writes:
> 
>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>> ---
>>   builtin/clone.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index 8d11b570a1..dbddd98f80 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -487,28 +487,28 @@ N_("Clone succeeded, but checkout failed.\n"
>> ...
>>   	if (junk_git_dir) {
>>   		strbuf_addstr(&sb, junk_git_dir);
>>   		remove_dir_recursively(&sb, 0);
>>   		strbuf_reset(&sb);
>>   	}
>>   	if (junk_work_tree) {
>>   		strbuf_addstr(&sb, junk_work_tree);
>>   		remove_dir_recursively(&sb, 0);
>> -		strbuf_reset(&sb);
>>   	}
>> +	strbuf_release(&sb);
>>   }
> 
> The code definitely needs a _release() at the end, but I feel
> lukewarm about the "if we are about to _release(), do not bother to
> _reset()" micro-optimization.  Keeping the existing two users that
> use sb as a (shared and reused) temporary similar would help those
> who add the third one or reuse the pattern in their code elsewhere.

That's not intended as an optimization, but as a promotion -- the reset
is moved to the outer block and upgraded to a release.  The result is
consistent with builtin/worktree.c::remove_junk().

> We could flip the "be nice to the next user by clearing after use"
> pattern to "clear any potential cruft before you use", i.e.
> 
> 	if (...) {
> 		strbuf_reset(&sb);
> 		strbuf_addstr(&sb, ...);
> 	}
> 	if (...) {
> 		strbuf_reset(&sb);
> 		strbuf_addstr(&sb, ...);
> 	}
> 	strbuf_release(&sb);
> 
> but that still tempts us for the same micro-optimization at the
> beginning where sb hasn't been used since STRBUF_INIT, so it is not
> a real "solution".

If code reuse is the goal then a different micro-optimization is more
of a hindrance IMHO: Reusing the same strbuf for both deletions.  A
deletion function that takes a plain string and handles strbuf
formalities for the caller would have avoided the leak, be easier to
use for deleting a third file and probably not have a measurable
performance impact due to the low number of calls.

René

Re: [PATCH 06/34] clone: release strbuf after use in remove_junk()

[Jeff King]

On Sun, Sep 10, 2017 at 08:27:40AM +0200, René Scharfe wrote:

> >>   	if (junk_work_tree) {
> >>   		strbuf_addstr(&sb, junk_work_tree);
> >>   		remove_dir_recursively(&sb, 0);
> >> -		strbuf_reset(&sb);
> >>   	}
> >> +	strbuf_release(&sb);
> >>   }
> > 
> > The code definitely needs a _release() at the end, but I feel
> > lukewarm about the "if we are about to _release(), do not bother to
> > _reset()" micro-optimization.  Keeping the existing two users that
> > use sb as a (shared and reused) temporary similar would help those
> > who add the third one or reuse the pattern in their code elsewhere.
> 
> That's not intended as an optimization, but as a promotion -- the reset
> is moved to the outer block and upgraded to a release.  The result is
> consistent with builtin/worktree.c::remove_junk().

Hmm. This is a cleanup function called only from signal and atexit
handlers. I don't think we actually do need to clean up, and this might
be a good candidate for UNLEAK().

And in fact, being called from a signal handler means we should
generally avoid touching malloc or free (which could be holding locks).
That would mean preferring a leak to strbuf_release(). Of course that is
the tip of the iceberg. We call strbuf_addstr() here, and
remove_dir_recursively() will grow our buffer.

So I actually wonder if junk_git_dir and junk_work_tree should be
pre-sized strbufs themselves. And that makes the leak "go away" in the
eyes of leak-checkers because we hold onto the static strbufs until
program exit.

I.e., something like this:

diff --git a/builtin/clone.c b/builtin/clone.c
index 8d11b570a1..a350f7801e 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -471,8 +471,19 @@ static void clone_local(const char *src_repo, const char *dest_repo)
 		fprintf(stderr, _("done.\n"));
 }
 
-static const char *junk_work_tree;
-static const char *junk_git_dir;
+static void register_junk(struct strbuf *junk, const char *str)
+{
+	/*
+	 * we don't want to have to allocate for recursive removal during a
+	 * signal handler, so pre-size our strbufs to something that is
+	 * unlikely to overflow.
+	 */
+	strbuf_grow(junk, 4096);
+	strbuf_addstr(junk, str);
+}
+
+static struct strbuf junk_work_tree = STRBUF_INIT;
+static struct strbuf junk_git_dir = STRBUF_INIT;
 static enum {
 	JUNK_LEAVE_NONE,
 	JUNK_LEAVE_REPO,
@@ -486,8 +497,6 @@ N_("Clone succeeded, but checkout failed.\n"
 
 static void remove_junk(void)
 {
-	struct strbuf sb = STRBUF_INIT;
-
 	switch (junk_mode) {
 	case JUNK_LEAVE_REPO:
 		warning("%s", _(junk_leave_repo_msg));
@@ -499,16 +508,10 @@ static void remove_junk(void)
 		break;
 	}
 
-	if (junk_git_dir) {
-		strbuf_addstr(&sb, junk_git_dir);
-		remove_dir_recursively(&sb, 0);
-		strbuf_reset(&sb);
-	}
-	if (junk_work_tree) {
-		strbuf_addstr(&sb, junk_work_tree);
-		remove_dir_recursively(&sb, 0);
-		strbuf_reset(&sb);
-	}
+	if (junk_git_dir.len)
+		remove_dir_recursively(&junk_git_dir, 0);
+	if (junk_work_tree.len)
+		remove_dir_recursively(&junk_work_tree, 0);
 }
 
 static void remove_junk_on_signal(int signo)
@@ -970,11 +973,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		if (!dest_exists && mkdir(work_tree, 0777))
 			die_errno(_("could not create work tree dir '%s'"),
 				  work_tree);
-		junk_work_tree = work_tree;
+		register_junk(&junk_work_tree, work_tree);
 		set_git_work_tree(work_tree);
 	}
 
-	junk_git_dir = real_git_dir ? real_git_dir : git_dir;
+	register_junk(&junk_git_dir, real_git_dir ? real_git_dir : git_dir);
 	if (safe_create_leading_directories_const(git_dir) < 0)
 		die(_("could not create leading directories of '%s'"), git_dir);
 

Technically this would probably also benefit from all of the variables
being marked volatile, but we'd have to cast the volatility away to use
any strbuf functions. :(

If we really wanted to make this robust for signals (and I'm not sure
that it is worth the effort), I suspect the best route would be to teach
the tempfile.c code (which tries very hard to be careful about signals
and volatility) to handle directories.

-Peff

Re: [PATCH 06/34] clone: release strbuf after use in remove_junk()

[René Scharfe]

Am 10.09.2017 um 09:30 schrieb Jeff King:
> On Sun, Sep 10, 2017 at 08:27:40AM +0200, René Scharfe wrote:
> 
>>>>    	if (junk_work_tree) {
>>>>    		strbuf_addstr(&sb, junk_work_tree);
>>>>    		remove_dir_recursively(&sb, 0);
>>>> -		strbuf_reset(&sb);
>>>>    	}
>>>> +	strbuf_release(&sb);
>>>>    }
>>>
>>> The code definitely needs a _release() at the end, but I feel
>>> lukewarm about the "if we are about to _release(), do not bother to
>>> _reset()" micro-optimization.  Keeping the existing two users that
>>> use sb as a (shared and reused) temporary similar would help those
>>> who add the third one or reuse the pattern in their code elsewhere.
>>
>> That's not intended as an optimization, but as a promotion -- the reset
>> is moved to the outer block and upgraded to a release.  The result is
>> consistent with builtin/worktree.c::remove_junk().
> 
> Hmm. This is a cleanup function called only from signal and atexit
> handlers. I don't think we actually do need to clean up, and this might
> be a good candidate for UNLEAK().
> 
> And in fact, being called from a signal handler means we should
> generally avoid touching malloc or free (which could be holding locks).
> That would mean preferring a leak to strbuf_release(). Of course that is
> the tip of the iceberg. We call strbuf_addstr() here, and
> remove_dir_recursively() will grow our buffer.

And we call opendir(3), readdir(3), and closedir(3), which are also not
listed as async-safe functions by POSIX [1].

> So I actually wonder if junk_git_dir and junk_work_tree should be
> pre-sized strbufs themselves. And that makes the leak "go away" in the
> eyes of leak-checkers because we hold onto the static strbufs until
> program exit.

We could start with a small buffer and expand it to match the path
length of written files as we go.

Deletion without readdir(3) requires us to keep a list of all written
files and directories, though.  Perhaps in the form of an append-only
log; the signal handler could then go through them in reverse order
and remove them.  Or is there a better way?

René


[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03_03

Re: [PATCH 06/34] clone: release strbuf after use in remove_junk()

[Jeff King]

On Sun, Sep 10, 2017 at 12:37:06PM +0200, René Scharfe wrote:

> > And in fact, being called from a signal handler means we should
> > generally avoid touching malloc or free (which could be holding locks).
> > That would mean preferring a leak to strbuf_release(). Of course that is
> > the tip of the iceberg. We call strbuf_addstr() here, and
> > remove_dir_recursively() will grow our buffer.
> 
> And we call opendir(3), readdir(3), and closedir(3), which are also not
> listed as async-safe functions by POSIX [1].

Good point. I don't know how dangerous those are in the real-world
(i.e., is POSIX leaving an out for some implementations, or are they
really unsafe on common platforms like Linux).

> > So I actually wonder if junk_git_dir and junk_work_tree should be
> > pre-sized strbufs themselves. And that makes the leak "go away" in the
> > eyes of leak-checkers because we hold onto the static strbufs until
> > program exit.
> 
> We could start with a small buffer and expand it to match the path
> length of written files as we go.

Yes, but I didn't want to touch each code site that creates a file,
which is a lot more invasive. I expect expanding to 4096 (or PATH_MAX)
would be sufficient in practice.

I'd also note that the current code isn't _remotely_ async safe and
nobody's complained. So I'm perfectly happy doing nothing, too. I care
a bit more about the tempfile.c interface because it's invoked a lot
more frequently.

> Deletion without readdir(3) requires us to keep a list of all written
> files and directories, though.  Perhaps in the form of an append-only
> log; the signal handler could then go through them in reverse order
> and remove them.  Or is there a better way?

No, I think that's how you'd have to do it. The implementation isn't all
that bad, but hitting every file-creation would be invasive. I'd
almost rather bail to exec-ing "rm -rf $dir", but we probably can't do
_that_ in a signal-handler either. :)

-Peff

Re: [PATCH 06/34] clone: release strbuf after use in remove_junk()

[René Scharfe]

Am 10.09.2017 um 19:38 schrieb Jeff King:
> On Sun, Sep 10, 2017 at 12:37:06PM +0200, René Scharfe wrote:
> 
>>> And in fact, being called from a signal handler means we should
>>> generally avoid touching malloc or free (which could be holding locks).
>>> That would mean preferring a leak to strbuf_release(). Of course that is
>>> the tip of the iceberg. We call strbuf_addstr() here, and
>>> remove_dir_recursively() will grow our buffer.
>>
>> And we call opendir(3), readdir(3), and closedir(3), which are also not
>> listed as async-safe functions by POSIX [1].
> 
> Good point. I don't know how dangerous those are in the real-world
> (i.e., is POSIX leaving an out for some implementations, or are they
> really unsafe on common platforms like Linux).

No idea.

>>> So I actually wonder if junk_git_dir and junk_work_tree should be
>>> pre-sized strbufs themselves. And that makes the leak "go away" in the
>>> eyes of leak-checkers because we hold onto the static strbufs until
>>> program exit.
>>
>> We could start with a small buffer and expand it to match the path
>> length of written files as we go.
> 
> Yes, but I didn't want to touch each code site that creates a file,
> which is a lot more invasive. I expect expanding to 4096 (or PATH_MAX)
> would be sufficient in practice.

Perhaps it is in most cases.  Having certainty would be better.  We can
leave unpack_trees() untouched and instead traverse the tree beforehand,
in order to find the longest path.  Perhaps we can do something similar
for init_db().

> I'd also note that the current code isn't _remotely_ async safe and
> nobody's complained. So I'm perfectly happy doing nothing, too. I care
> a bit more about the tempfile.c interface because it's invoked a lot
> more frequently.

I guess clones are not interrupted very often during checkout; same
with worktree creation.  So perhaps nasty things could happen with a
low probability, but nobody tried hard enough to hit that case?

>> Deletion without readdir(3) requires us to keep a list of all written
>> files and directories, though.  Perhaps in the form of an append-only
>> log; the signal handler could then go through them in reverse order
>> and remove them.  Or is there a better way?
> 
> No, I think that's how you'd have to do it. The implementation isn't all
> that bad, but hitting every file-creation would be invasive. I'd
> almost rather bail to exec-ing "rm -rf $dir", but we probably can't do
> _that_ in a signal-handler either. :)

Well, fork(2), execve(2), and waitpid(2) are on the list, so actually
you can.

mingw_spawnvpe(), which is used by start_command() on Windows,
allocates memory, though.  Preparing environment and argument list
in advance and just calling CreateProcess() in the signal handler
might work.

René

Re: [PATCH 06/34] clone: release strbuf after use in remove_junk()

[Jeff King]

On Mon, Sep 11, 2017 at 11:40:05PM +0200, René Scharfe wrote:

> > Yes, but I didn't want to touch each code site that creates a file,
> > which is a lot more invasive. I expect expanding to 4096 (or PATH_MAX)
> > would be sufficient in practice.
> 
> Perhaps it is in most cases.  Having certainty would be better.  We can
> leave unpack_trees() untouched and instead traverse the tree beforehand,
> in order to find the longest path.  Perhaps we can do something similar
> for init_db().

I wonder if it would be possible to just wrap open() in a transparent
way.

> > I'd also note that the current code isn't _remotely_ async safe and
> > nobody's complained. So I'm perfectly happy doing nothing, too. I care
> > a bit more about the tempfile.c interface because it's invoked a lot
> > more frequently.
> 
> I guess clones are not interrupted very often during checkout; same
> with worktree creation.  So perhaps nasty things could happen with a
> low probability, but nobody tried hard enough to hit that case?

Right, that's my guess. And "nasty" is quite likely to be "deadlocks on
a malloc or stdio lock". Which is not the end of the world.

> > No, I think that's how you'd have to do it. The implementation isn't all
> > that bad, but hitting every file-creation would be invasive. I'd
> > almost rather bail to exec-ing "rm -rf $dir", but we probably can't do
> > _that_ in a signal-handler either. :)
> 
> Well, fork(2), execve(2), and waitpid(2) are on the list, so actually
> you can.
> 
> mingw_spawnvpe(), which is used by start_command() on Windows,
> allocates memory, though.  Preparing environment and argument list
> in advance and just calling CreateProcess() in the signal handler
> might work.

Yeah, I imagine it's do-able with enough advance effort.

Given the lack of reports and the rapidly expanding complexity, I'm not
planning on looking further into this for now.

-Peff

[PATCH 07/34] commit: release strbuf on error return in commit_tree_extended()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 commit.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/commit.c b/commit.c
index 8b28415939..51f969fcbc 100644
--- a/commit.c
+++ b/commit.c
@@ -1514,60 +1514,63 @@ N_("Warning: commit message did not conform to UTF-8.\n"
 int commit_tree_extended(const char *msg, size_t msg_len,
 			 const unsigned char *tree,
 			 struct commit_list *parents, unsigned char *ret,
 			 const char *author, const char *sign_commit,
 			 struct commit_extra_header *extra)
 {
 	int result;
 	int encoding_is_utf8;
 	struct strbuf buffer;
 
 	assert_sha1_type(tree, OBJ_TREE);
 
 	if (memchr(msg, '\0', msg_len))
 		return error("a NUL byte in commit log message not allowed.");
 
 	/* Not having i18n.commitencoding is the same as having utf-8 */
 	encoding_is_utf8 = is_encoding_utf8(git_commit_encoding);
 
 	strbuf_init(&buffer, 8192); /* should avoid reallocs for the headers */
 	strbuf_addf(&buffer, "tree %s\n", sha1_to_hex(tree));
 
 	/*
 	 * NOTE! This ordering means that the same exact tree merged with a
 	 * different order of parents will be a _different_ changeset even
 	 * if everything else stays the same.
 	 */
 	while (parents) {
 		struct commit *parent = pop_commit(&parents);
 		strbuf_addf(&buffer, "parent %s\n",
 			    oid_to_hex(&parent->object.oid));
 	}
 
 	/* Person/date information */
 	if (!author)
 		author = git_author_info(IDENT_STRICT);
 	strbuf_addf(&buffer, "author %s\n", author);
 	strbuf_addf(&buffer, "committer %s\n", git_committer_info(IDENT_STRICT));
 	if (!encoding_is_utf8)
 		strbuf_addf(&buffer, "encoding %s\n", git_commit_encoding);
 
 	while (extra) {
 		add_extra_header(&buffer, extra);
 		extra = extra->next;
 	}
 	strbuf_addch(&buffer, '\n');
 
 	/* And add the comment */
 	strbuf_add(&buffer, msg, msg_len);
 
 	/* And check the encoding */
 	if (encoding_is_utf8 && !verify_utf8(&buffer))
 		fprintf(stderr, _(commit_utf8_warn));
 
-	if (sign_commit && do_sign_commit(&buffer, sign_commit))
-		return -1;
+	if (sign_commit && do_sign_commit(&buffer, sign_commit)) {
+		result = -1;
+		goto out;
+	}
 
 	result = write_sha1_file(buffer.buf, buffer.len, commit_type, ret);
+out:
 	strbuf_release(&buffer);
 	return result;
 }
-- 
2.14.1

Re: [PATCH 07/34] commit: release strbuf on error return in commit_tree_extended()

[Stefan Beller]

On Wed, Aug 30, 2017 at 10:49 AM, Rene Scharfe <l.s.r@web.de> wrote:
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  commit.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index 8b28415939..51f969fcbc 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1514,60 +1514,63 @@ N_("Warning: commit message did not conform to UTF-8.\n"
>  int commit_tree_extended(const char *msg, size_t msg_len,
>                          const unsigned char *tree,
>                          struct commit_list *parents, unsigned char *ret,
>                          const char *author, const char *sign_commit,
>                          struct commit_extra_header *extra)
>  {
>         int result;
>         int encoding_is_utf8;
>         struct strbuf buffer;
>
>         assert_sha1_type(tree, OBJ_TREE);
>
>         if (memchr(msg, '\0', msg_len))
>                 return error("a NUL byte in commit log message not allowed.");
>
>         /* Not having i18n.commitencoding is the same as having utf-8 */
>         encoding_is_utf8 = is_encoding_utf8(git_commit_encoding);
>
>         strbuf_init(&buffer, 8192); /* should avoid reallocs for the headers */
>         strbuf_addf(&buffer, "tree %s\n", sha1_to_hex(tree));
>
>         /*
>          * NOTE! This ordering means that the same exact tree merged with a
>          * different order of parents will be a _different_ changeset even
>          * if everything else stays the same.
>          */
>         while (parents) {
>                 struct commit *parent = pop_commit(&parents);
>                 strbuf_addf(&buffer, "parent %s\n",
>                             oid_to_hex(&parent->object.oid));
>         }
>
>         /* Person/date information */
>         if (!author)
>                 author = git_author_info(IDENT_STRICT);
>         strbuf_addf(&buffer, "author %s\n", author);
>         strbuf_addf(&buffer, "committer %s\n", git_committer_info(IDENT_STRICT));
>         if (!encoding_is_utf8)
>                 strbuf_addf(&buffer, "encoding %s\n", git_commit_encoding);
>
>         while (extra) {
>                 add_extra_header(&buffer, extra);
>                 extra = extra->next;
>         }
>         strbuf_addch(&buffer, '\n');
>
>         /* And add the comment */
>         strbuf_add(&buffer, msg, msg_len);
>
>         /* And check the encoding */
>         if (encoding_is_utf8 && !verify_utf8(&buffer))
>                 fprintf(stderr, _(commit_utf8_warn));
>
> -       if (sign_commit && do_sign_commit(&buffer, sign_commit))
> -               return -1;
> +       if (sign_commit && do_sign_commit(&buffer, sign_commit)) {
> +               result = -1;
> +               goto out;
> +       }

While this seems obviously correct (following the "goto cleanup" pattern),
I shortly wondered if it can be expressed more concisely, as we really skip
only one call:

    if (!sign_commit || !do_sign_commit(&buffer, sign_commit))
        result = write_sha1_file(...)
    else
        result = -1;

    strbuf_release(&buffer);
    return result;

Instead of an if, we could even inline it as

    result = (!sign_commit || !do_sign_commit(&buffer, sign_commit)) ?
        write_sha1_file(buffer.buf, buffer.len, commit_type, ret) : -1;

     strbuf_release(&buffer);

I am not sure if this is easier to read (maybe that is too dense?)
Just thought for food, the original patch looks good, too.

Thanks,
Stefan

[PATCH 08/34] connect: release strbuf on error return in git_connect()

[Rene Scharfe]

Reduce the scope of the variable cmd and release it before returning
early.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 connect.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index 49b28b83be..df56c0cbff 100644
--- a/connect.c
+++ b/connect.c
@@ -775,146 +775,148 @@ static void handle_ssh_variant(const char *ssh_command, int is_cmdline,
 struct child_process *git_connect(int fd[2], const char *url,
 				  const char *prog, int flags)
 {
 	char *hostandport, *path;
 	struct child_process *conn = &no_fork;
 	enum protocol protocol;
-	struct strbuf cmd = STRBUF_INIT;
 
 	/* Without this we cannot rely on waitpid() to tell
 	 * what happened to our children.
 	 */
 	signal(SIGCHLD, SIG_DFL);
 
 	protocol = parse_connect_url(url, &hostandport, &path);
 	if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
 		printf("Diag: url=%s\n", url ? url : "NULL");
 		printf("Diag: protocol=%s\n", prot_name(protocol));
 		printf("Diag: hostandport=%s\n", hostandport ? hostandport : "NULL");
 		printf("Diag: path=%s\n", path ? path : "NULL");
 		conn = NULL;
 	} else if (protocol == PROTO_GIT) {
 		/*
 		 * Set up virtual host information based on where we will
 		 * connect, unless the user has overridden us in
 		 * the environment.
 		 */
 		char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
 		if (target_host)
 			target_host = xstrdup(target_host);
 		else
 			target_host = xstrdup(hostandport);
 
 		transport_check_allowed("git");
 
 		/* These underlying connection commands die() if they
 		 * cannot connect.
 		 */
 		if (git_use_proxy(hostandport))
 			conn = git_proxy_connect(fd, hostandport);
 		else
 			git_tcp_connect(fd, hostandport, flags);
 		/*
 		 * Separate original protocol components prog and path
 		 * from extended host header with a NUL byte.
 		 *
 		 * Note: Do not add any other headers here!  Doing so
 		 * will cause older git-daemon servers to crash.
 		 */
 		packet_write_fmt(fd[1],
 			     "%s %s%chost=%s%c",
 			     prog, path, 0,
 			     target_host, 0);
 		free(target_host);
 	} else {
+		struct strbuf cmd = STRBUF_INIT;
+
 		conn = xmalloc(sizeof(*conn));
 		child_process_init(conn);
 
 		if (looks_like_command_line_option(path))
 			die("strange pathname '%s' blocked", path);
 
 		strbuf_addstr(&cmd, prog);
 		strbuf_addch(&cmd, ' ');
 		sq_quote_buf(&cmd, path);
 
 		/* remove repo-local variables from the environment */
 		conn->env = local_repo_env;
 		conn->use_shell = 1;
 		conn->in = conn->out = -1;
 		if (protocol == PROTO_SSH) {
 			const char *ssh;
 			int needs_batch = 0;
 			int port_option = 'p';
 			char *ssh_host = hostandport;
 			const char *port = NULL;
 			transport_check_allowed("ssh");
 			get_host_and_port(&ssh_host, &port);
 
 			if (!port)
 				port = get_port(ssh_host);
 
 			if (flags & CONNECT_DIAG_URL) {
 				printf("Diag: url=%s\n", url ? url : "NULL");
 				printf("Diag: protocol=%s\n", prot_name(protocol));
 				printf("Diag: userandhost=%s\n", ssh_host ? ssh_host : "NULL");
 				printf("Diag: port=%s\n", port ? port : "NONE");
 				printf("Diag: path=%s\n", path ? path : "NULL");
 
 				free(hostandport);
 				free(path);
 				free(conn);
+				strbuf_release(&cmd);
 				return NULL;
 			}
 
 			if (looks_like_command_line_option(ssh_host))
 				die("strange hostname '%s' blocked", ssh_host);
 
 			ssh = get_ssh_command();
 			if (ssh)
 				handle_ssh_variant(ssh, 1, &port_option,
 						   &needs_batch);
 			else {
 				/*
 				 * GIT_SSH is the no-shell version of
 				 * GIT_SSH_COMMAND (and must remain so for
 				 * historical compatibility).
 				 */
 				conn->use_shell = 0;
 
 				ssh = getenv("GIT_SSH");
 				if (!ssh)
 					ssh = "ssh";
 				else
 					handle_ssh_variant(ssh, 0,
 							   &port_option,
 							   &needs_batch);
 			}
 
 			argv_array_push(&conn->args, ssh);
 			if (flags & CONNECT_IPV4)
 				argv_array_push(&conn->args, "-4");
 			else if (flags & CONNECT_IPV6)
 				argv_array_push(&conn->args, "-6");
 			if (needs_batch)
 				argv_array_push(&conn->args, "-batch");
 			if (port) {
 				argv_array_pushf(&conn->args,
 						 "-%c", port_option);
 				argv_array_push(&conn->args, port);
 			}
 			argv_array_push(&conn->args, ssh_host);
 		} else {
 			transport_check_allowed("file");
 		}
 		argv_array_push(&conn->args, cmd.buf);
 
 		if (start_command(conn))
 			die("unable to fork");
 
 		fd[0] = conn->out; /* read from child's stdout */
 		fd[1] = conn->in;  /* write to child's stdin */
 		strbuf_release(&cmd);
 	}
 	free(hostandport);
 	free(path);
 	return conn;
 }
-- 
2.14.1

Re: [PATCH 08/34] connect: release strbuf on error return in git_connect()

[Stefan Beller]

>         } else {
> +               struct strbuf cmd = STRBUF_INIT;
> +
...
>                 strbuf_addstr(&cmd, prog);
>                 strbuf_addch(&cmd, ' ');
>                 sq_quote_buf(&cmd, path);

...
>                 if (start_command(conn))
>                         die("unable to fork");
...
>                 strbuf_release(&cmd);

Here I reread the cover letter, your intent is only to fix memleaks
on non-die code paths. Ok. (unlike the grand vision of an
"always valgrind clean program")

Thanks,
Stefan

[PATCH 09/34] convert: release strbuf on error return in filter_buffer_or_fd()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 convert.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/convert.c b/convert.c
index c5f0b21037..4a0ed8d3cb 100644
--- a/convert.c
+++ b/convert.c
@@ -393,63 +393,65 @@ struct filter_params {
 static int filter_buffer_or_fd(int in, int out, void *data)
 {
 	/*
 	 * Spawn cmd and feed the buffer contents through its stdin.
 	 */
 	struct child_process child_process = CHILD_PROCESS_INIT;
 	struct filter_params *params = (struct filter_params *)data;
 	int write_err, status;
 	const char *argv[] = { NULL, NULL };
 
 	/* apply % substitution to cmd */
 	struct strbuf cmd = STRBUF_INIT;
 	struct strbuf path = STRBUF_INIT;
 	struct strbuf_expand_dict_entry dict[] = {
 		{ "f", NULL, },
 		{ NULL, NULL, },
 	};
 
 	/* quote the path to preserve spaces, etc. */
 	sq_quote_buf(&path, params->path);
 	dict[0].value = path.buf;
 
 	/* expand all %f with the quoted path */
 	strbuf_expand(&cmd, params->cmd, strbuf_expand_dict_cb, &dict);
 	strbuf_release(&path);
 
 	argv[0] = cmd.buf;
 
 	child_process.argv = argv;
 	child_process.use_shell = 1;
 	child_process.in = -1;
 	child_process.out = out;
 
-	if (start_command(&child_process))
+	if (start_command(&child_process)) {
+		strbuf_release(&cmd);
 		return error("cannot fork to run external filter '%s'", params->cmd);
+	}
 
 	sigchain_push(SIGPIPE, SIG_IGN);
 
 	if (params->src) {
 		write_err = (write_in_full(child_process.in,
 					   params->src, params->size) < 0);
 		if (errno == EPIPE)
 			write_err = 0;
 	} else {
 		write_err = copy_fd(params->fd, child_process.in);
 		if (write_err == COPY_WRITE_ERROR && errno == EPIPE)
 			write_err = 0;
 	}
 
 	if (close(child_process.in))
 		write_err = 1;
 	if (write_err)
 		error("cannot feed the input to external filter '%s'", params->cmd);
 
 	sigchain_pop(SIGPIPE);
 
 	status = finish_command(&child_process);
 	if (status)
 		error("external filter '%s' failed %d", params->cmd, status);
 
 	strbuf_release(&cmd);
 	return (write_err || status);
 }
-- 
2.14.1

[PATCH 10/34] diff: release strbuf after use in diff_summary()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 diff.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/diff.c b/diff.c
index 3d3e553a98..4148ba6980 100644
--- a/diff.c
+++ b/diff.c
@@ -5294,28 +5294,29 @@ static void show_rename_copy(struct diff_options *opt, const char *renamecopy,
 static void diff_summary(struct diff_options *opt, struct diff_filepair *p)
 {
 	switch(p->status) {
 	case DIFF_STATUS_DELETED:
 		show_file_mode_name(opt, "delete", p->one);
 		break;
 	case DIFF_STATUS_ADDED:
 		show_file_mode_name(opt, "create", p->two);
 		break;
 	case DIFF_STATUS_COPIED:
 		show_rename_copy(opt, "copy", p);
 		break;
 	case DIFF_STATUS_RENAMED:
 		show_rename_copy(opt, "rename", p);
 		break;
 	default:
 		if (p->score) {
 			struct strbuf sb = STRBUF_INIT;
 			strbuf_addstr(&sb, " rewrite ");
 			quote_c_style(p->two->path, &sb, NULL, 0);
 			strbuf_addf(&sb, " (%d%%)\n", similarity_index(p));
 			emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,
 					 sb.buf, sb.len, 0);
+			strbuf_release(&sb);
 		}
 		show_mode_change(opt, p, !p->score);
 		break;
 	}
 }
-- 
2.14.1

Re: [PATCH 10/34] diff: release strbuf after use in diff_summary()

[Stefan Beller]

On Wed, Aug 30, 2017 at 10:49 AM, Rene Scharfe <l.s.r@web.de> wrote:
> Signed-off-by: Rene Scharfe <l.s.r@web.de>

This was careless programming in the recent 146fdb0dfe (diff.c:
emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY, 2017-06-29)
I missed the cleanup.

Thanks,
Stefan

[PATCH 11/34] diff: release strbuf after use in show_rename_copy()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 diff.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/diff.c b/diff.c
index 4148ba6980..33c65f492d 100644
--- a/diff.c
+++ b/diff.c
@@ -5281,14 +5281,15 @@ static void show_mode_change(struct diff_options *opt, struct diff_filepair *p,
 static void show_rename_copy(struct diff_options *opt, const char *renamecopy,
 		struct diff_filepair *p)
 {
 	struct strbuf sb = STRBUF_INIT;
 	char *names = pprint_rename(p->one->path, p->two->path);
 	strbuf_addf(&sb, " %s %s (%d%%)\n",
 			renamecopy, names, similarity_index(p));
 	free(names);
 	emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,
 				 sb.buf, sb.len, 0);
 	show_mode_change(opt, p, 0);
+	strbuf_release(&sb);
 }
 
 static void diff_summary(struct diff_options *opt, struct diff_filepair *p)
-- 
2.14.1

[PATCH 12/34] diff: release strbuf after use in show_stats()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 diff.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/diff.c b/diff.c
index 33c65f492d..64cdcf2331 100644
--- a/diff.c
+++ b/diff.c
@@ -2334,255 +2334,256 @@ void print_stat_summary(FILE *fp, int files,
 static void show_stats(struct diffstat_t *data, struct diff_options *options)
 {
 	int i, len, add, del, adds = 0, dels = 0;
 	uintmax_t max_change = 0, max_len = 0;
 	int total_files = data->nr, count;
 	int width, name_width, graph_width, number_width = 0, bin_width = 0;
 	const char *reset, *add_c, *del_c;
 	int extra_shown = 0;
 	const char *line_prefix = diff_line_prefix(options);
 	struct strbuf out = STRBUF_INIT;
 
 	if (data->nr == 0)
 		return;
 
 	count = options->stat_count ? options->stat_count : data->nr;
 
 	reset = diff_get_color_opt(options, DIFF_RESET);
 	add_c = diff_get_color_opt(options, DIFF_FILE_NEW);
 	del_c = diff_get_color_opt(options, DIFF_FILE_OLD);
 
 	/*
 	 * Find the longest filename and max number of changes
 	 */
 	for (i = 0; (i < count) && (i < data->nr); i++) {
 		struct diffstat_file *file = data->files[i];
 		uintmax_t change = file->added + file->deleted;
 
 		if (!file->is_interesting && (change == 0)) {
 			count++; /* not shown == room for one more */
 			continue;
 		}
 		fill_print_name(file);
 		len = strlen(file->print_name);
 		if (max_len < len)
 			max_len = len;
 
 		if (file->is_unmerged) {
 			/* "Unmerged" is 8 characters */
 			bin_width = bin_width < 8 ? 8 : bin_width;
 			continue;
 		}
 		if (file->is_binary) {
 			/* "Bin XXX -> YYY bytes" */
 			int w = 14 + decimal_width(file->added)
 				+ decimal_width(file->deleted);
 			bin_width = bin_width < w ? w : bin_width;
 			/* Display change counts aligned with "Bin" */
 			number_width = 3;
 			continue;
 		}
 
 		if (max_change < change)
 			max_change = change;
 	}
 	count = i; /* where we can stop scanning in data->files[] */
 
 	/*
 	 * We have width = stat_width or term_columns() columns total.
 	 * We want a maximum of min(max_len, stat_name_width) for the name part.
 	 * We want a maximum of min(max_change, stat_graph_width) for the +- part.
 	 * We also need 1 for " " and 4 + decimal_width(max_change)
 	 * for " | NNNN " and one the empty column at the end, altogether
 	 * 6 + decimal_width(max_change).
 	 *
 	 * If there's not enough space, we will use the smaller of
 	 * stat_name_width (if set) and 5/8*width for the filename,
 	 * and the rest for constant elements + graph part, but no more
 	 * than stat_graph_width for the graph part.
 	 * (5/8 gives 50 for filename and 30 for the constant parts + graph
 	 * for the standard terminal size).
 	 *
 	 * In other words: stat_width limits the maximum width, and
 	 * stat_name_width fixes the maximum width of the filename,
 	 * and is also used to divide available columns if there
 	 * aren't enough.
 	 *
 	 * Binary files are displayed with "Bin XXX -> YYY bytes"
 	 * instead of the change count and graph. This part is treated
 	 * similarly to the graph part, except that it is not
 	 * "scaled". If total width is too small to accommodate the
 	 * guaranteed minimum width of the filename part and the
 	 * separators and this message, this message will "overflow"
 	 * making the line longer than the maximum width.
 	 */
 
 	if (options->stat_width == -1)
 		width = term_columns() - strlen(line_prefix);
 	else
 		width = options->stat_width ? options->stat_width : 80;
 	number_width = decimal_width(max_change) > number_width ?
 		decimal_width(max_change) : number_width;
 
 	if (options->stat_graph_width == -1)
 		options->stat_graph_width = diff_stat_graph_width;
 
 	/*
 	 * Guarantee 3/8*16==6 for the graph part
 	 * and 5/8*16==10 for the filename part
 	 */
 	if (width < 16 + 6 + number_width)
 		width = 16 + 6 + number_width;
 
 	/*
 	 * First assign sizes that are wanted, ignoring available width.
 	 * strlen("Bin XXX -> YYY bytes") == bin_width, and the part
 	 * starting from "XXX" should fit in graph_width.
 	 */
 	graph_width = max_change + 4 > bin_width ? max_change : bin_width - 4;
 	if (options->stat_graph_width &&
 	    options->stat_graph_width < graph_width)
 		graph_width = options->stat_graph_width;
 
 	name_width = (options->stat_name_width > 0 &&
 		      options->stat_name_width < max_len) ?
 		options->stat_name_width : max_len;
 
 	/*
 	 * Adjust adjustable widths not to exceed maximum width
 	 */
 	if (name_width + number_width + 6 + graph_width > width) {
 		if (graph_width > width * 3/8 - number_width - 6) {
 			graph_width = width * 3/8 - number_width - 6;
 			if (graph_width < 6)
 				graph_width = 6;
 		}
 
 		if (options->stat_graph_width &&
 		    graph_width > options->stat_graph_width)
 			graph_width = options->stat_graph_width;
 		if (name_width > width - number_width - 6 - graph_width)
 			name_width = width - number_width - 6 - graph_width;
 		else
 			graph_width = width - number_width - 6 - name_width;
 	}
 
 	/*
 	 * From here name_width is the width of the name area,
 	 * and graph_width is the width of the graph area.
 	 * max_change is used to scale graph properly.
 	 */
 	for (i = 0; i < count; i++) {
 		const char *prefix = "";
 		struct diffstat_file *file = data->files[i];
 		char *name = file->print_name;
 		uintmax_t added = file->added;
 		uintmax_t deleted = file->deleted;
 		int name_len;
 
 		if (!file->is_interesting && (added + deleted == 0))
 			continue;
 
 		/*
 		 * "scale" the filename
 		 */
 		len = name_width;
 		name_len = strlen(name);
 		if (name_width < name_len) {
 			char *slash;
 			prefix = "...";
 			len -= 3;
 			name += name_len - len;
 			slash = strchr(name, '/');
 			if (slash)
 				name = slash;
 		}
 
 		if (file->is_binary) {
 			strbuf_addf(&out, " %s%-*s |", prefix, len, name);
 			strbuf_addf(&out, " %*s", number_width, "Bin");
 			if (!added && !deleted) {
 				strbuf_addch(&out, '\n');
 				emit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,
 						 out.buf, out.len, 0);
 				strbuf_reset(&out);
 				continue;
 			}
 			strbuf_addf(&out, " %s%"PRIuMAX"%s",
 				del_c, deleted, reset);
 			strbuf_addstr(&out, " -> ");
 			strbuf_addf(&out, "%s%"PRIuMAX"%s",
 				add_c, added, reset);
 			strbuf_addstr(&out, " bytes\n");
 			emit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,
 					 out.buf, out.len, 0);
 			strbuf_reset(&out);
 			continue;
 		}
 		else if (file->is_unmerged) {
 			strbuf_addf(&out, " %s%-*s |", prefix, len, name);
 			strbuf_addstr(&out, " Unmerged\n");
 			emit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,
 					 out.buf, out.len, 0);
 			strbuf_reset(&out);
 			continue;
 		}
 
 		/*
 		 * scale the add/delete
 		 */
 		add = added;
 		del = deleted;
 
 		if (graph_width <= max_change) {
 			int total = scale_linear(add + del, graph_width, max_change);
 			if (total < 2 && add && del)
 				/* width >= 2 due to the sanity check */
 				total = 2;
 			if (add < del) {
 				add = scale_linear(add, graph_width, max_change);
 				del = total - add;
 			} else {
 				del = scale_linear(del, graph_width, max_change);
 				add = total - del;
 			}
 		}
 		strbuf_addf(&out, " %s%-*s |", prefix, len, name);
 		strbuf_addf(&out, " %*"PRIuMAX"%s",
 			number_width, added + deleted,
 			added + deleted ? " " : "");
 		show_graph(&out, '+', add, add_c, reset);
 		show_graph(&out, '-', del, del_c, reset);
 		strbuf_addch(&out, '\n');
 		emit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,
 				 out.buf, out.len, 0);
 		strbuf_reset(&out);
 	}
 
 	for (i = 0; i < data->nr; i++) {
 		struct diffstat_file *file = data->files[i];
 		uintmax_t added = file->added;
 		uintmax_t deleted = file->deleted;
 
 		if (file->is_unmerged ||
 		    (!file->is_interesting && (added + deleted == 0))) {
 			total_files--;
 			continue;
 		}
 
 		if (!file->is_binary) {
 			adds += added;
 			dels += deleted;
 		}
 		if (i < count)
 			continue;
 		if (!extra_shown)
 			emit_diff_symbol(options,
 					 DIFF_SYMBOL_STATS_SUMMARY_ABBREV,
 					 NULL, 0, 0);
 		extra_shown = 1;
 	}
 
 	print_stat_summary_inserts_deletes(options, total_files, adds, dels);
+	strbuf_release(&out);
 }
 
 static void show_shortstats(struct diffstat_t *data, struct diff_options *options)
-- 
2.14.1

[PATCH 13/34] help: release strbuf on error return in exec_man_konqueror()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/help.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/help.c b/builtin/help.c
index 334a8494ab..991a8bb16c 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -137,21 +137,22 @@ static void exec_woman_emacs(const char *path, const char *page)
 static void exec_man_konqueror(const char *path, const char *page)
 {
 	const char *display = getenv("DISPLAY");
 	if (display && *display) {
 		struct strbuf man_page = STRBUF_INIT;
 		const char *filename = "kfmclient";
 
 		/* It's simpler to launch konqueror using kfmclient. */
 		if (path) {
 			size_t len;
 			if (strip_suffix(path, "/konqueror", &len))
 				path = xstrfmt("%.*s/kfmclient", (int)len, path);
 			filename = basename((char *)path);
 		} else
 			path = "kfmclient";
 		strbuf_addf(&man_page, "man:%s(1)", page);
 		execlp(path, filename, "newTab", man_page.buf, (char *)NULL);
 		warning_errno(_("failed to exec '%s'"), path);
+		strbuf_release(&man_page);
 	}
 }
 
-- 
2.14.1

[PATCH 14/34] help: release strbuf on error return in exec_man_man()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/help.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/help.c b/builtin/help.c
index 991a8bb16c..12fb48933e 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -167,9 +167,10 @@ static void exec_man_man(const char *path, const char *page)
 static void exec_man_cmd(const char *cmd, const char *page)
 {
 	struct strbuf shell_cmd = STRBUF_INIT;
 	strbuf_addf(&shell_cmd, "%s %s", cmd, page);
 	execl(SHELL_PATH, SHELL_PATH, "-c", shell_cmd.buf, (char *)NULL);
 	warning(_("failed to exec '%s'"), cmd);
+	strbuf_release(&shell_cmd);
 }
 
 static void add_man_viewer(const char *name)
-- 
2.14.1

[PATCH 15/34] help: release strbuf on error return in exec_woman_emacs()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/help.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/help.c b/builtin/help.c
index 12fb48933e..b3f60a8f30 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -123,14 +123,15 @@ static int check_emacsclient_version(void)
 static void exec_woman_emacs(const char *path, const char *page)
 {
 	if (!check_emacsclient_version()) {
 		/* This works only with emacsclient version >= 22. */
 		struct strbuf man_page = STRBUF_INIT;
 
 		if (!path)
 			path = "emacsclient";
 		strbuf_addf(&man_page, "(woman \"%s\")", page);
 		execlp(path, "emacsclient", "-e", man_page.buf, (char *)NULL);
 		warning_errno(_("failed to exec '%s'"), path);
+		strbuf_release(&man_page);
 	}
 }
 
-- 
2.14.1

[PATCH 16/34] mailinfo: release strbuf after use in handle_from()

[Rene Scharfe]

Clean up at the end and jump there instead of returning early.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 mailinfo.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/mailinfo.c b/mailinfo.c
index bd574cb752..b1f5159546 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -138,66 +138,65 @@ static void unquote_quoted_pair(struct strbuf *line)
 static void handle_from(struct mailinfo *mi, const struct strbuf *from)
 {
 	char *at;
 	size_t el;
 	struct strbuf f;
 
 	strbuf_init(&f, from->len);
 	strbuf_addbuf(&f, from);
 
 	unquote_quoted_pair(&f);
 
 	at = strchr(f.buf, '@');
 	if (!at) {
 		parse_bogus_from(mi, from);
-		return;
+		goto out;
 	}
 
 	/*
 	 * If we already have one email, don't take any confusing lines
 	 */
-	if (mi->email.len && strchr(at + 1, '@')) {
-		strbuf_release(&f);
-		return;
-	}
+	if (mi->email.len && strchr(at + 1, '@'))
+		goto out;
 
 	/* Pick up the string around '@', possibly delimited with <>
 	 * pair; that is the email part.
 	 */
 	while (at > f.buf) {
 		char c = at[-1];
 		if (isspace(c))
 			break;
 		if (c == '<') {
 			at[-1] = ' ';
 			break;
 		}
 		at--;
 	}
 	el = strcspn(at, " \n\t\r\v\f>");
 	strbuf_reset(&mi->email);
 	strbuf_add(&mi->email, at, el);
 	strbuf_remove(&f, at - f.buf, el + (at[el] ? 1 : 0));
 
 	/* The remainder is name.  It could be
 	 *
 	 * - "John Doe <john.doe@xz>"			(a), or
 	 * - "john.doe@xz (John Doe)"			(b), or
 	 * - "John (zzz) Doe <john.doe@xz> (Comment)"	(c)
 	 *
 	 * but we have removed the email part, so
 	 *
 	 * - remove extra spaces which could stay after email (case 'c'), and
 	 * - trim from both ends, possibly removing the () pair at the end
 	 *   (cases 'a' and 'b').
 	 */
 	cleanup_space(&f);
 	strbuf_trim(&f);
 	if (f.buf[0] == '(' && f.len && f.buf[f.len - 1] == ')') {
 		strbuf_remove(&f, 0, 1);
 		strbuf_setlen(&f, f.len - 1);
 	}
 
 	get_sane_name(&mi->name, &f, &mi->email);
+out:
 	strbuf_release(&f);
 }
 
-- 
2.14.1

[PATCH 17/34] mailinfo: release strbuf on error return in handle_boundary()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 mailinfo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mailinfo.c b/mailinfo.c
index b1f5159546..f2387a3267 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -911,48 +911,49 @@ static int find_boundary(struct mailinfo *mi, struct strbuf *line)
 static int handle_boundary(struct mailinfo *mi, struct strbuf *line)
 {
 	struct strbuf newline = STRBUF_INIT;
 
 	strbuf_addch(&newline, '\n');
 again:
 	if (line->len >= (*(mi->content_top))->len + 2 &&
 	    !memcmp(line->buf + (*(mi->content_top))->len, "--", 2)) {
 		/* we hit an end boundary */
 		/* pop the current boundary off the stack */
 		strbuf_release(*(mi->content_top));
 		FREE_AND_NULL(*(mi->content_top));
 
 		/* technically won't happen as is_multipart_boundary()
 		   will fail first.  But just in case..
 		 */
 		if (--mi->content_top < mi->content) {
 			error("Detected mismatched boundaries, can't recover");
 			mi->input_error = -1;
 			mi->content_top = mi->content;
+			strbuf_release(&newline);
 			return 0;
 		}
 		handle_filter(mi, &newline);
 		strbuf_release(&newline);
 		if (mi->input_error)
 			return 0;
 
 		/* skip to the next boundary */
 		if (!find_boundary(mi, line))
 			return 0;
 		goto again;
 	}
 
 	/* set some defaults */
 	mi->transfer_encoding = TE_DONTCARE;
 	strbuf_reset(&mi->charset);
 
 	/* slurp in this section's info */
 	while (read_one_header_line(line, mi->input))
 		check_header(mi, line, mi->p_hdr_data, 0);
 
 	strbuf_release(&newline);
 	/* replenish line */
 	if (strbuf_getline_lf(line, mi->input))
 		return 0;
 	strbuf_addch(line, '\n');
 	return 1;
 }
-- 
2.14.1

Re: [PATCH 17/34] mailinfo: release strbuf on error return in handle_boundary()

[Martin Ågren]

On 30 August 2017 at 19:49, Rene Scharfe <l.s.r@web.de> wrote:
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  mailinfo.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/mailinfo.c b/mailinfo.c
> index b1f5159546..f2387a3267 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -911,48 +911,49 @@ static int find_boundary(struct mailinfo *mi, struct strbuf *line)
>  static int handle_boundary(struct mailinfo *mi, struct strbuf *line)
>  {
>         struct strbuf newline = STRBUF_INIT;
>
>         strbuf_addch(&newline, '\n');
>  again:
>         if (line->len >= (*(mi->content_top))->len + 2 &&
>             !memcmp(line->buf + (*(mi->content_top))->len, "--", 2)) {
>                 /* we hit an end boundary */
>                 /* pop the current boundary off the stack */
>                 strbuf_release(*(mi->content_top));
>                 FREE_AND_NULL(*(mi->content_top));
>
>                 /* technically won't happen as is_multipart_boundary()
>                    will fail first.  But just in case..
>                  */
>                 if (--mi->content_top < mi->content) {
>                         error("Detected mismatched boundaries, can't recover");
>                         mi->input_error = -1;
>                         mi->content_top = mi->content;
> +                       strbuf_release(&newline);
>                         return 0;
>                 }

Since this code path can't be taken (or so it says): How did you find
this and the others? Static analysis? Grepping around?

Re: [PATCH 17/34] mailinfo: release strbuf on error return in handle_boundary()

[René Scharfe]

Am 30.08.2017 um 20:23 schrieb Martin Ågren:
> On 30 August 2017 at 19:49, Rene Scharfe <l.s.r@web.de> wrote:
>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>> ---
>>   mailinfo.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/mailinfo.c b/mailinfo.c
>> index b1f5159546..f2387a3267 100644
>> --- a/mailinfo.c
>> +++ b/mailinfo.c
>> @@ -911,48 +911,49 @@ static int find_boundary(struct mailinfo *mi, struct strbuf *line)
>>   static int handle_boundary(struct mailinfo *mi, struct strbuf *line)
>>   {
>>          struct strbuf newline = STRBUF_INIT;
>>
>>          strbuf_addch(&newline, '\n');
>>   again:
>>          if (line->len >= (*(mi->content_top))->len + 2 &&
>>              !memcmp(line->buf + (*(mi->content_top))->len, "--", 2)) {
>>                  /* we hit an end boundary */
>>                  /* pop the current boundary off the stack */
>>                  strbuf_release(*(mi->content_top));
>>                  FREE_AND_NULL(*(mi->content_top));
>>
>>                  /* technically won't happen as is_multipart_boundary()
>>                     will fail first.  But just in case..
>>                   */
>>                  if (--mi->content_top < mi->content) {
>>                          error("Detected mismatched boundaries, can't recover");
>>                          mi->input_error = -1;
>>                          mi->content_top = mi->content;
>> +                       strbuf_release(&newline);
>>                          return 0;
>>                  }
> 
> Since this code path can't be taken (or so it says): How did you find
> this and the others? Static analysis? Grepping around?

Code inspection: I looked for functions with STRBUF_INIT that return
without calling strbuf_release() with "git grep -W STRBUF_INIT" and
searching for return in less(1).

René

Re: [PATCH 17/34] mailinfo: release strbuf on error return in handle_boundary()

[Martin Ågren]

On 31 August 2017 at 19:21, René Scharfe <l.s.r@web.de> wrote:
> Am 30.08.2017 um 20:23 schrieb Martin Ågren:
>> On 30 August 2017 at 19:49, Rene Scharfe <l.s.r@web.de> wrote:
>>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>>> ---
>>>   mailinfo.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/mailinfo.c b/mailinfo.c
>>> index b1f5159546..f2387a3267 100644
>>> --- a/mailinfo.c
>>> +++ b/mailinfo.c
>>> @@ -911,48 +911,49 @@ static int find_boundary(struct mailinfo *mi, struct strbuf *line)
>>>   static int handle_boundary(struct mailinfo *mi, struct strbuf *line)
>>>   {
>>>          struct strbuf newline = STRBUF_INIT;
>>>
>>>          strbuf_addch(&newline, '\n');
>>>   again:
>>>          if (line->len >= (*(mi->content_top))->len + 2 &&
>>>              !memcmp(line->buf + (*(mi->content_top))->len, "--", 2)) {
>>>                  /* we hit an end boundary */
>>>                  /* pop the current boundary off the stack */
>>>                  strbuf_release(*(mi->content_top));
>>>                  FREE_AND_NULL(*(mi->content_top));
>>>
>>>                  /* technically won't happen as is_multipart_boundary()
>>>                     will fail first.  But just in case..
>>>                   */
>>>                  if (--mi->content_top < mi->content) {
>>>                          error("Detected mismatched boundaries, can't recover");
>>>                          mi->input_error = -1;
>>>                          mi->content_top = mi->content;
>>> +                       strbuf_release(&newline);
>>>                          return 0;
>>>                  }
>>
>> Since this code path can't be taken (or so it says): How did you find
>> this and the others? Static analysis? Grepping around?
>
> Code inspection: I looked for functions with STRBUF_INIT that return
> without calling strbuf_release() with "git grep -W STRBUF_INIT" and
> searching for return in less(1).

Thanks for sharing.

I read through this series and thought it looked good. One thing I like
is that it doesn't just blindly apply some standard solution in all
patches, but always tries to do what it feels is "right" for each
particular function.

Martin

[PATCH 18/34] merge: release strbuf after use in save_state()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/merge.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 7df3fe3927..4f8418246b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -250,27 +250,31 @@ static void drop_save(void)
 static int save_state(struct object_id *stash)
 {
 	int len;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buffer = STRBUF_INIT;
 	const char *argv[] = {"stash", "create", NULL};
+	int rc = -1;
 
 	cp.argv = argv;
 	cp.out = -1;
 	cp.git_cmd = 1;
 
 	if (start_command(&cp))
 		die(_("could not run stash."));
 	len = strbuf_read(&buffer, cp.out, 1024);
 	close(cp.out);
 
 	if (finish_command(&cp) || len < 0)
 		die(_("stash failed"));
 	else if (!len)		/* no changes */
-		return -1;
+		goto out;
 	strbuf_setlen(&buffer, buffer.len-1);
 	if (get_oid(buffer.buf, stash))
 		die(_("not a valid object: %s"), buffer.buf);
-	return 0;
+	rc = 0;
+out:
+	strbuf_release(&buffer);
+	return rc;
 }
 
 static void read_empty(unsigned const char *sha1, int verbose)
-- 
2.14.1

[PATCH 19/34] merge: release strbuf after use in write_merge_heads()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/merge.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index 4f8418246b..7bc3fe4b6d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -920,24 +920,25 @@ static int setup_with_upstream(const char ***argv)
 static void write_merge_heads(struct commit_list *remoteheads)
 {
 	struct commit_list *j;
 	struct strbuf buf = STRBUF_INIT;
 
 	for (j = remoteheads; j; j = j->next) {
 		struct object_id *oid;
 		struct commit *c = j->item;
 		if (c->util && merge_remote_util(c)->obj) {
 			oid = &merge_remote_util(c)->obj->oid;
 		} else {
 			oid = &c->object.oid;
 		}
 		strbuf_addf(&buf, "%s\n", oid_to_hex(oid));
 	}
 	write_file_buf(git_path_merge_head(), buf.buf, buf.len);
 
 	strbuf_reset(&buf);
 	if (fast_forward == FF_NO)
 		strbuf_addstr(&buf, "no-ff");
 	write_file_buf(git_path_merge_mode(), buf.buf, buf.len);
+	strbuf_release(&buf);
 }
 
 static void write_merge_state(struct commit_list *remoteheads)
-- 
2.14.1

[PATCH 20/34] notes: release strbuf after use in notes_copy_from_stdin()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/notes.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/notes.c b/builtin/notes.c
index 4303848e04..8e54f2d146 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -278,56 +278,57 @@ static int parse_reedit_arg(const struct option *opt, const char *arg, int unset
 static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct notes_rewrite_cfg *c = NULL;
 	struct notes_tree *t = NULL;
 	int ret = 0;
 	const char *msg = "Notes added by 'git notes copy'";
 
 	if (rewrite_cmd) {
 		c = init_copy_notes_for_rewrite(rewrite_cmd);
 		if (!c)
 			return 0;
 	} else {
 		init_notes(NULL, NULL, NULL, NOTES_INIT_WRITABLE);
 		t = &default_notes_tree;
 	}
 
 	while (strbuf_getline_lf(&buf, stdin) != EOF) {
 		struct object_id from_obj, to_obj;
 		struct strbuf **split;
 		int err;
 
 		split = strbuf_split(&buf, ' ');
 		if (!split[0] || !split[1])
 			die(_("malformed input line: '%s'."), buf.buf);
 		strbuf_rtrim(split[0]);
 		strbuf_rtrim(split[1]);
 		if (get_oid(split[0]->buf, &from_obj))
 			die(_("failed to resolve '%s' as a valid ref."), split[0]->buf);
 		if (get_oid(split[1]->buf, &to_obj))
 			die(_("failed to resolve '%s' as a valid ref."), split[1]->buf);
 
 		if (rewrite_cmd)
 			err = copy_note_for_rewrite(c, &from_obj, &to_obj);
 		else
 			err = copy_note(t, &from_obj, &to_obj, force,
 					combine_notes_overwrite);
 
 		if (err) {
 			error(_("failed to copy notes from '%s' to '%s'"),
 			      split[0]->buf, split[1]->buf);
 			ret = 1;
 		}
 
 		strbuf_list_free(split);
 	}
 
 	if (!rewrite_cmd) {
 		commit_notes(t, msg);
 		free_notes(t);
 	} else {
 		finish_copy_notes_for_rewrite(c, msg);
 	}
+	strbuf_release(&buf);
 	return ret;
 }
 
-- 
2.14.1

[PATCH 02/34] am: release strbuf on error return in hg_patch_to_mail()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/am.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 3c50b03faa..3d38b3fe9f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -881,75 +881,84 @@ static int split_mail_stgit_series(struct am_state *state, const char **paths,
 static int hg_patch_to_mail(FILE *out, FILE *in, int keep_cr)
 {
 	struct strbuf sb = STRBUF_INIT;
+	int rc = 0;
 
 	while (!strbuf_getline_lf(&sb, in)) {
 		const char *str;
 
 		if (skip_prefix(sb.buf, "# User ", &str))
 			fprintf(out, "From: %s\n", str);
 		else if (skip_prefix(sb.buf, "# Date ", &str)) {
 			timestamp_t timestamp;
 			long tz, tz2;
 			char *end;
 
 			errno = 0;
 			timestamp = parse_timestamp(str, &end, 10);
-			if (errno)
-				return error(_("invalid timestamp"));
+			if (errno) {
+				rc = error(_("invalid timestamp"));
+				goto exit;
+			}
 
-			if (!skip_prefix(end, " ", &str))
-				return error(_("invalid Date line"));
+			if (!skip_prefix(end, " ", &str)) {
+				rc = error(_("invalid Date line"));
+				goto exit;
+			}
 
 			errno = 0;
 			tz = strtol(str, &end, 10);
-			if (errno)
-				return error(_("invalid timezone offset"));
+			if (errno) {
+				rc = error(_("invalid timezone offset"));
+				goto exit;
+			}
 
-			if (*end)
-				return error(_("invalid Date line"));
+			if (*end) {
+				rc = error(_("invalid Date line"));
+				goto exit;
+			}
 
 			/*
 			 * mercurial's timezone is in seconds west of UTC,
 			 * however git's timezone is in hours + minutes east of
 			 * UTC. Convert it.
 			 */
 			tz2 = labs(tz) / 3600 * 100 + labs(tz) % 3600 / 60;
 			if (tz > 0)
 				tz2 = -tz2;
 
 			fprintf(out, "Date: %s\n", show_date(timestamp, tz2, DATE_MODE(RFC2822)));
 		} else if (starts_with(sb.buf, "# ")) {
 			continue;
 		} else {
 			fprintf(out, "\n%s\n", sb.buf);
 			break;
 		}
 	}
 
 	strbuf_reset(&sb);
 	while (strbuf_fread(&sb, 8192, in) > 0) {
 		fwrite(sb.buf, 1, sb.len, out);
 		strbuf_reset(&sb);
 	}
-
+exit:
 	strbuf_release(&sb);
-	return 0;
+	return rc;
 }
 
 /**
  * Splits a list of files/directories into individual email patches. Each path
  * in `paths` must be a file/directory that is formatted according to
  * `patch_format`.
  *
  * Once split out, the individual email patches will be stored in the state
  * directory, with each patch's filename being its index, padded to state->prec
  * digits.
  *
  * state->cur will be set to the index of the first mail, and state->last will
  * be set to the index of the last mail.
  *
  * Set keep_cr to 0 to convert all lines ending with \r\n to end with \n, 1
  * to disable this behavior, -1 to use the default configured setting.
  *
  * Returns 0 on success, -1 on failure.
  */
-- 
2.14.1

[PATCH 03/34] am: release strbuf after use in safe_to_abort()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/am.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/am.c b/builtin/am.c
index 3d38b3fe9f..d7513f5375 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2095,29 +2095,30 @@ static void am_skip(struct am_state *state)
 static int safe_to_abort(const struct am_state *state)
 {
 	struct strbuf sb = STRBUF_INIT;
 	struct object_id abort_safety, head;
 
 	if (file_exists(am_path(state, "dirtyindex")))
 		return 0;
 
 	if (read_state_file(&sb, state, "abort-safety", 1) > 0) {
 		if (get_oid_hex(sb.buf, &abort_safety))
 			die(_("could not parse %s"), am_path(state, "abort-safety"));
 	} else
 		oidclr(&abort_safety);
+	strbuf_release(&sb);
 
 	if (get_oid("HEAD", &head))
 		oidclr(&head);
 
 	if (!oidcmp(&head, &abort_safety))
 		return 1;
 
 	warning(_("You seem to have moved HEAD since the last 'am' failure.\n"
 		"Not rewinding to ORIG_HEAD"));
 
 	return 0;
 }
 
 /**
  * Aborts the current am session if it is safe to do so.
  */
-- 
2.14.1

[PATCH 04/34] check-ref-format: release strbuf after use in check_ref_format_branch()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/check-ref-format.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index eac499450f..6c40ff110b 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -39,12 +39,13 @@ static char *collapse_slashes(const char *refname)
 static int check_ref_format_branch(const char *arg)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int nongit;
 
 	setup_git_directory_gently(&nongit);
 	if (strbuf_check_branch_ref(&sb, arg))
 		die("'%s' is not a valid branch name", arg);
 	printf("%s\n", sb.buf + 11);
+	strbuf_release(&sb);
 	return 0;
 }
 
-- 
2.14.1

[PATCH 08/34] connect: release strbuf on error return in git_connect()

[Rene Scharfe]

Reduce the scope of the variable cmd and release it before returning
early.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 connect.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index 49b28b83be..df56c0cbff 100644
--- a/connect.c
+++ b/connect.c
@@ -775,146 +775,148 @@ static void handle_ssh_variant(const char *ssh_command, int is_cmdline,
 struct child_process *git_connect(int fd[2], const char *url,
 				  const char *prog, int flags)
 {
 	char *hostandport, *path;
 	struct child_process *conn = &no_fork;
 	enum protocol protocol;
-	struct strbuf cmd = STRBUF_INIT;
 
 	/* Without this we cannot rely on waitpid() to tell
 	 * what happened to our children.
 	 */
 	signal(SIGCHLD, SIG_DFL);
 
 	protocol = parse_connect_url(url, &hostandport, &path);
 	if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
 		printf("Diag: url=%s\n", url ? url : "NULL");
 		printf("Diag: protocol=%s\n", prot_name(protocol));
 		printf("Diag: hostandport=%s\n", hostandport ? hostandport : "NULL");
 		printf("Diag: path=%s\n", path ? path : "NULL");
 		conn = NULL;
 	} else if (protocol == PROTO_GIT) {
 		/*
 		 * Set up virtual host information based on where we will
 		 * connect, unless the user has overridden us in
 		 * the environment.
 		 */
 		char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
 		if (target_host)
 			target_host = xstrdup(target_host);
 		else
 			target_host = xstrdup(hostandport);
 
 		transport_check_allowed("git");
 
 		/* These underlying connection commands die() if they
 		 * cannot connect.
 		 */
 		if (git_use_proxy(hostandport))
 			conn = git_proxy_connect(fd, hostandport);
 		else
 			git_tcp_connect(fd, hostandport, flags);
 		/*
 		 * Separate original protocol components prog and path
 		 * from extended host header with a NUL byte.
 		 *
 		 * Note: Do not add any other headers here!  Doing so
 		 * will cause older git-daemon servers to crash.
 		 */
 		packet_write_fmt(fd[1],
 			     "%s %s%chost=%s%c",
 			     prog, path, 0,
 			     target_host, 0);
 		free(target_host);
 	} else {
+		struct strbuf cmd = STRBUF_INIT;
+
 		conn = xmalloc(sizeof(*conn));
 		child_process_init(conn);
 
 		if (looks_like_command_line_option(path))
 			die("strange pathname '%s' blocked", path);
 
 		strbuf_addstr(&cmd, prog);
 		strbuf_addch(&cmd, ' ');
 		sq_quote_buf(&cmd, path);
 
 		/* remove repo-local variables from the environment */
 		conn->env = local_repo_env;
 		conn->use_shell = 1;
 		conn->in = conn->out = -1;
 		if (protocol == PROTO_SSH) {
 			const char *ssh;
 			int needs_batch = 0;
 			int port_option = 'p';
 			char *ssh_host = hostandport;
 			const char *port = NULL;
 			transport_check_allowed("ssh");
 			get_host_and_port(&ssh_host, &port);
 
 			if (!port)
 				port = get_port(ssh_host);
 
 			if (flags & CONNECT_DIAG_URL) {
 				printf("Diag: url=%s\n", url ? url : "NULL");
 				printf("Diag: protocol=%s\n", prot_name(protocol));
 				printf("Diag: userandhost=%s\n", ssh_host ? ssh_host : "NULL");
 				printf("Diag: port=%s\n", port ? port : "NONE");
 				printf("Diag: path=%s\n", path ? path : "NULL");
 
 				free(hostandport);
 				free(path);
 				free(conn);
+				strbuf_release(&cmd);
 				return NULL;
 			}
 
 			if (looks_like_command_line_option(ssh_host))
 				die("strange hostname '%s' blocked", ssh_host);
 
 			ssh = get_ssh_command();
 			if (ssh)
 				handle_ssh_variant(ssh, 1, &port_option,
 						   &needs_batch);
 			else {
 				/*
 				 * GIT_SSH is the no-shell version of
 				 * GIT_SSH_COMMAND (and must remain so for
 				 * historical compatibility).
 				 */
 				conn->use_shell = 0;
 
 				ssh = getenv("GIT_SSH");
 				if (!ssh)
 					ssh = "ssh";
 				else
 					handle_ssh_variant(ssh, 0,
 							   &port_option,
 							   &needs_batch);
 			}
 
 			argv_array_push(&conn->args, ssh);
 			if (flags & CONNECT_IPV4)
 				argv_array_push(&conn->args, "-4");
 			else if (flags & CONNECT_IPV6)
 				argv_array_push(&conn->args, "-6");
 			if (needs_batch)
 				argv_array_push(&conn->args, "-batch");
 			if (port) {
 				argv_array_pushf(&conn->args,
 						 "-%c", port_option);
 				argv_array_push(&conn->args, port);
 			}
 			argv_array_push(&conn->args, ssh_host);
 		} else {
 			transport_check_allowed("file");
 		}
 		argv_array_push(&conn->args, cmd.buf);
 
 		if (start_command(conn))
 			die("unable to fork");
 
 		fd[0] = conn->out; /* read from child's stdout */
 		fd[1] = conn->in;  /* write to child's stdin */
 		strbuf_release(&cmd);
 	}
 	free(hostandport);
 	free(path);
 	return conn;
 }
-- 
2.14.1

[PATCH 09/34] convert: release strbuf on error return in filter_buffer_or_fd()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 convert.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/convert.c b/convert.c
index c5f0b21037..4a0ed8d3cb 100644
--- a/convert.c
+++ b/convert.c
@@ -393,63 +393,65 @@ struct filter_params {
 static int filter_buffer_or_fd(int in, int out, void *data)
 {
 	/*
 	 * Spawn cmd and feed the buffer contents through its stdin.
 	 */
 	struct child_process child_process = CHILD_PROCESS_INIT;
 	struct filter_params *params = (struct filter_params *)data;
 	int write_err, status;
 	const char *argv[] = { NULL, NULL };
 
 	/* apply % substitution to cmd */
 	struct strbuf cmd = STRBUF_INIT;
 	struct strbuf path = STRBUF_INIT;
 	struct strbuf_expand_dict_entry dict[] = {
 		{ "f", NULL, },
 		{ NULL, NULL, },
 	};
 
 	/* quote the path to preserve spaces, etc. */
 	sq_quote_buf(&path, params->path);
 	dict[0].value = path.buf;
 
 	/* expand all %f with the quoted path */
 	strbuf_expand(&cmd, params->cmd, strbuf_expand_dict_cb, &dict);
 	strbuf_release(&path);
 
 	argv[0] = cmd.buf;
 
 	child_process.argv = argv;
 	child_process.use_shell = 1;
 	child_process.in = -1;
 	child_process.out = out;
 
-	if (start_command(&child_process))
+	if (start_command(&child_process)) {
+		strbuf_release(&cmd);
 		return error("cannot fork to run external filter '%s'", params->cmd);
+	}
 
 	sigchain_push(SIGPIPE, SIG_IGN);
 
 	if (params->src) {
 		write_err = (write_in_full(child_process.in,
 					   params->src, params->size) < 0);
 		if (errno == EPIPE)
 			write_err = 0;
 	} else {
 		write_err = copy_fd(params->fd, child_process.in);
 		if (write_err == COPY_WRITE_ERROR && errno == EPIPE)
 			write_err = 0;
 	}
 
 	if (close(child_process.in))
 		write_err = 1;
 	if (write_err)
 		error("cannot feed the input to external filter '%s'", params->cmd);
 
 	sigchain_pop(SIGPIPE);
 
 	status = finish_command(&child_process);
 	if (status)
 		error("external filter '%s' failed %d", params->cmd, status);
 
 	strbuf_release(&cmd);
 	return (write_err || status);
 }
-- 
2.14.1

[PATCH 11/34] diff: release strbuf after use in show_rename_copy()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 diff.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/diff.c b/diff.c
index 4148ba6980..33c65f492d 100644
--- a/diff.c
+++ b/diff.c
@@ -5281,14 +5281,15 @@ static void show_mode_change(struct diff_options *opt, struct diff_filepair *p,
 static void show_rename_copy(struct diff_options *opt, const char *renamecopy,
 		struct diff_filepair *p)
 {
 	struct strbuf sb = STRBUF_INIT;
 	char *names = pprint_rename(p->one->path, p->two->path);
 	strbuf_addf(&sb, " %s %s (%d%%)\n",
 			renamecopy, names, similarity_index(p));
 	free(names);
 	emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,
 				 sb.buf, sb.len, 0);
 	show_mode_change(opt, p, 0);
+	strbuf_release(&sb);
 }
 
 static void diff_summary(struct diff_options *opt, struct diff_filepair *p)
-- 
2.14.1

[PATCH 12/34] diff: release strbuf after use in show_stats()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 diff.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/diff.c b/diff.c
index 33c65f492d..64cdcf2331 100644
--- a/diff.c
+++ b/diff.c
@@ -2334,255 +2334,256 @@ void print_stat_summary(FILE *fp, int files,
 static void show_stats(struct diffstat_t *data, struct diff_options *options)
 {
 	int i, len, add, del, adds = 0, dels = 0;
 	uintmax_t max_change = 0, max_len = 0;
 	int total_files = data->nr, count;
 	int width, name_width, graph_width, number_width = 0, bin_width = 0;
 	const char *reset, *add_c, *del_c;
 	int extra_shown = 0;
 	const char *line_prefix = diff_line_prefix(options);
 	struct strbuf out = STRBUF_INIT;
 
 	if (data->nr == 0)
 		return;
 
 	count = options->stat_count ? options->stat_count : data->nr;
 
 	reset = diff_get_color_opt(options, DIFF_RESET);
 	add_c = diff_get_color_opt(options, DIFF_FILE_NEW);
 	del_c = diff_get_color_opt(options, DIFF_FILE_OLD);
 
 	/*
 	 * Find the longest filename and max number of changes
 	 */
 	for (i = 0; (i < count) && (i < data->nr); i++) {
 		struct diffstat_file *file = data->files[i];
 		uintmax_t change = file->added + file->deleted;
 
 		if (!file->is_interesting && (change == 0)) {
 			count++; /* not shown == room for one more */
 			continue;
 		}
 		fill_print_name(file);
 		len = strlen(file->print_name);
 		if (max_len < len)
 			max_len = len;
 
 		if (file->is_unmerged) {
 			/* "Unmerged" is 8 characters */
 			bin_width = bin_width < 8 ? 8 : bin_width;
 			continue;
 		}
 		if (file->is_binary) {
 			/* "Bin XXX -> YYY bytes" */
 			int w = 14 + decimal_width(file->added)
 				+ decimal_width(file->deleted);
 			bin_width = bin_width < w ? w : bin_width;
 			/* Display change counts aligned with "Bin" */
 			number_width = 3;
 			continue;
 		}
 
 		if (max_change < change)
 			max_change = change;
 	}
 	count = i; /* where we can stop scanning in data->files[] */
 
 	/*
 	 * We have width = stat_width or term_columns() columns total.
 	 * We want a maximum of min(max_len, stat_name_width) for the name part.
 	 * We want a maximum of min(max_change, stat_graph_width) for the +- part.
 	 * We also need 1 for " " and 4 + decimal_width(max_change)
 	 * for " | NNNN " and one the empty column at the end, altogether
 	 * 6 + decimal_width(max_change).
 	 *
 	 * If there's not enough space, we will use the smaller of
 	 * stat_name_width (if set) and 5/8*width for the filename,
 	 * and the rest for constant elements + graph part, but no more
 	 * than stat_graph_width for the graph part.
 	 * (5/8 gives 50 for filename and 30 for the constant parts + graph
 	 * for the standard terminal size).
 	 *
 	 * In other words: stat_width limits the maximum width, and
 	 * stat_name_width fixes the maximum width of the filename,
 	 * and is also used to divide available columns if there
 	 * aren't enough.
 	 *
 	 * Binary files are displayed with "Bin XXX -> YYY bytes"
 	 * instead of the change count and graph. This part is treated
 	 * similarly to the graph part, except that it is not
 	 * "scaled". If total width is too small to accommodate the
 	 * guaranteed minimum width of the filename part and the
 	 * separators and this message, this message will "overflow"
 	 * making the line longer than the maximum width.
 	 */
 
 	if (options->stat_width == -1)
 		width = term_columns() - strlen(line_prefix);
 	else
 		width = options->stat_width ? options->stat_width : 80;
 	number_width = decimal_width(max_change) > number_width ?
 		decimal_width(max_change) : number_width;
 
 	if (options->stat_graph_width == -1)
 		options->stat_graph_width = diff_stat_graph_width;
 
 	/*
 	 * Guarantee 3/8*16==6 for the graph part
 	 * and 5/8*16==10 for the filename part
 	 */
 	if (width < 16 + 6 + number_width)
 		width = 16 + 6 + number_width;
 
 	/*
 	 * First assign sizes that are wanted, ignoring available width.
 	 * strlen("Bin XXX -> YYY bytes") == bin_width, and the part
 	 * starting from "XXX" should fit in graph_width.
 	 */
 	graph_width = max_change + 4 > bin_width ? max_change : bin_width - 4;
 	if (options->stat_graph_width &&
 	    options->stat_graph_width < graph_width)
 		graph_width = options->stat_graph_width;
 
 	name_width = (options->stat_name_width > 0 &&
 		      options->stat_name_width < max_len) ?
 		options->stat_name_width : max_len;
 
 	/*
 	 * Adjust adjustable widths not to exceed maximum width
 	 */
 	if (name_width + number_width + 6 + graph_width > width) {
 		if (graph_width > width * 3/8 - number_width - 6) {
 			graph_width = width * 3/8 - number_width - 6;
 			if (graph_width < 6)
 				graph_width = 6;
 		}
 
 		if (options->stat_graph_width &&
 		    graph_width > options->stat_graph_width)
 			graph_width = options->stat_graph_width;
 		if (name_width > width - number_width - 6 - graph_width)
 			name_width = width - number_width - 6 - graph_width;
 		else
 			graph_width = width - number_width - 6 - name_width;
 	}
 
 	/*
 	 * From here name_width is the width of the name area,
 	 * and graph_width is the width of the graph area.
 	 * max_change is used to scale graph properly.
 	 */
 	for (i = 0; i < count; i++) {
 		const char *prefix = "";
 		struct diffstat_file *file = data->files[i];
 		char *name = file->print_name;
 		uintmax_t added = file->added;
 		uintmax_t deleted = file->deleted;
 		int name_len;
 
 		if (!file->is_interesting && (added + deleted == 0))
 			continue;
 
 		/*
 		 * "scale" the filename
 		 */
 		len = name_width;
 		name_len = strlen(name);
 		if (name_width < name_len) {
 			char *slash;
 			prefix = "...";
 			len -= 3;
 			name += name_len - len;
 			slash = strchr(name, '/');
 			if (slash)
 				name = slash;
 		}
 
 		if (file->is_binary) {
 			strbuf_addf(&out, " %s%-*s |", prefix, len, name);
 			strbuf_addf(&out, " %*s", number_width, "Bin");
 			if (!added && !deleted) {
 				strbuf_addch(&out, '\n');
 				emit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,
 						 out.buf, out.len, 0);
 				strbuf_reset(&out);
 				continue;
 			}
 			strbuf_addf(&out, " %s%"PRIuMAX"%s",
 				del_c, deleted, reset);
 			strbuf_addstr(&out, " -> ");
 			strbuf_addf(&out, "%s%"PRIuMAX"%s",
 				add_c, added, reset);
 			strbuf_addstr(&out, " bytes\n");
 			emit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,
 					 out.buf, out.len, 0);
 			strbuf_reset(&out);
 			continue;
 		}
 		else if (file->is_unmerged) {
 			strbuf_addf(&out, " %s%-*s |", prefix, len, name);
 			strbuf_addstr(&out, " Unmerged\n");
 			emit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,
 					 out.buf, out.len, 0);
 			strbuf_reset(&out);
 			continue;
 		}
 
 		/*
 		 * scale the add/delete
 		 */
 		add = added;
 		del = deleted;
 
 		if (graph_width <= max_change) {
 			int total = scale_linear(add + del, graph_width, max_change);
 			if (total < 2 && add && del)
 				/* width >= 2 due to the sanity check */
 				total = 2;
 			if (add < del) {
 				add = scale_linear(add, graph_width, max_change);
 				del = total - add;
 			} else {
 				del = scale_linear(del, graph_width, max_change);
 				add = total - del;
 			}
 		}
 		strbuf_addf(&out, " %s%-*s |", prefix, len, name);
 		strbuf_addf(&out, " %*"PRIuMAX"%s",
 			number_width, added + deleted,
 			added + deleted ? " " : "");
 		show_graph(&out, '+', add, add_c, reset);
 		show_graph(&out, '-', del, del_c, reset);
 		strbuf_addch(&out, '\n');
 		emit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,
 				 out.buf, out.len, 0);
 		strbuf_reset(&out);
 	}
 
 	for (i = 0; i < data->nr; i++) {
 		struct diffstat_file *file = data->files[i];
 		uintmax_t added = file->added;
 		uintmax_t deleted = file->deleted;
 
 		if (file->is_unmerged ||
 		    (!file->is_interesting && (added + deleted == 0))) {
 			total_files--;
 			continue;
 		}
 
 		if (!file->is_binary) {
 			adds += added;
 			dels += deleted;
 		}
 		if (i < count)
 			continue;
 		if (!extra_shown)
 			emit_diff_symbol(options,
 					 DIFF_SYMBOL_STATS_SUMMARY_ABBREV,
 					 NULL, 0, 0);
 		extra_shown = 1;
 	}
 
 	print_stat_summary_inserts_deletes(options, total_files, adds, dels);
+	strbuf_release(&out);
 }
 
 static void show_shortstats(struct diffstat_t *data, struct diff_options *options)
-- 
2.14.1

[PATCH 21/34] refs: release strbuf on error return in write_pseudoref()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index b0106b8162..d8dc86b1f5 100644
--- a/refs.c
+++ b/refs.c
@@ -597,45 +597,45 @@ long get_files_ref_lock_timeout_ms(void)
 static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
 			   const unsigned char *old_sha1, struct strbuf *err)
 {
 	const char *filename;
 	int fd;
 	static struct lock_file lock;
 	struct strbuf buf = STRBUF_INIT;
 	int ret = -1;
 
 	strbuf_addf(&buf, "%s\n", sha1_to_hex(sha1));
 
 	filename = git_path("%s", pseudoref);
 	fd = hold_lock_file_for_update_timeout(&lock, filename,
 					       LOCK_DIE_ON_ERROR,
 					       get_files_ref_lock_timeout_ms());
 	if (fd < 0) {
 		strbuf_addf(err, "could not open '%s' for writing: %s",
 			    filename, strerror(errno));
-		return -1;
+		goto done;
 	}
 
 	if (old_sha1) {
 		unsigned char actual_old_sha1[20];
 
 		if (read_ref(pseudoref, actual_old_sha1))
 			die("could not read ref '%s'", pseudoref);
 		if (hashcmp(actual_old_sha1, old_sha1)) {
 			strbuf_addf(err, "unexpected sha1 when writing '%s'", pseudoref);
 			rollback_lock_file(&lock);
 			goto done;
 		}
 	}
 
 	if (write_in_full(fd, buf.buf, buf.len) != buf.len) {
 		strbuf_addf(err, "could not write to '%s'", filename);
 		rollback_lock_file(&lock);
 		goto done;
 	}
 
 	commit_lock_file(&lock);
 	ret = 0;
 done:
 	strbuf_release(&buf);
 	return ret;
 }
-- 
2.14.1

[PATCH 08/34] connect: release strbuf on error return in git_connect()

[Rene Scharfe]

Reduce the scope of the variable cmd and release it before returning
early.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 connect.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index 49b28b83be..df56c0cbff 100644
--- a/connect.c
+++ b/connect.c
@@ -775,146 +775,148 @@ static void handle_ssh_variant(const char *ssh_command, int is_cmdline,
 struct child_process *git_connect(int fd[2], const char *url,
 				  const char *prog, int flags)
 {
 	char *hostandport, *path;
 	struct child_process *conn = &no_fork;
 	enum protocol protocol;
-	struct strbuf cmd = STRBUF_INIT;
 
 	/* Without this we cannot rely on waitpid() to tell
 	 * what happened to our children.
 	 */
 	signal(SIGCHLD, SIG_DFL);
 
 	protocol = parse_connect_url(url, &hostandport, &path);
 	if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
 		printf("Diag: url=%s\n", url ? url : "NULL");
 		printf("Diag: protocol=%s\n", prot_name(protocol));
 		printf("Diag: hostandport=%s\n", hostandport ? hostandport : "NULL");
 		printf("Diag: path=%s\n", path ? path : "NULL");
 		conn = NULL;
 	} else if (protocol == PROTO_GIT) {
 		/*
 		 * Set up virtual host information based on where we will
 		 * connect, unless the user has overridden us in
 		 * the environment.
 		 */
 		char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
 		if (target_host)
 			target_host = xstrdup(target_host);
 		else
 			target_host = xstrdup(hostandport);
 
 		transport_check_allowed("git");
 
 		/* These underlying connection commands die() if they
 		 * cannot connect.
 		 */
 		if (git_use_proxy(hostandport))
 			conn = git_proxy_connect(fd, hostandport);
 		else
 			git_tcp_connect(fd, hostandport, flags);
 		/*
 		 * Separate original protocol components prog and path
 		 * from extended host header with a NUL byte.
 		 *
 		 * Note: Do not add any other headers here!  Doing so
 		 * will cause older git-daemon servers to crash.
 		 */
 		packet_write_fmt(fd[1],
 			     "%s %s%chost=%s%c",
 			     prog, path, 0,
 			     target_host, 0);
 		free(target_host);
 	} else {
+		struct strbuf cmd = STRBUF_INIT;
+
 		conn = xmalloc(sizeof(*conn));
 		child_process_init(conn);
 
 		if (looks_like_command_line_option(path))
 			die("strange pathname '%s' blocked", path);
 
 		strbuf_addstr(&cmd, prog);
 		strbuf_addch(&cmd, ' ');
 		sq_quote_buf(&cmd, path);
 
 		/* remove repo-local variables from the environment */
 		conn->env = local_repo_env;
 		conn->use_shell = 1;
 		conn->in = conn->out = -1;
 		if (protocol == PROTO_SSH) {
 			const char *ssh;
 			int needs_batch = 0;
 			int port_option = 'p';
 			char *ssh_host = hostandport;
 			const char *port = NULL;
 			transport_check_allowed("ssh");
 			get_host_and_port(&ssh_host, &port);
 
 			if (!port)
 				port = get_port(ssh_host);
 
 			if (flags & CONNECT_DIAG_URL) {
 				printf("Diag: url=%s\n", url ? url : "NULL");
 				printf("Diag: protocol=%s\n", prot_name(protocol));
 				printf("Diag: userandhost=%s\n", ssh_host ? ssh_host : "NULL");
 				printf("Diag: port=%s\n", port ? port : "NONE");
 				printf("Diag: path=%s\n", path ? path : "NULL");
 
 				free(hostandport);
 				free(path);
 				free(conn);
+				strbuf_release(&cmd);
 				return NULL;
 			}
 
 			if (looks_like_command_line_option(ssh_host))
 				die("strange hostname '%s' blocked", ssh_host);
 
 			ssh = get_ssh_command();
 			if (ssh)
 				handle_ssh_variant(ssh, 1, &port_option,
 						   &needs_batch);
 			else {
 				/*
 				 * GIT_SSH is the no-shell version of
 				 * GIT_SSH_COMMAND (and must remain so for
 				 * historical compatibility).
 				 */
 				conn->use_shell = 0;
 
 				ssh = getenv("GIT_SSH");
 				if (!ssh)
 					ssh = "ssh";
 				else
 					handle_ssh_variant(ssh, 0,
 							   &port_option,
 							   &needs_batch);
 			}
 
 			argv_array_push(&conn->args, ssh);
 			if (flags & CONNECT_IPV4)
 				argv_array_push(&conn->args, "-4");
 			else if (flags & CONNECT_IPV6)
 				argv_array_push(&conn->args, "-6");
 			if (needs_batch)
 				argv_array_push(&conn->args, "-batch");
 			if (port) {
 				argv_array_pushf(&conn->args,
 						 "-%c", port_option);
 				argv_array_push(&conn->args, port);
 			}
 			argv_array_push(&conn->args, ssh_host);
 		} else {
 			transport_check_allowed("file");
 		}
 		argv_array_push(&conn->args, cmd.buf);
 
 		if (start_command(conn))
 			die("unable to fork");
 
 		fd[0] = conn->out; /* read from child's stdout */
 		fd[1] = conn->in;  /* write to child's stdin */
 		strbuf_release(&cmd);
 	}
 	free(hostandport);
 	free(path);
 	return conn;
 }
-- 
2.14.1

[PATCH 21/34] refs: release strbuf on error return in write_pseudoref()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index b0106b8162..d8dc86b1f5 100644
--- a/refs.c
+++ b/refs.c
@@ -597,45 +597,45 @@ long get_files_ref_lock_timeout_ms(void)
 static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
 			   const unsigned char *old_sha1, struct strbuf *err)
 {
 	const char *filename;
 	int fd;
 	static struct lock_file lock;
 	struct strbuf buf = STRBUF_INIT;
 	int ret = -1;
 
 	strbuf_addf(&buf, "%s\n", sha1_to_hex(sha1));
 
 	filename = git_path("%s", pseudoref);
 	fd = hold_lock_file_for_update_timeout(&lock, filename,
 					       LOCK_DIE_ON_ERROR,
 					       get_files_ref_lock_timeout_ms());
 	if (fd < 0) {
 		strbuf_addf(err, "could not open '%s' for writing: %s",
 			    filename, strerror(errno));
-		return -1;
+		goto done;
 	}
 
 	if (old_sha1) {
 		unsigned char actual_old_sha1[20];
 
 		if (read_ref(pseudoref, actual_old_sha1))
 			die("could not read ref '%s'", pseudoref);
 		if (hashcmp(actual_old_sha1, old_sha1)) {
 			strbuf_addf(err, "unexpected sha1 when writing '%s'", pseudoref);
 			rollback_lock_file(&lock);
 			goto done;
 		}
 	}
 
 	if (write_in_full(fd, buf.buf, buf.len) != buf.len) {
 		strbuf_addf(err, "could not write to '%s'", filename);
 		rollback_lock_file(&lock);
 		goto done;
 	}
 
 	commit_lock_file(&lock);
 	ret = 0;
 done:
 	strbuf_release(&buf);
 	return ret;
 }
-- 
2.14.1

[PATCH 22/34] remote: release strbuf after use in read_remote_branches()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/remote.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/remote.c b/builtin/remote.c
index a995ea86c1..d0bf999abf 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -554,23 +554,24 @@ struct rename_info {
 static int read_remote_branches(const char *refname,
 	const struct object_id *oid, int flags, void *cb_data)
 {
 	struct rename_info *rename = cb_data;
 	struct strbuf buf = STRBUF_INIT;
 	struct string_list_item *item;
 	int flag;
 	struct object_id orig_oid;
 	const char *symref;
 
 	strbuf_addf(&buf, "refs/remotes/%s/", rename->old);
 	if (starts_with(refname, buf.buf)) {
 		item = string_list_append(rename->remote_branches, xstrdup(refname));
 		symref = resolve_ref_unsafe(refname, RESOLVE_REF_READING,
 					    orig_oid.hash, &flag);
 		if (flag & REF_ISSYMREF)
 			item->util = xstrdup(symref);
 		else
 			item->util = NULL;
 	}
+	strbuf_release(&buf);
 
 	return 0;
 }
-- 
2.14.1

[PATCH 23/34] remote: release strbuf after use in migrate_file()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/remote.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/remote.c b/builtin/remote.c
index d0bf999abf..0a56d7da66 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -579,23 +579,24 @@ static int read_remote_branches(const char *refname,
 static int migrate_file(struct remote *remote)
 {
 	struct strbuf buf = STRBUF_INIT;
 	int i;
 
 	strbuf_addf(&buf, "remote.%s.url", remote->name);
 	for (i = 0; i < remote->url_nr; i++)
 		git_config_set_multivar(buf.buf, remote->url[i], "^$", 0);
 	strbuf_reset(&buf);
 	strbuf_addf(&buf, "remote.%s.push", remote->name);
 	for (i = 0; i < remote->push_refspec_nr; i++)
 		git_config_set_multivar(buf.buf, remote->push_refspec[i], "^$", 0);
 	strbuf_reset(&buf);
 	strbuf_addf(&buf, "remote.%s.fetch", remote->name);
 	for (i = 0; i < remote->fetch_refspec_nr; i++)
 		git_config_set_multivar(buf.buf, remote->fetch_refspec[i], "^$", 0);
 	if (remote->origin == REMOTE_REMOTES)
 		unlink_or_warn(git_path("remotes/%s", remote->name));
 	else if (remote->origin == REMOTE_BRANCHES)
 		unlink_or_warn(git_path("branches/%s", remote->name));
+	strbuf_release(&buf);
 
 	return 0;
 }
-- 
2.14.1

[PATCH 24/34] remote: release strbuf after use in set_url()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/remote.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 0a56d7da66..33ba739332 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1509,87 +1509,87 @@ static int get_url(int argc, const char **argv)
 static int set_url(int argc, const char **argv)
 {
 	int i, push_mode = 0, add_mode = 0, delete_mode = 0;
 	int matches = 0, negative_matches = 0;
 	const char *remotename = NULL;
 	const char *newurl = NULL;
 	const char *oldurl = NULL;
 	struct remote *remote;
 	regex_t old_regex;
 	const char **urlset;
 	int urlset_nr;
 	struct strbuf name_buf = STRBUF_INIT;
 	struct option options[] = {
 		OPT_BOOL('\0', "push", &push_mode,
 			 N_("manipulate push URLs")),
 		OPT_BOOL('\0', "add", &add_mode,
 			 N_("add URL")),
 		OPT_BOOL('\0', "delete", &delete_mode,
 			    N_("delete URLs")),
 		OPT_END()
 	};
 	argc = parse_options(argc, argv, NULL, options, builtin_remote_seturl_usage,
 			     PARSE_OPT_KEEP_ARGV0);
 
 	if (add_mode && delete_mode)
 		die(_("--add --delete doesn't make sense"));
 
 	if (argc < 3 || argc > 4 || ((add_mode || delete_mode) && argc != 3))
 		usage_with_options(builtin_remote_seturl_usage, options);
 
 	remotename = argv[1];
 	newurl = argv[2];
 	if (argc > 3)
 		oldurl = argv[3];
 
 	if (delete_mode)
 		oldurl = newurl;
 
 	remote = remote_get(remotename);
 	if (!remote_is_configured(remote, 1))
 		die(_("No such remote '%s'"), remotename);
 
 	if (push_mode) {
 		strbuf_addf(&name_buf, "remote.%s.pushurl", remotename);
 		urlset = remote->pushurl;
 		urlset_nr = remote->pushurl_nr;
 	} else {
 		strbuf_addf(&name_buf, "remote.%s.url", remotename);
 		urlset = remote->url;
 		urlset_nr = remote->url_nr;
 	}
 
 	/* Special cases that add new entry. */
 	if ((!oldurl && !delete_mode) || add_mode) {
 		if (add_mode)
 			git_config_set_multivar(name_buf.buf, newurl,
 						       "^$", 0);
 		else
 			git_config_set(name_buf.buf, newurl);
-		strbuf_release(&name_buf);
-
-		return 0;
+		goto out;
 	}
 
 	/* Old URL specified. Demand that one matches. */
 	if (regcomp(&old_regex, oldurl, REG_EXTENDED))
 		die(_("Invalid old URL pattern: %s"), oldurl);
 
 	for (i = 0; i < urlset_nr; i++)
 		if (!regexec(&old_regex, urlset[i], 0, NULL, 0))
 			matches++;
 		else
 			negative_matches++;
 	if (!delete_mode && !matches)
 		die(_("No such URL found: %s"), oldurl);
 	if (delete_mode && !negative_matches && !push_mode)
 		die(_("Will not delete all non-push URLs"));
 
 	regfree(&old_regex);
 
 	if (!delete_mode)
 		git_config_set_multivar(name_buf.buf, newurl, oldurl, 0);
 	else
 		git_config_set_multivar(name_buf.buf, NULL, oldurl, 1);
+out:
+	strbuf_release(&name_buf);
 	return 0;
 }
 
-- 
2.14.1

[PATCH 25/34] send-pack: release strbuf on error return in send_pack()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 send-pack.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/send-pack.c b/send-pack.c
index 11d6f3d983..b865f662e4 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -377,253 +377,256 @@ static void reject_invalid_nonce(const char *nonce, int len)
 int send_pack(struct send_pack_args *args,
 	      int fd[], struct child_process *conn,
 	      struct ref *remote_refs,
 	      struct oid_array *extra_have)
 {
 	int in = fd[0];
 	int out = fd[1];
 	struct strbuf req_buf = STRBUF_INIT;
 	struct strbuf cap_buf = STRBUF_INIT;
 	struct ref *ref;
 	int need_pack_data = 0;
 	int allow_deleting_refs = 0;
 	int status_report = 0;
 	int use_sideband = 0;
 	int quiet_supported = 0;
 	int agent_supported = 0;
 	int use_atomic = 0;
 	int atomic_supported = 0;
 	int use_push_options = 0;
 	int push_options_supported = 0;
 	unsigned cmds_sent = 0;
 	int ret;
 	struct async demux;
 	const char *push_cert_nonce = NULL;
 
 	/* Does the other end support the reporting? */
 	if (server_supports("report-status"))
 		status_report = 1;
 	if (server_supports("delete-refs"))
 		allow_deleting_refs = 1;
 	if (server_supports("ofs-delta"))
 		args->use_ofs_delta = 1;
 	if (server_supports("side-band-64k"))
 		use_sideband = 1;
 	if (server_supports("quiet"))
 		quiet_supported = 1;
 	if (server_supports("agent"))
 		agent_supported = 1;
 	if (server_supports("no-thin"))
 		args->use_thin_pack = 0;
 	if (server_supports("atomic"))
 		atomic_supported = 1;
 	if (server_supports("push-options"))
 		push_options_supported = 1;
 
 	if (args->push_cert != SEND_PACK_PUSH_CERT_NEVER) {
 		int len;
 		push_cert_nonce = server_feature_value("push-cert", &len);
 		if (push_cert_nonce) {
 			reject_invalid_nonce(push_cert_nonce, len);
 			push_cert_nonce = xmemdupz(push_cert_nonce, len);
 		} else if (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {
 			die(_("the receiving end does not support --signed push"));
 		} else if (args->push_cert == SEND_PACK_PUSH_CERT_IF_ASKED) {
 			warning(_("not sending a push certificate since the"
 				  " receiving end does not support --signed"
 				  " push"));
 		}
 	}
 
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
 			"Perhaps you should specify a branch such as 'master'.\n");
 		return 0;
 	}
 	if (args->atomic && !atomic_supported)
 		die(_("the receiving end does not support --atomic push"));
 
 	use_atomic = atomic_supported && args->atomic;
 
 	if (args->push_options && !push_options_supported)
 		die(_("the receiving end does not support push options"));
 
 	use_push_options = push_options_supported && args->push_options;
 
 	if (status_report)
 		strbuf_addstr(&cap_buf, " report-status");
 	if (use_sideband)
 		strbuf_addstr(&cap_buf, " side-band-64k");
 	if (quiet_supported && (args->quiet || !args->progress))
 		strbuf_addstr(&cap_buf, " quiet");
 	if (use_atomic)
 		strbuf_addstr(&cap_buf, " atomic");
 	if (use_push_options)
 		strbuf_addstr(&cap_buf, " push-options");
 	if (agent_supported)
 		strbuf_addf(&cap_buf, " agent=%s", git_user_agent_sanitized());
 
 	/*
 	 * NEEDSWORK: why does delete-refs have to be so specific to
 	 * send-pack machinery that set_ref_status_for_push() cannot
 	 * set this bit for us???
 	 */
 	for (ref = remote_refs; ref; ref = ref->next)
 		if (ref->deletion && !allow_deleting_refs)
 			ref->status = REF_STATUS_REJECT_NODELETE;
 
 	if (!args->dry_run)
 		advertise_shallow_grafts_buf(&req_buf);
 
 	if (!args->dry_run && push_cert_nonce)
 		cmds_sent = generate_push_cert(&req_buf, remote_refs, args,
 					       cap_buf.buf, push_cert_nonce);
 
 	/*
 	 * Clear the status for each ref and see if we need to send
 	 * the pack data.
 	 */
 	for (ref = remote_refs; ref; ref = ref->next) {
 		switch (check_to_send_update(ref, args)) {
 		case 0: /* no error */
 			break;
 		case CHECK_REF_STATUS_REJECTED:
 			/*
 			 * When we know the server would reject a ref update if
 			 * we were to send it and we're trying to send the refs
 			 * atomically, abort the whole operation.
 			 */
-			if (use_atomic)
+			if (use_atomic) {
+				strbuf_release(&req_buf);
+				strbuf_release(&cap_buf);
 				return atomic_push_failure(args, remote_refs, ref);
+			}
 			/* Fallthrough for non atomic case. */
 		default:
 			continue;
 		}
 		if (!ref->deletion)
 			need_pack_data = 1;
 
 		if (args->dry_run || !status_report)
 			ref->status = REF_STATUS_OK;
 		else
 			ref->status = REF_STATUS_EXPECTING_REPORT;
 	}
 
 	/*
 	 * Finally, tell the other end!
 	 */
 	for (ref = remote_refs; ref; ref = ref->next) {
 		char *old_hex, *new_hex;
 
 		if (args->dry_run || push_cert_nonce)
 			continue;
 
 		if (check_to_send_update(ref, args) < 0)
 			continue;
 
 		old_hex = oid_to_hex(&ref->old_oid);
 		new_hex = oid_to_hex(&ref->new_oid);
 		if (!cmds_sent) {
 			packet_buf_write(&req_buf,
 					 "%s %s %s%c%s",
 					 old_hex, new_hex, ref->name, 0,
 					 cap_buf.buf);
 			cmds_sent = 1;
 		} else {
 			packet_buf_write(&req_buf, "%s %s %s",
 					 old_hex, new_hex, ref->name);
 		}
 	}
 
 	if (use_push_options) {
 		struct string_list_item *item;
 
 		packet_buf_flush(&req_buf);
 		for_each_string_list_item(item, args->push_options)
 			packet_buf_write(&req_buf, "%s", item->string);
 	}
 
 	if (args->stateless_rpc) {
 		if (!args->dry_run && (cmds_sent || is_repository_shallow())) {
 			packet_buf_flush(&req_buf);
 			send_sideband(out, -1, req_buf.buf, req_buf.len, LARGE_PACKET_MAX);
 		}
 	} else {
 		write_or_die(out, req_buf.buf, req_buf.len);
 		packet_flush(out);
 	}
 	strbuf_release(&req_buf);
 	strbuf_release(&cap_buf);
 
 	if (use_sideband && cmds_sent) {
 		memset(&demux, 0, sizeof(demux));
 		demux.proc = sideband_demux;
 		demux.data = fd;
 		demux.out = -1;
 		demux.isolate_sigpipe = 1;
 		if (start_async(&demux))
 			die("send-pack: unable to fork off sideband demultiplexer");
 		in = demux.out;
 	}
 
 	if (need_pack_data && cmds_sent) {
 		if (pack_objects(out, remote_refs, extra_have, args) < 0) {
 			for (ref = remote_refs; ref; ref = ref->next)
 				ref->status = REF_STATUS_NONE;
 			if (args->stateless_rpc)
 				close(out);
 			if (git_connection_is_socket(conn))
 				shutdown(fd[0], SHUT_WR);
 
 			/*
 			 * Do not even bother with the return value; we know we
 			 * are failing, and just want the error() side effects.
 			 */
 			if (status_report)
 				receive_unpack_status(in);
 
 			if (use_sideband) {
 				close(demux.out);
 				finish_async(&demux);
 			}
 			fd[1] = -1;
 			return -1;
 		}
 		if (!args->stateless_rpc)
 			/* Closed by pack_objects() via start_command() */
 			fd[1] = -1;
 	}
 	if (args->stateless_rpc && cmds_sent)
 		packet_flush(out);
 
 	if (status_report && cmds_sent)
 		ret = receive_status(in, remote_refs);
 	else
 		ret = 0;
 	if (args->stateless_rpc)
 		packet_flush(out);
 
 	if (use_sideband && cmds_sent) {
 		close(demux.out);
 		if (finish_async(&demux)) {
 			error("error in sideband demultiplexer");
 			ret = -1;
 		}
 	}
 
 	if (ret < 0)
 		return ret;
 
 	if (args->porcelain)
 		return 0;
 
 	for (ref = remote_refs; ref; ref = ref->next) {
 		switch (ref->status) {
 		case REF_STATUS_NONE:
 		case REF_STATUS_UPTODATE:
 		case REF_STATUS_OK:
 			break;
 		default:
 			return -1;
 		}
 	}
 	return 0;
 }
-- 
2.14.1

[PATCH 26/34] sha1_file: release strbuf on error return in index_path()

[Rene Scharfe]

strbuf_readlink() already frees the buffer for us on error.  Clean up
if write_sha1_file() fails as well instead of returning early.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 sha1_file.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index f56bb5cae7..7d9c9aed2f 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1819,33 +1819,33 @@ int index_fd(struct object_id *oid, int fd, struct stat *st,
 int index_path(struct object_id *oid, const char *path, struct stat *st, unsigned flags)
 {
 	int fd;
 	struct strbuf sb = STRBUF_INIT;
+	int rc = 0;
 
 	switch (st->st_mode & S_IFMT) {
 	case S_IFREG:
 		fd = open(path, O_RDONLY);
 		if (fd < 0)
 			return error_errno("open(\"%s\")", path);
 		if (index_fd(oid, fd, st, OBJ_BLOB, path, flags) < 0)
 			return error("%s: failed to insert into database",
 				     path);
 		break;
 	case S_IFLNK:
 		if (strbuf_readlink(&sb, path, st->st_size))
 			return error_errno("readlink(\"%s\")", path);
 		if (!(flags & HASH_WRITE_OBJECT))
 			hash_sha1_file(sb.buf, sb.len, blob_type, oid->hash);
 		else if (write_sha1_file(sb.buf, sb.len, blob_type, oid->hash))
-			return error("%s: failed to insert into database",
-				     path);
+			rc = error("%s: failed to insert into database", path);
 		strbuf_release(&sb);
 		break;
 	case S_IFDIR:
 		return resolve_gitlink_ref(path, "HEAD", oid->hash);
 	default:
 		return error("%s: unsupported file type", path);
 	}
-	return 0;
+	return rc;
 }
 
 int read_pack_header(int fd, struct pack_header *header)
-- 
2.14.1

[PATCH 27/34] shortlog: release strbuf after use in insert_one_record()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/shortlog.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 43c4799ea9..48af16c681 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -50,66 +50,67 @@ static int compare_by_list(const void *a1, const void *a2)
 static void insert_one_record(struct shortlog *log,
 			      const char *author,
 			      const char *oneline)
 {
 	struct string_list_item *item;
 	const char *mailbuf, *namebuf;
 	size_t namelen, maillen;
 	struct strbuf namemailbuf = STRBUF_INIT;
 	struct ident_split ident;
 
 	if (split_ident_line(&ident, author, strlen(author)))
 		return;
 
 	namebuf = ident.name_begin;
 	mailbuf = ident.mail_begin;
 	namelen = ident.name_end - ident.name_begin;
 	maillen = ident.mail_end - ident.mail_begin;
 
 	map_user(&log->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
 	strbuf_add(&namemailbuf, namebuf, namelen);
 
 	if (log->email)
 		strbuf_addf(&namemailbuf, " <%.*s>", (int)maillen, mailbuf);
 
 	item = string_list_insert(&log->list, namemailbuf.buf);
+	strbuf_release(&namemailbuf);
 
 	if (log->summary)
 		item->util = (void *)(UTIL_TO_INT(item) + 1);
 	else {
 		const char *dot3 = log->common_repo_prefix;
 		char *buffer, *p;
 		struct strbuf subject = STRBUF_INIT;
 		const char *eol;
 
 		/* Skip any leading whitespace, including any blank lines. */
 		while (*oneline && isspace(*oneline))
 			oneline++;
 		eol = strchr(oneline, '\n');
 		if (!eol)
 			eol = oneline + strlen(oneline);
 		if (starts_with(oneline, "[PATCH")) {
 			char *eob = strchr(oneline, ']');
 			if (eob && (!eol || eob < eol))
 				oneline = eob + 1;
 		}
 		while (*oneline && isspace(*oneline) && *oneline != '\n')
 			oneline++;
 		format_subject(&subject, oneline, " ");
 		buffer = strbuf_detach(&subject, NULL);
 
 		if (dot3) {
 			int dot3len = strlen(dot3);
 			if (dot3len > 5) {
 				while ((p = strstr(buffer, dot3)) != NULL) {
 					int taillen = strlen(p) - dot3len;
 					memcpy(p, "/.../", 5);
 					memmove(p + 5, p + dot3len, taillen + 1);
 				}
 			}
 		}
 
 		if (item->util == NULL)
 			item->util = xcalloc(1, sizeof(struct string_list));
 		string_list_append(item->util, buffer);
 	}
 }
-- 
2.14.1

Re: [PATCH 27/34] shortlog: release strbuf after use in insert_one_record()

[Junio C Hamano]

Rene Scharfe <l.s.r@web.de> writes:

> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  builtin/shortlog.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index 43c4799ea9..48af16c681 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -50,66 +50,67 @@ static int compare_by_list(const void *a1, const void *a2)
>  static void insert_one_record(struct shortlog *log,
>  			      const char *author,
>  			      const char *oneline)
>  {
> ...
>  	item = string_list_insert(&log->list, namemailbuf.buf);
> +	strbuf_release(&namemailbuf);

As log->list.strdup_strings is set to true in shortlog_init(),
namemailbuf.buf will leak without this.

An alterative, as this is the only place we add to log->list, could
be to make log->list take ownership of the string by not adding a
_release() here but instead _detach(), I guess.

It is also curious that at the end of shortlog_output(), we set the
log->list.strdup_strings again to true immediately before calling
string_list_clear() on it.  I suspect that is unnecessary?

Re: [PATCH 27/34] shortlog: release strbuf after use in insert_one_record()

[Jeff King]

On Thu, Sep 07, 2017 at 04:51:12AM +0900, Junio C Hamano wrote:

> > diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> > index 43c4799ea9..48af16c681 100644
> > --- a/builtin/shortlog.c
> > +++ b/builtin/shortlog.c
> > @@ -50,66 +50,67 @@ static int compare_by_list(const void *a1, const void *a2)
> >  static void insert_one_record(struct shortlog *log,
> >  			      const char *author,
> >  			      const char *oneline)
> >  {
> > ...
> >  	item = string_list_insert(&log->list, namemailbuf.buf);
> > +	strbuf_release(&namemailbuf);
> 
> As log->list.strdup_strings is set to true in shortlog_init(),
> namemailbuf.buf will leak without this.
> 
> An alterative, as this is the only place we add to log->list, could
> be to make log->list take ownership of the string by not adding a
> _release() here but instead _detach(), I guess.

I agree that would be better, but I think it's a little tricky. The
string_list_insert() call may make a new entry, or it may return an
existing one. We'd still need to free in the latter case. I'm not sure
the string_list interface makes it easy to tell the difference.

> It is also curious that at the end of shortlog_output(), we set the
> log->list.strdup_strings again to true immediately before calling
> string_list_clear() on it.  I suspect that is unnecessary?

I think so. I was curious whether I might have introduced this weirdness
when I refactored this code a year or so ago. But no, it looks like it
dates all the way back to the very first version of shortlog.c.

-Peff

Re: [PATCH 27/34] shortlog: release strbuf after use in insert_one_record()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Sep 07, 2017 at 04:51:12AM +0900, Junio C Hamano wrote:
>
>> > diff --git a/builtin/shortlog.c b/builtin/shortlog.c
>> > index 43c4799ea9..48af16c681 100644
>> > --- a/builtin/shortlog.c
>> > +++ b/builtin/shortlog.c
>> > @@ -50,66 +50,67 @@ static int compare_by_list(const void *a1, const void *a2)
>> >  static void insert_one_record(struct shortlog *log,
>> >  			      const char *author,
>> >  			      const char *oneline)
>> >  {
>> > ...
>> >  	item = string_list_insert(&log->list, namemailbuf.buf);
>> > +	strbuf_release(&namemailbuf);
>> 
>> As log->list.strdup_strings is set to true in shortlog_init(),
>> namemailbuf.buf will leak without this.
>> 
>> An alterative, as this is the only place we add to log->list, could
>> be to make log->list take ownership of the string by not adding a
>> _release() here but instead _detach(), I guess.
>
> I agree that would be better, but I think it's a little tricky. The
> string_list_insert() call may make a new entry, or it may return an
> existing one. We'd still need to free in the latter case. I'm not sure
> the string_list interface makes it easy to tell the difference.

True; I do not think string_list API does.  But for this particular
application, I suspect that we can by looking at the util field of
the item returned.  A newly created one has NULL, but we always make
it non-NULL before leaving this function.

>> It is also curious that at the end of shortlog_output(), we set the
>> log->list.strdup_strings again to true immediately before calling
>> string_list_clear() on it.  I suspect that is unnecessary?
>
> I think so. I was curious whether I might have introduced this weirdness
> when I refactored this code a year or so ago. But no, it looks like it
> dates all the way back to the very first version of shortlog.c.

Hmph.

Re: [PATCH 27/34] shortlog: release strbuf after use in insert_one_record()

[Jeff King]

On Fri, Sep 08, 2017 at 09:33:38AM +0900, Junio C Hamano wrote:

> >> An alterative, as this is the only place we add to log->list, could
> >> be to make log->list take ownership of the string by not adding a
> >> _release() here but instead _detach(), I guess.
> >
> > I agree that would be better, but I think it's a little tricky. The
> > string_list_insert() call may make a new entry, or it may return an
> > existing one. We'd still need to free in the latter case. I'm not sure
> > the string_list interface makes it easy to tell the difference.
> 
> True; I do not think string_list API does.  But for this particular
> application, I suspect that we can by looking at the util field of
> the item returned.  A newly created one has NULL, but we always make
> it non-NULL before leaving this function.

Yeah, I agree that would work here.

I also wondered if we could get away with avoiding the malloc entirely
here. Especially in the "shortlog -n" case, it is identical to the name
field we already have in ident.name. So ideally we'd do a lookup to see
if we have the entry before allocating anything (since we do one lookup
per commit, but only insert once per unique author).

But that doesn't quite work, because ident.name doesn't put to a
NUL-terminated string, and string_list only handles strings.

We _can_ reuse the same buffer over and over:

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 43c4799ea9..7328abf4a1 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -54,7 +54,7 @@ static void insert_one_record(struct shortlog *log,
 	struct string_list_item *item;
 	const char *mailbuf, *namebuf;
 	size_t namelen, maillen;
-	struct strbuf namemailbuf = STRBUF_INIT;
+	static struct strbuf namemailbuf = STRBUF_INIT;
 	struct ident_split ident;
 
 	if (split_ident_line(&ident, author, strlen(author)))
@@ -66,6 +66,7 @@ static void insert_one_record(struct shortlog *log,
 	maillen = ident.mail_end - ident.mail_begin;
 
 	map_user(&log->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
+	strbuf_reset(&namemailbuf);
 	strbuf_add(&namemailbuf, namebuf, namelen);
 
 	if (log->email)

That saves the malloc, if not the extra copying. It shows about a 1%
speed up on "git shortlog -ns" on linux.git. Probably that's not worth
caring too much about, but it also "solves" the leaking problem (I'm not
sure if the speedup is from calling malloc less frequently, or from
lowering our peak heap usage due to fixing the leak).

-Peff

Re: [PATCH 27/34] shortlog: release strbuf after use in insert_one_record()

[Jeff King]

On Thu, Sep 07, 2017 at 11:56:48PM -0400, Jeff King wrote:

> > True; I do not think string_list API does.  But for this particular
> > application, I suspect that we can by looking at the util field of
> > the item returned.  A newly created one has NULL, but we always make
> > it non-NULL before leaving this function.
> 
> Yeah, I agree that would work here.
> 
> I also wondered if we could get away with avoiding the malloc entirely
> here. Especially in the "shortlog -n" case, it is identical to the name
> field we already have in ident.name. So ideally we'd do a lookup to see
> if we have the entry before allocating anything (since we do one lookup
> per commit, but only insert once per unique author).
> 
> But that doesn't quite work, because ident.name doesn't put to a
> NUL-terminated string, and string_list only handles strings.

I happened to look at this more while digging on an unrelated shortlog
bug. I think the whole thing could actually be reorganized a bit.

We call insert_one_record() from shortlog_add_commit(). The latter
formats "%an <%ae>", only to have the former parse it back to its
constituent parts. That seems rather silly.

This is an artifact of shortlog's original mode, which was to parse "git
log" output. But for an internal traversal, we can just format the
correct item right off the bat. That part of insert_one_record() is also
where we handle the mailmap mapping. But again, the internal traversal
can just "%aE" to format that correctly in the first place.

IOW, something like the patch below, which pushes the re-parsing out to
the stdin code-path, and lets the internal traversal format directly
into the final buffer. It seems to be about 3% faster than the existing
code, and fixes the leak (by dropping that variable entirely).

-Peff

---
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 43c4799ea9..e29875b843 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -52,26 +52,8 @@ static void insert_one_record(struct shortlog *log,
 			      const char *oneline)
 {
 	struct string_list_item *item;
-	const char *mailbuf, *namebuf;
-	size_t namelen, maillen;
-	struct strbuf namemailbuf = STRBUF_INIT;
-	struct ident_split ident;
 
-	if (split_ident_line(&ident, author, strlen(author)))
-		return;
-
-	namebuf = ident.name_begin;
-	mailbuf = ident.mail_begin;
-	namelen = ident.name_end - ident.name_begin;
-	maillen = ident.mail_end - ident.mail_begin;
-
-	map_user(&log->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
-	strbuf_add(&namemailbuf, namebuf, namelen);
-
-	if (log->email)
-		strbuf_addf(&namemailbuf, " <%.*s>", (int)maillen, mailbuf);
-
-	item = string_list_insert(&log->list, namemailbuf.buf);
+	item = string_list_insert(&log->list, author);
 
 	if (log->summary)
 		item->util = (void *)(UTIL_TO_INT(item) + 1);
@@ -114,9 +96,33 @@ static void insert_one_record(struct shortlog *log,
 	}
 }
 
+static int parse_stdin_author(struct shortlog *log,
+			       struct strbuf *out, const char *in)
+{
+	const char *mailbuf, *namebuf;
+	size_t namelen, maillen;
+	struct ident_split ident;
+
+	if (split_ident_line(&ident, in, strlen(in)))
+		return -1;
+
+	namebuf = ident.name_begin;
+	mailbuf = ident.mail_begin;
+	namelen = ident.name_end - ident.name_begin;
+	maillen = ident.mail_end - ident.mail_begin;
+
+	map_user(&log->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
+	strbuf_add(out, namebuf, namelen);
+	if (log->email)
+		strbuf_addf(out, " <%.*s>", (int)maillen, mailbuf);
+
+	return 0;
+}
+
 static void read_from_stdin(struct shortlog *log)
 {
 	struct strbuf author = STRBUF_INIT;
+	struct strbuf mapped_author = STRBUF_INIT;
 	struct strbuf oneline = STRBUF_INIT;
 	static const char *author_match[2] = { "Author: ", "author " };
 	static const char *committer_match[2] = { "Commit: ", "committer " };
@@ -134,9 +140,15 @@ static void read_from_stdin(struct shortlog *log)
 		while (strbuf_getline_lf(&oneline, stdin) != EOF &&
 		       !oneline.len)
 			; /* discard blanks */
-		insert_one_record(log, v, oneline.buf);
+
+		strbuf_reset(&mapped_author);
+		if (parse_stdin_author(log, &mapped_author, v) < 0)
+			continue;
+
+		insert_one_record(log, mapped_author.buf, oneline.buf);
 	}
 	strbuf_release(&author);
+	strbuf_release(&mapped_author);
 	strbuf_release(&oneline);
 }
 
@@ -153,7 +165,9 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
 	ctx.date_mode.type = DATE_NORMAL;
 	ctx.output_encoding = get_log_output_encoding();
 
-	fmt = log->committer ? "%cn <%ce>" : "%an <%ae>";
+	fmt = log->committer ?
+		(log->email ? "%cN <%cE>" : "%cN") :
+		(log->email ? "%aN <%aE>" : "%aN");
 
 	format_commit_message(commit, fmt, &author, &ctx);
 	if (!log->summary) {

Re: [PATCH 27/34] shortlog: release strbuf after use in insert_one_record()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> IOW, something like the patch below, which pushes the re-parsing out to
> the stdin code-path, and lets the internal traversal format directly
> into the final buffer. It seems to be about 3% faster than the existing
> code, and fixes the leak (by dropping that variable entirely).

Wow, that is soooo logical a conclusion that I somewhat feel ashamed
that I didn't think of it myself.  

Nicely done.

>
> -Peff
>
> ---
> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index 43c4799ea9..e29875b843 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -52,26 +52,8 @@ static void insert_one_record(struct shortlog *log,
>  			      const char *oneline)
>  {
>  	struct string_list_item *item;
> -	const char *mailbuf, *namebuf;
> -	size_t namelen, maillen;
> -	struct strbuf namemailbuf = STRBUF_INIT;
> -	struct ident_split ident;
>  
> -	if (split_ident_line(&ident, author, strlen(author)))
> -		return;
> -
> -	namebuf = ident.name_begin;
> -	mailbuf = ident.mail_begin;
> -	namelen = ident.name_end - ident.name_begin;
> -	maillen = ident.mail_end - ident.mail_begin;
> -
> -	map_user(&log->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
> -	strbuf_add(&namemailbuf, namebuf, namelen);
> -
> -	if (log->email)
> -		strbuf_addf(&namemailbuf, " <%.*s>", (int)maillen, mailbuf);
> -
> -	item = string_list_insert(&log->list, namemailbuf.buf);
> +	item = string_list_insert(&log->list, author);
>  
>  	if (log->summary)
>  		item->util = (void *)(UTIL_TO_INT(item) + 1);
> @@ -114,9 +96,33 @@ static void insert_one_record(struct shortlog *log,
>  	}
>  }
>  
> +static int parse_stdin_author(struct shortlog *log,
> +			       struct strbuf *out, const char *in)
> +{
> +	const char *mailbuf, *namebuf;
> +	size_t namelen, maillen;
> +	struct ident_split ident;
> +
> +	if (split_ident_line(&ident, in, strlen(in)))
> +		return -1;
> +
> +	namebuf = ident.name_begin;
> +	mailbuf = ident.mail_begin;
> +	namelen = ident.name_end - ident.name_begin;
> +	maillen = ident.mail_end - ident.mail_begin;
> +
> +	map_user(&log->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
> +	strbuf_add(out, namebuf, namelen);
> +	if (log->email)
> +		strbuf_addf(out, " <%.*s>", (int)maillen, mailbuf);
> +
> +	return 0;
> +}
> +
>  static void read_from_stdin(struct shortlog *log)
>  {
>  	struct strbuf author = STRBUF_INIT;
> +	struct strbuf mapped_author = STRBUF_INIT;
>  	struct strbuf oneline = STRBUF_INIT;
>  	static const char *author_match[2] = { "Author: ", "author " };
>  	static const char *committer_match[2] = { "Commit: ", "committer " };
> @@ -134,9 +140,15 @@ static void read_from_stdin(struct shortlog *log)
>  		while (strbuf_getline_lf(&oneline, stdin) != EOF &&
>  		       !oneline.len)
>  			; /* discard blanks */
> -		insert_one_record(log, v, oneline.buf);
> +
> +		strbuf_reset(&mapped_author);
> +		if (parse_stdin_author(log, &mapped_author, v) < 0)
> +			continue;
> +
> +		insert_one_record(log, mapped_author.buf, oneline.buf);
>  	}
>  	strbuf_release(&author);
> +	strbuf_release(&mapped_author);
>  	strbuf_release(&oneline);
>  }
>  
> @@ -153,7 +165,9 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
>  	ctx.date_mode.type = DATE_NORMAL;
>  	ctx.output_encoding = get_log_output_encoding();
>  
> -	fmt = log->committer ? "%cn <%ce>" : "%an <%ae>";
> +	fmt = log->committer ?
> +		(log->email ? "%cN <%cE>" : "%cN") :
> +		(log->email ? "%aN <%aE>" : "%aN");
>  
>  	format_commit_message(commit, fmt, &author, &ctx);
>  	if (!log->summary) {

[PATCH] shortlog: skip format/parse roundtrip for internal traversal

[Jeff King]

On Fri, Sep 08, 2017 at 03:39:36PM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > IOW, something like the patch below, which pushes the re-parsing out to
> > the stdin code-path, and lets the internal traversal format directly
> > into the final buffer. It seems to be about 3% faster than the existing
> > code, and fixes the leak (by dropping that variable entirely).
> 
> Wow, that is soooo logical a conclusion that I somewhat feel ashamed
> that I didn't think of it myself.
> 
> Nicely done.

Thanks. Here it is with a commit message.

Note that the non-stdin path no longer looks at the "mailmap" entry of
"struct shortlog" (instead we use the one cached inside pretty.c). But
we still waste time loading it. I'm not sure if it's worth addressing
that. It's only once per program invocation, and it's a little tricky to
fix (we do shortlog_init() before we know whether or not we're using
stdin). We could just load it lazily, though, which would cover the
stdin case.

-- >8 --
Subject: shortlog: skip format/parse roundtrip for internal traversal

The original git-shortlog command parsed the output of
git-log, and the logic went something like this:

  1. Read stdin looking for "author" lines.

  2. Parse the identity into its name/email bits.

  3. Apply mailmap to the name/email.

  4. Reformat the identity into a single buffer that is our
     "key" for grouping entries (either a name by default,
     or "name <email>" if --email was given).

The first part happens in read_from_stdin(), and the other
three steps are part of insert_one_record().

When we do an internal traversal, we just swap out the stdin
read in step 1 for reading the commit objects ourselves.
Prior to 2db6b83d18 (shortlog: replace hand-parsing of
author with pretty-printer, 2016-01-18), that made sense; we
still had to parse the ident in the commit message.

But after that commit, we use pretty.c's "%an <%ae>" to get
the author ident (for simplicity). Which means that the
pretty printer is doing a parse/format under the hood, and
then we parse the result, apply the mailmap, and format the
result again.

Instead, we can just ask pretty.c to do all of those steps
for us (including the mailmap via "%aN <%aE>", and not
formatting the address when --email is missing).

And then we can push steps 2-4 into read_from_stdin(). This
speeds up "git shortlog -ns" on linux.git by about 3%, and
eliminates a leak in insert_one_record() of the namemailbuf
strbuf.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/shortlog.c | 56 ++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 43c4799ea9..e29875b843 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -52,26 +52,8 @@ static void insert_one_record(struct shortlog *log,
 			      const char *oneline)
 {
 	struct string_list_item *item;
-	const char *mailbuf, *namebuf;
-	size_t namelen, maillen;
-	struct strbuf namemailbuf = STRBUF_INIT;
-	struct ident_split ident;
 
-	if (split_ident_line(&ident, author, strlen(author)))
-		return;
-
-	namebuf = ident.name_begin;
-	mailbuf = ident.mail_begin;
-	namelen = ident.name_end - ident.name_begin;
-	maillen = ident.mail_end - ident.mail_begin;
-
-	map_user(&log->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
-	strbuf_add(&namemailbuf, namebuf, namelen);
-
-	if (log->email)
-		strbuf_addf(&namemailbuf, " <%.*s>", (int)maillen, mailbuf);
-
-	item = string_list_insert(&log->list, namemailbuf.buf);
+	item = string_list_insert(&log->list, author);
 
 	if (log->summary)
 		item->util = (void *)(UTIL_TO_INT(item) + 1);
@@ -114,9 +96,33 @@ static void insert_one_record(struct shortlog *log,
 	}
 }
 
+static int parse_stdin_author(struct shortlog *log,
+			       struct strbuf *out, const char *in)
+{
+	const char *mailbuf, *namebuf;
+	size_t namelen, maillen;
+	struct ident_split ident;
+
+	if (split_ident_line(&ident, in, strlen(in)))
+		return -1;
+
+	namebuf = ident.name_begin;
+	mailbuf = ident.mail_begin;
+	namelen = ident.name_end - ident.name_begin;
+	maillen = ident.mail_end - ident.mail_begin;
+
+	map_user(&log->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
+	strbuf_add(out, namebuf, namelen);
+	if (log->email)
+		strbuf_addf(out, " <%.*s>", (int)maillen, mailbuf);
+
+	return 0;
+}
+
 static void read_from_stdin(struct shortlog *log)
 {
 	struct strbuf author = STRBUF_INIT;
+	struct strbuf mapped_author = STRBUF_INIT;
 	struct strbuf oneline = STRBUF_INIT;
 	static const char *author_match[2] = { "Author: ", "author " };
 	static const char *committer_match[2] = { "Commit: ", "committer " };
@@ -134,9 +140,15 @@ static void read_from_stdin(struct shortlog *log)
 		while (strbuf_getline_lf(&oneline, stdin) != EOF &&
 		       !oneline.len)
 			; /* discard blanks */
-		insert_one_record(log, v, oneline.buf);
+
+		strbuf_reset(&mapped_author);
+		if (parse_stdin_author(log, &mapped_author, v) < 0)
+			continue;
+
+		insert_one_record(log, mapped_author.buf, oneline.buf);
 	}
 	strbuf_release(&author);
+	strbuf_release(&mapped_author);
 	strbuf_release(&oneline);
 }
 
@@ -153,7 +165,9 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
 	ctx.date_mode.type = DATE_NORMAL;
 	ctx.output_encoding = get_log_output_encoding();
 
-	fmt = log->committer ? "%cn <%ce>" : "%an <%ae>";
+	fmt = log->committer ?
+		(log->email ? "%cN <%cE>" : "%cN") :
+		(log->email ? "%aN <%aE>" : "%aN");
 
 	format_commit_message(commit, fmt, &author, &ctx);
 	if (!log->summary) {
-- 
2.14.1.769.g4f4ea7dfd3

Re: [PATCH] shortlog: skip format/parse roundtrip for internal traversal

[René Scharfe]

Am 08.09.2017 um 11:21 schrieb Jeff King:
> Note that the non-stdin path no longer looks at the "mailmap" entry of
> "struct shortlog" (instead we use the one cached inside pretty.c). But
> we still waste time loading it. I'm not sure if it's worth addressing
> that. It's only once per program invocation, and it's a little tricky to
> fix (we do shortlog_init() before we know whether or not we're using
> stdin). We could just load it lazily, though, which would cover the
> stdin case.

The difference in performance and memory usage will only be measurable
with really big mailmap files.  However, it may be an opportunity for
simplifying the mailmap API in general.  Conceptually the map data
should fit into struct repository instead of being read and stored by
each user, right?

> -- >8 --
> Subject: shortlog: skip format/parse roundtrip for internal traversal
> 
> The original git-shortlog command parsed the output of
> git-log, and the logic went something like this:
> 
>    1. Read stdin looking for "author" lines.
> 
>    2. Parse the identity into its name/email bits.
> 
>    3. Apply mailmap to the name/email.
> 
>    4. Reformat the identity into a single buffer that is our
>       "key" for grouping entries (either a name by default,
>       or "name <email>" if --email was given).
> 
> The first part happens in read_from_stdin(), and the other
> three steps are part of insert_one_record().
> 
> When we do an internal traversal, we just swap out the stdin
> read in step 1 for reading the commit objects ourselves.
> Prior to 2db6b83d18 (shortlog: replace hand-parsing of
> author with pretty-printer, 2016-01-18), that made sense; we
> still had to parse the ident in the commit message.
> 
> But after that commit, we use pretty.c's "%an <%ae>" to get
> the author ident (for simplicity). Which means that the
> pretty printer is doing a parse/format under the hood, and
> then we parse the result, apply the mailmap, and format the
> result again.
> 
> Instead, we can just ask pretty.c to do all of those steps
> for us (including the mailmap via "%aN <%aE>", and not
> formatting the address when --email is missing).
> 
> And then we can push steps 2-4 into read_from_stdin(). This
> speeds up "git shortlog -ns" on linux.git by about 3%, and
> eliminates a leak in insert_one_record() of the namemailbuf
> strbuf.

Great!  Thanks for stepping back, looking at the bigger
picture and making it prettier.

> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   builtin/shortlog.c | 56 ++++++++++++++++++++++++++++++++++--------------------
>   1 file changed, 35 insertions(+), 21 deletions(-)

While longer, the resulting code is split up into more
digestible chunks.

René

Re: [PATCH] shortlog: skip format/parse roundtrip for internal traversal

[Jeff King]

On Sun, Sep 10, 2017 at 10:44:46AM +0200, René Scharfe wrote:

> Am 08.09.2017 um 11:21 schrieb Jeff King:
> > Note that the non-stdin path no longer looks at the "mailmap" entry of
> > "struct shortlog" (instead we use the one cached inside pretty.c). But
> > we still waste time loading it. I'm not sure if it's worth addressing
> > that. It's only once per program invocation, and it's a little tricky to
> > fix (we do shortlog_init() before we know whether or not we're using
> > stdin). We could just load it lazily, though, which would cover the
> > stdin case.
> 
> The difference in performance and memory usage will only be measurable
> with really big mailmap files.  However, it may be an opportunity for
> simplifying the mailmap API in general.  Conceptually the map data
> should fit into struct repository instead of being read and stored by
> each user, right?

Yes, I think it would be fine to have a single "the_mailmap", and in
post-struct-repository world, that's where it should go.

-Peff

[PATCH 08/34] connect: release strbuf on error return in git_connect()

[Rene Scharfe]

Reduce the scope of the variable cmd and release it before returning
early.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 connect.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index 49b28b83be..df56c0cbff 100644
--- a/connect.c
+++ b/connect.c
@@ -775,146 +775,148 @@ static void handle_ssh_variant(const char *ssh_command, int is_cmdline,
 struct child_process *git_connect(int fd[2], const char *url,
 				  const char *prog, int flags)
 {
 	char *hostandport, *path;
 	struct child_process *conn = &no_fork;
 	enum protocol protocol;
-	struct strbuf cmd = STRBUF_INIT;
 
 	/* Without this we cannot rely on waitpid() to tell
 	 * what happened to our children.
 	 */
 	signal(SIGCHLD, SIG_DFL);
 
 	protocol = parse_connect_url(url, &hostandport, &path);
 	if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
 		printf("Diag: url=%s\n", url ? url : "NULL");
 		printf("Diag: protocol=%s\n", prot_name(protocol));
 		printf("Diag: hostandport=%s\n", hostandport ? hostandport : "NULL");
 		printf("Diag: path=%s\n", path ? path : "NULL");
 		conn = NULL;
 	} else if (protocol == PROTO_GIT) {
 		/*
 		 * Set up virtual host information based on where we will
 		 * connect, unless the user has overridden us in
 		 * the environment.
 		 */
 		char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
 		if (target_host)
 			target_host = xstrdup(target_host);
 		else
 			target_host = xstrdup(hostandport);
 
 		transport_check_allowed("git");
 
 		/* These underlying connection commands die() if they
 		 * cannot connect.
 		 */
 		if (git_use_proxy(hostandport))
 			conn = git_proxy_connect(fd, hostandport);
 		else
 			git_tcp_connect(fd, hostandport, flags);
 		/*
 		 * Separate original protocol components prog and path
 		 * from extended host header with a NUL byte.
 		 *
 		 * Note: Do not add any other headers here!  Doing so
 		 * will cause older git-daemon servers to crash.
 		 */
 		packet_write_fmt(fd[1],
 			     "%s %s%chost=%s%c",
 			     prog, path, 0,
 			     target_host, 0);
 		free(target_host);
 	} else {
+		struct strbuf cmd = STRBUF_INIT;
+
 		conn = xmalloc(sizeof(*conn));
 		child_process_init(conn);
 
 		if (looks_like_command_line_option(path))
 			die("strange pathname '%s' blocked", path);
 
 		strbuf_addstr(&cmd, prog);
 		strbuf_addch(&cmd, ' ');
 		sq_quote_buf(&cmd, path);
 
 		/* remove repo-local variables from the environment */
 		conn->env = local_repo_env;
 		conn->use_shell = 1;
 		conn->in = conn->out = -1;
 		if (protocol == PROTO_SSH) {
 			const char *ssh;
 			int needs_batch = 0;
 			int port_option = 'p';
 			char *ssh_host = hostandport;
 			const char *port = NULL;
 			transport_check_allowed("ssh");
 			get_host_and_port(&ssh_host, &port);
 
 			if (!port)
 				port = get_port(ssh_host);
 
 			if (flags & CONNECT_DIAG_URL) {
 				printf("Diag: url=%s\n", url ? url : "NULL");
 				printf("Diag: protocol=%s\n", prot_name(protocol));
 				printf("Diag: userandhost=%s\n", ssh_host ? ssh_host : "NULL");
 				printf("Diag: port=%s\n", port ? port : "NONE");
 				printf("Diag: path=%s\n", path ? path : "NULL");
 
 				free(hostandport);
 				free(path);
 				free(conn);
+				strbuf_release(&cmd);
 				return NULL;
 			}
 
 			if (looks_like_command_line_option(ssh_host))
 				die("strange hostname '%s' blocked", ssh_host);
 
 			ssh = get_ssh_command();
 			if (ssh)
 				handle_ssh_variant(ssh, 1, &port_option,
 						   &needs_batch);
 			else {
 				/*
 				 * GIT_SSH is the no-shell version of
 				 * GIT_SSH_COMMAND (and must remain so for
 				 * historical compatibility).
 				 */
 				conn->use_shell = 0;
 
 				ssh = getenv("GIT_SSH");
 				if (!ssh)
 					ssh = "ssh";
 				else
 					handle_ssh_variant(ssh, 0,
 							   &port_option,
 							   &needs_batch);
 			}
 
 			argv_array_push(&conn->args, ssh);
 			if (flags & CONNECT_IPV4)
 				argv_array_push(&conn->args, "-4");
 			else if (flags & CONNECT_IPV6)
 				argv_array_push(&conn->args, "-6");
 			if (needs_batch)
 				argv_array_push(&conn->args, "-batch");
 			if (port) {
 				argv_array_pushf(&conn->args,
 						 "-%c", port_option);
 				argv_array_push(&conn->args, port);
 			}
 			argv_array_push(&conn->args, ssh_host);
 		} else {
 			transport_check_allowed("file");
 		}
 		argv_array_push(&conn->args, cmd.buf);
 
 		if (start_command(conn))
 			die("unable to fork");
 
 		fd[0] = conn->out; /* read from child's stdout */
 		fd[1] = conn->in;  /* write to child's stdin */
 		strbuf_release(&cmd);
 	}
 	free(hostandport);
 	free(path);
 	return conn;
 }
-- 
2.14.1

[PATCH 21/34] refs: release strbuf on error return in write_pseudoref()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index b0106b8162..d8dc86b1f5 100644
--- a/refs.c
+++ b/refs.c
@@ -597,45 +597,45 @@ long get_files_ref_lock_timeout_ms(void)
 static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
 			   const unsigned char *old_sha1, struct strbuf *err)
 {
 	const char *filename;
 	int fd;
 	static struct lock_file lock;
 	struct strbuf buf = STRBUF_INIT;
 	int ret = -1;
 
 	strbuf_addf(&buf, "%s\n", sha1_to_hex(sha1));
 
 	filename = git_path("%s", pseudoref);
 	fd = hold_lock_file_for_update_timeout(&lock, filename,
 					       LOCK_DIE_ON_ERROR,
 					       get_files_ref_lock_timeout_ms());
 	if (fd < 0) {
 		strbuf_addf(err, "could not open '%s' for writing: %s",
 			    filename, strerror(errno));
-		return -1;
+		goto done;
 	}
 
 	if (old_sha1) {
 		unsigned char actual_old_sha1[20];
 
 		if (read_ref(pseudoref, actual_old_sha1))
 			die("could not read ref '%s'", pseudoref);
 		if (hashcmp(actual_old_sha1, old_sha1)) {
 			strbuf_addf(err, "unexpected sha1 when writing '%s'", pseudoref);
 			rollback_lock_file(&lock);
 			goto done;
 		}
 	}
 
 	if (write_in_full(fd, buf.buf, buf.len) != buf.len) {
 		strbuf_addf(err, "could not write to '%s'", filename);
 		rollback_lock_file(&lock);
 		goto done;
 	}
 
 	commit_lock_file(&lock);
 	ret = 0;
 done:
 	strbuf_release(&buf);
 	return ret;
 }
-- 
2.14.1

[PATCH 25/34] send-pack: release strbuf on error return in send_pack()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 send-pack.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/send-pack.c b/send-pack.c
index 11d6f3d983..b865f662e4 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -377,253 +377,256 @@ static void reject_invalid_nonce(const char *nonce, int len)
 int send_pack(struct send_pack_args *args,
 	      int fd[], struct child_process *conn,
 	      struct ref *remote_refs,
 	      struct oid_array *extra_have)
 {
 	int in = fd[0];
 	int out = fd[1];
 	struct strbuf req_buf = STRBUF_INIT;
 	struct strbuf cap_buf = STRBUF_INIT;
 	struct ref *ref;
 	int need_pack_data = 0;
 	int allow_deleting_refs = 0;
 	int status_report = 0;
 	int use_sideband = 0;
 	int quiet_supported = 0;
 	int agent_supported = 0;
 	int use_atomic = 0;
 	int atomic_supported = 0;
 	int use_push_options = 0;
 	int push_options_supported = 0;
 	unsigned cmds_sent = 0;
 	int ret;
 	struct async demux;
 	const char *push_cert_nonce = NULL;
 
 	/* Does the other end support the reporting? */
 	if (server_supports("report-status"))
 		status_report = 1;
 	if (server_supports("delete-refs"))
 		allow_deleting_refs = 1;
 	if (server_supports("ofs-delta"))
 		args->use_ofs_delta = 1;
 	if (server_supports("side-band-64k"))
 		use_sideband = 1;
 	if (server_supports("quiet"))
 		quiet_supported = 1;
 	if (server_supports("agent"))
 		agent_supported = 1;
 	if (server_supports("no-thin"))
 		args->use_thin_pack = 0;
 	if (server_supports("atomic"))
 		atomic_supported = 1;
 	if (server_supports("push-options"))
 		push_options_supported = 1;
 
 	if (args->push_cert != SEND_PACK_PUSH_CERT_NEVER) {
 		int len;
 		push_cert_nonce = server_feature_value("push-cert", &len);
 		if (push_cert_nonce) {
 			reject_invalid_nonce(push_cert_nonce, len);
 			push_cert_nonce = xmemdupz(push_cert_nonce, len);
 		} else if (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {
 			die(_("the receiving end does not support --signed push"));
 		} else if (args->push_cert == SEND_PACK_PUSH_CERT_IF_ASKED) {
 			warning(_("not sending a push certificate since the"
 				  " receiving end does not support --signed"
 				  " push"));
 		}
 	}
 
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
 			"Perhaps you should specify a branch such as 'master'.\n");
 		return 0;
 	}
 	if (args->atomic && !atomic_supported)
 		die(_("the receiving end does not support --atomic push"));
 
 	use_atomic = atomic_supported && args->atomic;
 
 	if (args->push_options && !push_options_supported)
 		die(_("the receiving end does not support push options"));
 
 	use_push_options = push_options_supported && args->push_options;
 
 	if (status_report)
 		strbuf_addstr(&cap_buf, " report-status");
 	if (use_sideband)
 		strbuf_addstr(&cap_buf, " side-band-64k");
 	if (quiet_supported && (args->quiet || !args->progress))
 		strbuf_addstr(&cap_buf, " quiet");
 	if (use_atomic)
 		strbuf_addstr(&cap_buf, " atomic");
 	if (use_push_options)
 		strbuf_addstr(&cap_buf, " push-options");
 	if (agent_supported)
 		strbuf_addf(&cap_buf, " agent=%s", git_user_agent_sanitized());
 
 	/*
 	 * NEEDSWORK: why does delete-refs have to be so specific to
 	 * send-pack machinery that set_ref_status_for_push() cannot
 	 * set this bit for us???
 	 */
 	for (ref = remote_refs; ref; ref = ref->next)
 		if (ref->deletion && !allow_deleting_refs)
 			ref->status = REF_STATUS_REJECT_NODELETE;
 
 	if (!args->dry_run)
 		advertise_shallow_grafts_buf(&req_buf);
 
 	if (!args->dry_run && push_cert_nonce)
 		cmds_sent = generate_push_cert(&req_buf, remote_refs, args,
 					       cap_buf.buf, push_cert_nonce);
 
 	/*
 	 * Clear the status for each ref and see if we need to send
 	 * the pack data.
 	 */
 	for (ref = remote_refs; ref; ref = ref->next) {
 		switch (check_to_send_update(ref, args)) {
 		case 0: /* no error */
 			break;
 		case CHECK_REF_STATUS_REJECTED:
 			/*
 			 * When we know the server would reject a ref update if
 			 * we were to send it and we're trying to send the refs
 			 * atomically, abort the whole operation.
 			 */
-			if (use_atomic)
+			if (use_atomic) {
+				strbuf_release(&req_buf);
+				strbuf_release(&cap_buf);
 				return atomic_push_failure(args, remote_refs, ref);
+			}
 			/* Fallthrough for non atomic case. */
 		default:
 			continue;
 		}
 		if (!ref->deletion)
 			need_pack_data = 1;
 
 		if (args->dry_run || !status_report)
 			ref->status = REF_STATUS_OK;
 		else
 			ref->status = REF_STATUS_EXPECTING_REPORT;
 	}
 
 	/*
 	 * Finally, tell the other end!
 	 */
 	for (ref = remote_refs; ref; ref = ref->next) {
 		char *old_hex, *new_hex;
 
 		if (args->dry_run || push_cert_nonce)
 			continue;
 
 		if (check_to_send_update(ref, args) < 0)
 			continue;
 
 		old_hex = oid_to_hex(&ref->old_oid);
 		new_hex = oid_to_hex(&ref->new_oid);
 		if (!cmds_sent) {
 			packet_buf_write(&req_buf,
 					 "%s %s %s%c%s",
 					 old_hex, new_hex, ref->name, 0,
 					 cap_buf.buf);
 			cmds_sent = 1;
 		} else {
 			packet_buf_write(&req_buf, "%s %s %s",
 					 old_hex, new_hex, ref->name);
 		}
 	}
 
 	if (use_push_options) {
 		struct string_list_item *item;
 
 		packet_buf_flush(&req_buf);
 		for_each_string_list_item(item, args->push_options)
 			packet_buf_write(&req_buf, "%s", item->string);
 	}
 
 	if (args->stateless_rpc) {
 		if (!args->dry_run && (cmds_sent || is_repository_shallow())) {
 			packet_buf_flush(&req_buf);
 			send_sideband(out, -1, req_buf.buf, req_buf.len, LARGE_PACKET_MAX);
 		}
 	} else {
 		write_or_die(out, req_buf.buf, req_buf.len);
 		packet_flush(out);
 	}
 	strbuf_release(&req_buf);
 	strbuf_release(&cap_buf);
 
 	if (use_sideband && cmds_sent) {
 		memset(&demux, 0, sizeof(demux));
 		demux.proc = sideband_demux;
 		demux.data = fd;
 		demux.out = -1;
 		demux.isolate_sigpipe = 1;
 		if (start_async(&demux))
 			die("send-pack: unable to fork off sideband demultiplexer");
 		in = demux.out;
 	}
 
 	if (need_pack_data && cmds_sent) {
 		if (pack_objects(out, remote_refs, extra_have, args) < 0) {
 			for (ref = remote_refs; ref; ref = ref->next)
 				ref->status = REF_STATUS_NONE;
 			if (args->stateless_rpc)
 				close(out);
 			if (git_connection_is_socket(conn))
 				shutdown(fd[0], SHUT_WR);
 
 			/*
 			 * Do not even bother with the return value; we know we
 			 * are failing, and just want the error() side effects.
 			 */
 			if (status_report)
 				receive_unpack_status(in);
 
 			if (use_sideband) {
 				close(demux.out);
 				finish_async(&demux);
 			}
 			fd[1] = -1;
 			return -1;
 		}
 		if (!args->stateless_rpc)
 			/* Closed by pack_objects() via start_command() */
 			fd[1] = -1;
 	}
 	if (args->stateless_rpc && cmds_sent)
 		packet_flush(out);
 
 	if (status_report && cmds_sent)
 		ret = receive_status(in, remote_refs);
 	else
 		ret = 0;
 	if (args->stateless_rpc)
 		packet_flush(out);
 
 	if (use_sideband && cmds_sent) {
 		close(demux.out);
 		if (finish_async(&demux)) {
 			error("error in sideband demultiplexer");
 			ret = -1;
 		}
 	}
 
 	if (ret < 0)
 		return ret;
 
 	if (args->porcelain)
 		return 0;
 
 	for (ref = remote_refs; ref; ref = ref->next) {
 		switch (ref->status) {
 		case REF_STATUS_NONE:
 		case REF_STATUS_UPTODATE:
 		case REF_STATUS_OK:
 			break;
 		default:
 			return -1;
 		}
 	}
 	return 0;
 }
-- 
2.14.1

[PATCH 28/34] sequencer: release strbuf after use in save_head()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 sequencer.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index fcceabb80f..60636ce54b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1563,23 +1563,26 @@ static int create_seq_dir(void)
 static int save_head(const char *head)
 {
 	static struct lock_file head_lock;
 	struct strbuf buf = STRBUF_INIT;
 	int fd;
+	ssize_t written;
 
 	fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);
 	if (fd < 0) {
 		rollback_lock_file(&head_lock);
 		return error_errno(_("could not lock HEAD"));
 	}
 	strbuf_addf(&buf, "%s\n", head);
-	if (write_in_full(fd, buf.buf, buf.len) < 0) {
+	written = write_in_full(fd, buf.buf, buf.len);
+	strbuf_release(&buf);
+	if (written < 0) {
 		rollback_lock_file(&head_lock);
 		return error_errno(_("could not write to '%s'"),
 				   git_path_head_file());
 	}
 	if (commit_lock_file(&head_lock) < 0) {
 		rollback_lock_file(&head_lock);
 		return error(_("failed to finalize '%s'."), git_path_head_file());
 	}
 	return 0;
 }
-- 
2.14.1

[PATCH 30/34] userdiff: release strbuf after use in userdiff_get_textconv()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 userdiff.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/userdiff.c b/userdiff.c
index 2c1502f719..6321103ce2 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -284,16 +284,17 @@ struct userdiff_driver *userdiff_find_by_path(const char *path)
 struct userdiff_driver *userdiff_get_textconv(struct userdiff_driver *driver)
 {
 	if (!driver->textconv)
 		return NULL;
 
 	if (driver->textconv_want_cache && !driver->textconv_cache) {
 		struct notes_cache *c = xmalloc(sizeof(*c));
 		struct strbuf name = STRBUF_INIT;
 
 		strbuf_addf(&name, "textconv/%s", driver->name);
 		notes_cache_init(c, name.buf, driver->textconv);
 		driver->textconv_cache = c;
+		strbuf_release(&name);
 	}
 
 	return driver;
 }
-- 
2.14.1

[PATCH 29/34] transport-helper: release strbuf after use in process_connect_service()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 transport-helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/transport-helper.c b/transport-helper.c
index 8f68d69a86..519a244583 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -548,62 +548,63 @@ static int fetch_with_import(struct transport *transport,
 static int process_connect_service(struct transport *transport,
 				   const char *name, const char *exec)
 {
 	struct helper_data *data = transport->data;
 	struct strbuf cmdbuf = STRBUF_INIT;
 	struct child_process *helper;
 	int r, duped, ret = 0;
 	FILE *input;
 
 	helper = get_helper(transport);
 
 	/*
 	 * Yes, dup the pipe another time, as we need unbuffered version
 	 * of input pipe as FILE*. fclose() closes the underlying fd and
 	 * stream buffering only can be changed before first I/O operation
 	 * on it.
 	 */
 	duped = dup(helper->out);
 	if (duped < 0)
 		die_errno("Can't dup helper output fd");
 	input = xfdopen(duped, "r");
 	setvbuf(input, NULL, _IONBF, 0);
 
 	/*
 	 * Handle --upload-pack and friends. This is fire and forget...
 	 * just warn if it fails.
 	 */
 	if (strcmp(name, exec)) {
 		r = set_helper_option(transport, "servpath", exec);
 		if (r > 0)
 			warning("Setting remote service path not supported by protocol.");
 		else if (r < 0)
 			warning("Invalid remote service path.");
 	}
 
 	if (data->connect)
 		strbuf_addf(&cmdbuf, "connect %s\n", name);
 	else
 		goto exit;
 
 	sendline(data, &cmdbuf);
 	if (recvline_fh(input, &cmdbuf, name))
 		exit(128);
 
 	if (!strcmp(cmdbuf.buf, "")) {
 		data->no_disconnect_req = 1;
 		if (debug)
 			fprintf(stderr, "Debug: Smart transport connection "
 				"ready.\n");
 		ret = 1;
 	} else if (!strcmp(cmdbuf.buf, "fallback")) {
 		if (debug)
 			fprintf(stderr, "Debug: Falling back to dumb "
 				"transport.\n");
 	} else
 		die("Unknown response to connect: %s",
 			cmdbuf.buf);
 
 exit:
+	strbuf_release(&cmdbuf);
 	fclose(input);
 	return ret;
 }
-- 
2.14.1

[PATCH 31/34] utf8: release strbuf on error return in strbuf_utf8_replace()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 utf8.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/utf8.c b/utf8.c
index 0c8e011a58..47a42047c8 100644
--- a/utf8.c
+++ b/utf8.c
@@ -354,49 +354,50 @@ void strbuf_add_wrapped_bytes(struct strbuf *buf, const char *data, int len,
 void strbuf_utf8_replace(struct strbuf *sb_src, int pos, int width,
 			 const char *subst)
 {
 	struct strbuf sb_dst = STRBUF_INIT;
 	char *src = sb_src->buf;
 	char *end = src + sb_src->len;
 	char *dst;
 	int w = 0, subst_len = 0;
 
 	if (subst)
 		subst_len = strlen(subst);
 	strbuf_grow(&sb_dst, sb_src->len + subst_len);
 	dst = sb_dst.buf;
 
 	while (src < end) {
 		char *old;
 		size_t n;
 
 		while ((n = display_mode_esc_sequence_len(src))) {
 			memcpy(dst, src, n);
 			src += n;
 			dst += n;
 		}
 
 		if (src >= end)
 			break;
 
 		old = src;
 		n = utf8_width((const char**)&src, NULL);
 		if (!src) 	/* broken utf-8, do nothing */
-			return;
+			goto out;
 		if (n && w >= pos && w < pos + width) {
 			if (subst) {
 				memcpy(dst, subst, subst_len);
 				dst += subst_len;
 				subst = NULL;
 			}
 			w += n;
 			continue;
 		}
 		memcpy(dst, old, src - old);
 		dst += src - old;
 		w += n;
 	}
 	strbuf_setlen(&sb_dst, dst - sb_dst.buf);
 	strbuf_swap(sb_src, &sb_dst);
+out:
 	strbuf_release(&sb_dst);
 }
 
-- 
2.14.1

[PATCH 32/34] vcs-svn: release strbuf after use in end_revision()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 vcs-svn/svndump.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index ec6b350611..08d136b8cc 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -311,13 +311,14 @@ static void begin_revision(const char *remote_ref)
 static void end_revision(const char *note_ref)
 {
 	struct strbuf mark = STRBUF_INIT;
 	if (rev_ctx.revision) {
 		fast_export_end_commit(rev_ctx.revision);
 		fast_export_begin_note(rev_ctx.revision, "remote-svn",
 				"Note created by remote-svn.", rev_ctx.timestamp, note_ref);
 		strbuf_addf(&mark, ":%"PRIu32, rev_ctx.revision);
 		fast_export_note(mark.buf, "inline");
 		fast_export_buf_to_data(&rev_ctx.note);
+		strbuf_release(&mark);
 	}
 }
 
-- 
2.14.1

[PATCH 33/34] wt-status: release strbuf after use in read_rebase_todolist()

[Rene Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 wt-status.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/wt-status.c b/wt-status.c
index 77c27c5113..cafafb5ecd 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1175,24 +1175,25 @@ static void abbrev_sha1_in_line(struct strbuf *line)
 static int read_rebase_todolist(const char *fname, struct string_list *lines)
 {
 	struct strbuf line = STRBUF_INIT;
 	FILE *f = fopen(git_path("%s", fname), "r");
 
 	if (!f) {
 		if (errno == ENOENT)
 			return -1;
 		die_errno("Could not open file %s for reading",
 			  git_path("%s", fname));
 	}
 	while (!strbuf_getline_lf(&line, f)) {
 		if (line.len && line.buf[0] == comment_line_char)
 			continue;
 		strbuf_trim(&line);
 		if (!line.len)
 			continue;
 		abbrev_sha1_in_line(&line);
 		string_list_append(lines, line.buf);
 	}
 	fclose(f);
+	strbuf_release(&line);
 	return 0;
 }
 
-- 
2.14.1

[PATCH 34/34] wt-status: release strbuf after use in wt_longstatus_print_tracking()

[Rene Scharfe]

If format_tracking_info() returns 0 only if it didn't touch its strbuf
parameter, so it's OK to exit early in that case.  Clean up sb in the
other case.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 wt-status.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/wt-status.c b/wt-status.c
index cafafb5ecd..ac972acbab 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -998,34 +998,35 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
 static void wt_longstatus_print_tracking(struct wt_status *s)
 {
 	struct strbuf sb = STRBUF_INIT;
 	const char *cp, *ep, *branch_name;
 	struct branch *branch;
 	char comment_line_string[3];
 	int i;
 
 	assert(s->branch && !s->is_initial);
 	if (!skip_prefix(s->branch, "refs/heads/", &branch_name))
 		return;
 	branch = branch_get(branch_name);
 	if (!format_tracking_info(branch, &sb))
 		return;
 
 	i = 0;
 	if (s->display_comment_prefix) {
 		comment_line_string[i++] = comment_line_char;
 		comment_line_string[i++] = ' ';
 	}
 	comment_line_string[i] = '\0';
 
 	for (cp = sb.buf; (ep = strchr(cp, '\n')) != NULL; cp = ep + 1)
 		color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s),
 				 "%s%.*s", comment_line_string,
 				 (int)(ep - cp), cp);
 	if (s->display_comment_prefix)
 		color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), "%c",
 				 comment_line_char);
 	else
 		fputs("\n", s->fp);
+	strbuf_release(&sb);
 }
 
 static int has_unmerged(struct wt_status *s)
-- 
2.14.1

Re: [PATCH 34/34] wt-status: release strbuf after use in wt_longstatus_print_tracking()

[Junio C Hamano]

Rene Scharfe <l.s.r@web.de> writes:

> If format_tracking_info() returns 0 only if it didn't touch its strbuf
> parameter, so it's OK to exit early in that case.  Clean up sb in the
> other case.

These two "if"s confuse me; perhaps the first one is not needed?

Re: [PATCH 34/34] wt-status: release strbuf after use in wt_longstatus_print_tracking()

[René Scharfe]

Am 06.09.2017 um 21:51 schrieb Junio C Hamano:
> Rene Scharfe <l.s.r@web.de> writes:
> 
>> If format_tracking_info() returns 0 only if it didn't touch its strbuf
>> parameter, so it's OK to exit early in that case.  Clean up sb in the
>> other case.
> 
> These two "if"s confuse me; perhaps the first one is not needed?

Yes, removing it looks like the best way to make that sentence clearer.
Another would be to replace "only if" with "then".

René

Re: [PATCH 34/34] wt-status: release strbuf after use in wt_longstatus_print_tracking()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 06.09.2017 um 21:51 schrieb Junio C Hamano:
>> Rene Scharfe <l.s.r@web.de> writes:
>> 
>>> If format_tracking_info() returns 0 only if it didn't touch its strbuf
>>> parameter, so it's OK to exit early in that case.  Clean up sb in the
>>> other case.
>> 
>> These two "if"s confuse me; perhaps the first one is not needed?
>
> Yes, removing it looks like the best way to make that sentence clearer.
> Another would be to replace "only if" with "then".

Either makes it understandable. Thanks.

Re: [PATCH 00/34] plug strbuf memory leaks

[Stefan Beller]

On Wed, Aug 30, 2017 at 10:49 AM, Rene Scharfe <l.s.r@web.de> wrote:
> Release allocated strbufs in functions that are at least potentionally
> library-like; cmd_*() functions are out of scope because the process
> ends with them and the OS cleans up for us anyway.  The patches are
> split by function and were generated with --function-context for easier
> reviewing.

Thanks for this series! All patches look good to me.
Stefan

Re: [PATCH 00/34] plug strbuf memory leaks

[Junio C Hamano]

Rene Scharfe <l.s.r@web.de> writes:

> Release allocated strbufs in functions that are at least potentionally
> library-like; cmd_*() functions are out of scope because the process
> ends with them and the OS cleans up for us anyway.  The patches are
> split by function and were generated with --function-context for easier
> reviewing.

I read through all of them and they looked sensible.  I had a few
comments on individual patches, though.