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

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

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

- used 0 (black) for the foreground color attributes in winansi when we
  have no console to print to, anyway.

- clarified in the commit message when we hit the path, and why, where
  we set the foreground color attributes to "all black".

- reworded the commit message talking about splitting the PATH (on
  Windows, it is delimited by semicolons, not colons, but it is even
  better to just talk about the path delimiters because it does not
  really happen which character is used, but it is important which role
  it plays).

- rewrote the split_commit_in_progress() function to have a more natural
  flow while still fixing the memory leak.


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(): simplify & 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         | 12 ++++++++++++
 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              | 29 +++++++++++++++--------------
 23 files changed, 148 insertions(+), 64 deletions(-)


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

Interdiff vs v3:

 diff --git a/compat/winansi.c b/compat/winansi.c
 index 861b79d8c31..a11a0f16d27 100644
 --- a/compat/winansi.c
 +++ b/compat/winansi.c
 @@ -105,8 +105,13 @@ static int is_console(int fd)
  	if (!fd) {
  		if (!GetConsoleMode(hcon, &mode))
  			return 0;
 -		sbi.wAttributes = FOREGROUND_BLUE | FOREGROUND_GREEN |
 -			FOREGROUND_RED;
 +		/*
 +		 * This code path is only reached if there is no console
 +		 * attached to stdout/stderr, i.e. we will not need to output
 +		 * any text to any console, therefore we might just as well
 +		 * use black as foreground color.
 +		 */
 +		sbi.wAttributes = 0;
  	} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
  		return 0;
  
 diff --git a/wt-status.c b/wt-status.c
 index 1f3f6bcb980..117ac8cfb01 100644
 --- a/wt-status.c
 +++ b/wt-status.c
 @@ -1082,34 +1082,29 @@ static char *read_line_from_git_path(const char *filename)
  static int split_commit_in_progress(struct wt_status *s)
  {
  	int split_in_progress = 0;
 -	char *head = read_line_from_git_path("HEAD");
 -	char *orig_head = read_line_from_git_path("ORIG_HEAD");
 -	char *rebase_amend = read_line_from_git_path("rebase-merge/amend");
 -	char *rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
 -
 -	if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
 -	    !s->branch || strcmp(s->branch, "HEAD")) {
 -		free(head);
 -		free(orig_head);
 -		free(rebase_amend);
 -		free(rebase_orig_head);
 -		return split_in_progress;
 -	}
 -
 -	if (!strcmp(rebase_amend, rebase_orig_head)) {
 -		if (strcmp(head, rebase_amend))
 -			split_in_progress = 1;
 -	} else if (strcmp(orig_head, rebase_orig_head)) {
 -		split_in_progress = 1;
 -	}
 +	char *head, *orig_head, *rebase_amend, *rebase_orig_head;
 +
 +	if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
 +	    !s->branch || strcmp(s->branch, "HEAD"))
 +		return 0;
  
 -	if (!s->amend && !s->nowarn && !s->workdir_dirty)
 -		split_in_progress = 0;
 +	head = read_line_from_git_path("HEAD");
 +	orig_head = read_line_from_git_path("ORIG_HEAD");
 +	rebase_amend = read_line_from_git_path("rebase-merge/amend");
 +	rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
 +
 +	if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
 +		; /* fall through, no split in progress */
 +	else if (!strcmp(rebase_amend, rebase_orig_head))
 +		split_in_progress = !!strcmp(head, rebase_amend);
 +	else if (strcmp(orig_head, rebase_orig_head))
 +		split_in_progress = 1;
  
  	free(head);
  	free(orig_head);
  	free(rebase_amend);
  	free(rebase_orig_head);
 +
  	return split_in_progress;
  }
  

-- 
2.12.2.windows.2.800.gede8f145e06