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

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

[-- Attachment #1: Type: text/plain, Size: 8886 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 v2:

- renamed the `p` variables introduced by the patch series to the more
  explanatory `to_free`.

- reworded incorrect commit message claiming that
  setup_discovered_git_dir() was using a static variable that is
  actually a singleton

- reverted a code move that would have resulted in accessing
  uninitialized data of callers of mailinfo() that do not die() right
  away but clean up faithfully


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(): plug memory leak
  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      | 10 ++++++++++
 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               |  9 ++++++++-
 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, 135 insertions(+), 51 deletions(-)


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

Interdiff vs v2:

 diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
 index 9b3efc8e987..664400b8169 100644
 --- a/builtin/mailsplit.c
 +++ b/builtin/mailsplit.c
 @@ -232,9 +232,18 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,
  
  	do {
  		peek = fgetc(f);
 +		if (peek == EOF) {
 +			if (f == stdin)
 +				/* empty stdin is OK */
 +				ret = skip;
 +			else {
 +				fclose(f);
 +				error(_("empty mbox: '%s'"), file);
 +			}
 +			goto out;
 +		}
  	} while (isspace(peek));
 -	if (peek != EOF)
 -		ungetc(peek, f);
 +	ungetc(peek, f);
  
  	if (strbuf_getwholeline(&buf, f, '\n')) {
  		/* empty stdin is OK */
 diff --git a/builtin/mktree.c b/builtin/mktree.c
 index f0354bc9718..da0fd8cd706 100644
 --- a/builtin/mktree.c
 +++ b/builtin/mktree.c
 @@ -72,7 +72,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss
  	unsigned mode;
  	enum object_type mode_type; /* object type derived from mode */
  	enum object_type obj_type; /* object type derived from sha */
 -	char *path, *p = NULL;
 +	char *path, *to_free = NULL;
  	unsigned char sha1[20];
  
  	ptr = buf;
 @@ -102,7 +102,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss
  		struct strbuf p_uq = STRBUF_INIT;
  		if (unquote_c_style(&p_uq, path, NULL))
  			die("invalid quoting");
 -		path = p = strbuf_detach(&p_uq, NULL);
 +		path = to_free = strbuf_detach(&p_uq, NULL);
  	}
  
  	/*
 @@ -136,7 +136,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss
  	}
  
  	append_to_tree(mode, sha1, path);
 -	free(p);
 +	free(to_free);
  }
  
  int cmd_mktree(int ac, const char **av, const char *prefix)
 diff --git a/builtin/name-rev.c b/builtin/name-rev.c
 index a4ce73fb1e9..e7a3fe7ee70 100644
 --- a/builtin/name-rev.c
 +++ b/builtin/name-rev.c
 @@ -28,7 +28,7 @@ static void name_rev(struct commit *commit,
  	struct rev_name *name = (struct rev_name *)commit->util;
  	struct commit_list *parents;
  	int parent_number = 1;
 -	char *p = NULL;
 +	char *to_free = NULL;
  
  	parse_commit(commit);
  
 @@ -36,7 +36,7 @@ static void name_rev(struct commit *commit,
  		return;
  
  	if (deref) {
 -		tip_name = p = xstrfmt("%s^0", tip_name);
 +		tip_name = to_free = xstrfmt("%s^0", tip_name);
  
  		if (generation)
  			die("generation: %d, but deref?", generation);
 @@ -55,7 +55,7 @@ static void name_rev(struct commit *commit,
  		name->generation = generation;
  		name->distance = distance;
  	} else {
 -		free(p);
 +		free(to_free);
  		return;
  	}
  
 diff --git a/mailinfo.c b/mailinfo.c
 index a319911b510..f92cb9f729c 100644
 --- a/mailinfo.c
 +++ b/mailinfo.c
 @@ -1097,6 +1097,9 @@ 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);
  		if (peek == EOF) {
 @@ -1106,9 +1109,6 @@ int mailinfo(struct mailinfo *mi, const char *msg, const char *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/setup.c b/setup.c
 index 12efca85a41..e3f7699a902 100644
 --- a/setup.c
 +++ b/setup.c
 @@ -703,15 +703,15 @@ static const char *setup_discovered_git_dir(const char *gitdir,
  
  	/* --work-tree is set without --git-dir; use discovered one */
  	if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
 -		char *p = NULL;
 +		char *to_free = NULL;
  		const char *ret;
  
  		if (offset != cwd->len && !is_absolute_path(gitdir))
 -			gitdir = p = real_pathdup(gitdir, 1);
 +			gitdir = to_free = real_pathdup(gitdir, 1);
  		if (chdir(cwd->buf))
  			die_errno("Could not come back to cwd");
  		ret = setup_explicit_git_dir(gitdir, cwd, nongit_ok);
 -		free(p);
 +		free(to_free);
  		return ret;
  	}
  

-- 
2.12.2.windows.2.800.gede8f145e06