[PATCH v2 00/25] Address a couple of issues identified by Coverity

[Johannes Schindelin <johannes.schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 14159 bytes --]

I recently registered the git-for-windows fork with Coverity to ensure
that even the Windows-specific patches get some static analysis love.

While at it, I squashed a couple of obvious issues in the part that is
not Windows-specific.

Note: while this patch series squashes some of those issues, the
remaining issues are not necessarily all false positives (e.g. Coverity
getting fooled by our FLEX_ARRAY trick into believing that the last
member of the struct is indeed a 0 or 1 size array) or intentional (e.g.
builtins not releasing memory because exit() is called right after
returning from the function that leaks memory).

Notable examples of the remaining issues are:

- a couple of callers of shorten_unambiguous_ref() assume that they do
  not have to release the memory returned from that function, often
  assigning the pointer to a `const` variable that typically does not
  hold a pointer that needs to be free()d. My hunch is that we will want
  to convert that function to have a fixed number of static buffers and
  use those in a round robin fashion à la sha1_to_hex().

- pack-redundant.c seems to have hard-to-reason-about code paths that
  may eventually leak memory. Essentially, the custody of the allocated
  memory is not clear at all.

- fast-import.c calls strbuf_detach() on the command_buf without any
  obvious rationale. Turns out that *some* code paths assign
  command_buf.buf to a `recent_command` which releases the buffer later.
  However, from a cursory look it appears to me as if there are some
  other code paths that skip that assignment, effectively causing a memory
  leak once strbuf_detach() is called.

Sadly, I lack the time to pursue those remaining issues further.

Changes since v1:

- clarified in the commit messages for the setup_*git_dir() functions
  that we are not really plugging a memory leak, but marking singletons
  as `static` to help Coverity not to complain about this again

- dropped the patch to http-backend, as it is supposedly a one-shot
  program using exit() as "garbage collector".

- the difftool patch does a more thorough job of fixing leaks now

- reworded the commit subject of the mailinfo/mailsplit patch to sport
  a prefix indicating the overall area of the fix

- changed the mailinfo/mailsplit patch to *really* handle EOF correctly

- simplified the patch to get_mail_commit_oid(), and now it even returns
  the correct error value!

- adjusted the comment in builtin/checkout.c that talked about leaking
  the cache_entry (which is not leaked anymore)

- add forgotten free(buf) in fast-export.c in an early return

- line-log data is now released properly

- a couple more memory leaks in add_reflog_for_walk() have been found
  and addressed (this one *really* needs a couple more eyeballs, though,
  it is just a much bigger change than I originally anticipated)


Johannes Schindelin (25):
  mingw: avoid memory leak when splitting PATH
  winansi: avoid use of uninitialized value
  winansi: avoid buffer overrun
  add_commit_patch_id(): avoid allocating memory unnecessarily
  git_config_rename_section_in_file(): avoid resource leak
  get_mail_commit_oid(): avoid resource leak
  difftool: address a couple of resource/memory leaks
  status: close file descriptor after reading git-rebase-todo
  mailinfo & mailsplit: check for EOF while parsing
  cat-file: fix memory leak
  checkout: fix memory leak
  split_commit_in_progress(): fix memory leak
  setup_bare_git_dir(): help static analysis
  setup_discovered_git_dir(): help static analysis
  pack-redundant: plug memory leak
  mktree: plug memory leaks reported by Coverity
  fast-export: avoid leaking memory in handle_tag()
  receive-pack: plug memory leak in update()
  line-log: avoid memory leak
  shallow: avoid memory leak
  add_reflog_for_walk: avoid memory leak
  remote: plug memory leak in match_explicit()
  name-rev: avoid leaking memory in the `deref` case
  show_worktree(): plug memory leak
  submodule_uses_worktrees(): plug memory leak

 builtin/am.c             | 15 ++++++---------
 builtin/cat-file.c       |  1 +
 builtin/checkout.c       | 17 +++++++++--------
 builtin/difftool.c       | 33 +++++++++++++++++++++++----------
 builtin/fast-export.c    |  2 ++
 builtin/mailsplit.c      |  3 ++-
 builtin/mktree.c         |  5 +++--
 builtin/name-rev.c       |  7 +++++--
 builtin/pack-redundant.c |  1 +
 builtin/receive-pack.c   |  4 +++-
 builtin/worktree.c       |  8 +++++---
 compat/mingw.c           |  4 +++-
 compat/winansi.c         |  7 +++++++
 config.c                 |  5 ++++-
 line-log.c               |  1 +
 mailinfo.c               | 15 +++++++++++----
 patch-ids.c              |  3 ++-
 reflog-walk.c            | 20 +++++++++++++++++---
 remote.c                 |  5 +++--
 setup.c                  | 11 ++++++++---
 shallow.c                |  8 ++++++--
 worktree.c               |  2 +-
 wt-status.c              |  8 +++++++-
 23 files changed, 130 insertions(+), 55 deletions(-)


base-commit: 027a3b943b444a3e3a76f9a89803fc10245b858f
Published-As: https://github.com/dscho/git/releases/tag/coverity-v2
Fetch-It-Via: git fetch https://github.com/dscho/git coverity-v2

Interdiff vs v1:

 diff --git a/builtin/am.c b/builtin/am.c
 index 067dd4fc20d..9c5c2c778e2 100644
 --- a/builtin/am.c
 +++ b/builtin/am.c
 @@ -1353,18 +1353,14 @@ static int get_mail_commit_oid(struct object_id *commit_id, const char *mail)
  	const char *x;
  	int ret = 0;
  
 -	if (strbuf_getline_lf(&sb, fp))
 -		ret = -1;
 -
 -	if (!ret && !skip_prefix(sb.buf, "From ", &x))
 -		ret = -1;
 -
 -	if (!ret && get_oid_hex(x, commit_id) < 0)
 +	if (strbuf_getline_lf(&sb, fp) ||
 +	    !skip_prefix(sb.buf, "From ", &x) ||
 +	    get_oid_hex(x, commit_id) < 0)
  		ret = -1;
  
  	strbuf_release(&sb);
  	fclose(fp);
 -	return 0;
 +	return ret;
  }
  
  /**
 diff --git a/builtin/checkout.c b/builtin/checkout.c
 index 98f98256608..5faea3a05fa 100644
 --- a/builtin/checkout.c
 +++ b/builtin/checkout.c
 @@ -235,14 +235,14 @@ static int checkout_merged(int pos, const struct checkout *state)
  	/*
  	 * NEEDSWORK:
  	 * There is absolutely no reason to write this as a blob object
 -	 * and create a phony cache entry just to leak.  This hack is
 -	 * primarily to get to the write_entry() machinery that massages
 -	 * the contents to work-tree format and writes out which only
 -	 * allows it for a cache entry.  The code in write_entry() needs
 -	 * to be refactored to allow us to feed a <buffer, size, mode>
 -	 * instead of a cache entry.  Such a refactoring would help
 -	 * merge_recursive as well (it also writes the merge result to the
 -	 * object database even when it may contain conflicts).
 +	 * and create a phony cache entry.  This hack is primarily to get
 +	 * to the write_entry() machinery that massages the contents to
 +	 * work-tree format and writes out which only allows it for a
 +	 * cache entry.  The code in write_entry() needs to be refactored
 +	 * to allow us to feed a <buffer, size, mode> instead of a cache
 +	 * entry.  Such a refactoring would help merge_recursive as well
 +	 * (it also writes the merge result to the object database even
 +	 * when it may contain conflicts).
  	 */
  	if (write_sha1_file(result_buf.ptr, result_buf.size,
  			    blob_type, oid.hash))
 diff --git a/builtin/difftool.c b/builtin/difftool.c
 index a4f1d117ef6..b9a892f2693 100644
 --- a/builtin/difftool.c
 +++ b/builtin/difftool.c
 @@ -440,8 +440,10 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
  		}
  
  		if (lmode && status != 'C') {
 -			if (checkout_path(lmode, &loid, src_path, &lstate))
 -				return error("could not write '%s'", src_path);
 +			if (checkout_path(lmode, &loid, src_path, &lstate)) {
 +				ret = error("could not write '%s'", src_path);
 +				goto finish;
 +			}
  		}
  
  		if (rmode && !S_ISLNK(rmode)) {
 @@ -457,9 +459,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
  			hashmap_add(&working_tree_dups, entry);
  
  			if (!use_wt_file(workdir, dst_path, &roid)) {
 -				if (checkout_path(rmode, &roid, dst_path, &rstate))
 -					return error("could not write '%s'",
 -						     dst_path);
 +				if (checkout_path(rmode, &roid, dst_path,
 +						  &rstate)) {
 +					ret = error("could not write '%s'",
 +						    dst_path);
 +					goto finish;
 +				}
  			} else if (!is_null_oid(&roid)) {
  				/*
  				 * Changes in the working tree need special
 @@ -474,10 +479,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
  						ADD_CACHE_JUST_APPEND);
  
  				add_path(&rdir, rdir_len, dst_path);
 -				if (ensure_leading_directories(rdir.buf))
 -					return error("could not create "
 -						     "directory for '%s'",
 -						     dst_path);
 +				if (ensure_leading_directories(rdir.buf)) {
 +					ret = error("could not create "
 +						    "directory for '%s'",
 +						    dst_path);
 +					goto finish;
 +				}
  				add_path(&wtdir, wtdir_len, dst_path);
  				if (symlinks) {
  					if (symlink(wtdir.buf, rdir.buf)) {
 @@ -499,13 +506,14 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
  	}
  
  	fclose(fp);
 +	fp = NULL;
  	if (finish_command(&child)) {
  		ret = error("error occurred running diff --raw");
  		goto finish;
  	}
  
  	if (!i)
 -		return 0;
 +		goto finish;
  
  	/*
  	 * Changes to submodules require special treatment.This loop writes a
 @@ -628,6 +636,9 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
  		exit_cleanup(tmpdir, rc);
  
  finish:
 +	if (fp)
 +		fclose(fp);
 +
  	free(lbase_dir);
  	free(rbase_dir);
  	strbuf_release(&ldir);
 diff --git a/builtin/fast-export.c b/builtin/fast-export.c
 index 828d41c0c11..64617ad8e36 100644
 --- a/builtin/fast-export.c
 +++ b/builtin/fast-export.c
 @@ -734,6 +734,7 @@ static void handle_tag(const char *name, struct tag *tag)
  			     oid_to_hex(&tag->object.oid));
  		case DROP:
  			/* Ignore this tag altogether */
 +			free(buf);
  			return;
  		case REWRITE:
  			if (tagged->type != OBJ_COMMIT) {
 diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
 index c0d88f97512..9b3efc8e987 100644
 --- a/builtin/mailsplit.c
 +++ b/builtin/mailsplit.c
 @@ -232,8 +232,9 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,
  
  	do {
  		peek = fgetc(f);
 -	} while (peek >= 0 && isspace(peek));
 -	ungetc(peek, f);
 +	} while (isspace(peek));
 +	if (peek != EOF)
 +		ungetc(peek, f);
  
  	if (strbuf_getwholeline(&buf, f, '\n')) {
  		/* empty stdin is OK */
 diff --git a/http-backend.c b/http-backend.c
 index d12572fda10..eef0a361f4f 100644
 --- a/http-backend.c
 +++ b/http-backend.c
 @@ -681,10 +681,8 @@ int cmd_main(int argc, const char **argv)
  		if (!regexec(&re, dir, 1, out, 0)) {
  			size_t n;
  
 -			if (strcmp(method, c->method)) {
 -				free(dir);
 +			if (strcmp(method, c->method))
  				return bad_request(&hdr, c);
 -			}
  
  			cmd = c;
  			n = out[0].rm_eo - out[0].rm_so;
 @@ -710,7 +708,5 @@ int cmd_main(int argc, const char **argv)
  					   max_request_buffer);
  
  	cmd->imp(&hdr, cmd_arg);
 -	free(dir);
 -	free(cmd_arg);
  	return 0;
  }
 diff --git a/line-log.c b/line-log.c
 index 19d46e9ea2c..b9087814b8c 100644
 --- a/line-log.c
 +++ b/line-log.c
 @@ -1125,7 +1125,7 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c
  	changed = process_all_files(&parent_range, rev, &queue, range);
  	if (parent)
  		add_line_range(rev, parent, parent_range);
 -	free(parent_range);
 +	free_line_log_data(parent_range);
  	return changed;
  }
  
 diff --git a/mailinfo.c b/mailinfo.c
 index 60dcad7b714..a319911b510 100644
 --- a/mailinfo.c
 +++ b/mailinfo.c
 @@ -882,7 +882,10 @@ static int read_one_header_line(struct strbuf *line, FILE *in)
  	for (;;) {
  		int peek;
  
 -		peek = fgetc(in); ungetc(peek, in);
 +		peek = fgetc(in);
 +		if (peek == EOF)
 +			break;
 +		ungetc(peek, in);
  		if (peek != ' ' && peek != '\t')
  			break;
  		if (strbuf_getline_lf(&continuation, in))
 @@ -1094,14 +1097,18 @@ int mailinfo(struct mailinfo *mi, const char *msg, const char *patch)
  		return -1;
  	}
  
 -	mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
 -	mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
 -
  	do {
  		peek = fgetc(mi->input);
 -	} while (peek >= 0 && isspace(peek));
 +		if (peek == EOF) {
 +			fclose(cmitmsg);
 +			return error("empty patch: '%s'", patch);
 +		}
 +	} while (isspace(peek));
  	ungetc(peek, mi->input);
  
 +	mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
 +	mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
 +
  	/* process the email header */
  	while (read_one_header_line(&line, mi->input))
  		check_header(mi, &line, mi->p_hdr_data, 1);
 diff --git a/reflog-walk.c b/reflog-walk.c
 index ec66f2b16e6..c63eb1a3fd7 100644
 --- a/reflog-walk.c
 +++ b/reflog-walk.c
 @@ -197,17 +197,27 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
  				reflogs = read_complete_reflog(branch);
  			}
  		}
 -		if (!reflogs || reflogs->nr == 0)
 +		if (!reflogs || reflogs->nr == 0) {
 +			if (reflogs) {
 +				free(reflogs->ref);
 +				free(reflogs);
 +			}
 +			free(branch);
  			return -1;
 +		}
  		string_list_insert(&info->complete_reflogs, branch)->util
  			= reflogs;
  	}
 +	free(branch);
  
  	commit_reflog = xcalloc(1, sizeof(struct commit_reflog));
  	if (recno < 0) {
  		commit_reflog->recno = get_reflog_recno_by_time(reflogs, timestamp);
  		if (commit_reflog->recno < 0) {
 -			free(branch);
 +			if (reflogs) {
 +				free(reflogs->ref);
 +				free(reflogs);
 +			}
  			free(commit_reflog);
  			return -1;
  		}

-- 
2.12.2.windows.2.800.gede8f145e06