patch, 26 partsAddress a couple of issues identified by Coverity

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.

Johannes Schindelin (26):
  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
  http-backend: avoid memory leaks
  difftool: close file descriptors after reading
  status: close file descriptor after reading git-rebase-todo
  Check for EOF while parsing mails
  cat-file: fix memory leak
  checkout: fix memory leak
  split_commit_in_progress(): fix memory leak
  setup_bare_git_dir(): fix memory leak
  setup_discovered_git_dir(): fix 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             | 11 ++++++-----
 builtin/cat-file.c       |  1 +
 builtin/checkout.c       |  1 +
 builtin/difftool.c       |  2 ++
 builtin/fast-export.c    |  1 +
 builtin/mailsplit.c      |  2 +-
 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 ++++-
 http-backend.c           |  6 +++++-
 line-log.c               |  1 +
 mailinfo.c               |  2 +-
 patch-ids.c              |  3 ++-
 reflog-walk.c            |  6 +++++-
 remote.c                 |  5 +++--
 setup.c                  | 11 ++++++++---
 shallow.c                |  8 ++++++--
 worktree.c               |  2 +-
 wt-status.c              |  8 +++++++-
 24 files changed, 82 insertions(+), 29 deletions(-)

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

In the (admittedly, concocted) case that PATH consists only of colons, we would leak the duplicated string.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

When stdout is not connected to a Win32 console, we incorrectly used an uninitialized value for the "plain" character attributes.

Detected by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 2 ++
 1 file changed, 2 insertions(+)

When we could not convert the UTF-8 sequence into Unicode for writing to the Console, we should not try to write an insanely-long sequence of invalid wide characters (mistaking the negative return value for an unsigned length).

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 5 +++++
 1 file changed, 5 insertions(+)

It would appear that we allocate (and forget to release) memory if the patch ID is not even defined.

Reported by the Coverity tool.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 patch-ids.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

When we fail to read, or parse, the file, we still want to close the file descriptor and release the strbuf.

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/am.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

On Wed, Apr 26, 2017 at 1:19 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

In similar cases of fixing mem leaks, we'd put a label here and make excessive use of goto, instead of setting ret to -1. As "ret" and the commands are short, this is visually just as appealing.

Thanks, Stefan

Stefan Beller <sbeller@google.com> writes:

I wouldn't call that visually appealing. Fixing resource leaks is good, and only because this function is short enough and the funny way to skip over various operation need to last for just a few instructions, it is tolerable. If the function were shorter, then we may have used

	if (!strbuf_getline_lf() &&
	    skip_prefix() &&
	    !get_oid_hex())
		ret = 0; /* happy */
	else
		ret = -1;
	release resources here;
        return ret;

and that would have been OK. If longer, as you said, jumping to a label at the end of function to release the acquired resource would be a lot more maintainable way than either of the above alternatives that are only usable for short functions.

The patch as posted makes the function fail to return -1 when it finds problems, by the way. It needs to return "ret" not the hardcoded "0" at the end.

Hi Junio & Stefan,

On Wed, 26 Apr 2017, Junio C Hamano wrote:

I did almost what you suggested here, but I avoided the explicit ret = 0, as it is initialized that way.

Oops.

Ciao, Dscho

Am 26.04.2017 um 22:19 schrieb Johannes Schindelin:

You forgot to 'return ret;', didn't you?

-- Hannes

Hi Hannes,

On Thu, 27 Apr 2017, Johannes Sixt wrote:

I sure did!

Thanks, Dscho

In case of errors, we really want the file descriptor to be closed.

Discovered by a Coverity scan.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 config.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Spotted by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/difftool.c | 2 ++
 1 file changed, 2 insertions(+)

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

This one looks sensible.

The huge loop we see in the pre-context of this hunk has many "return"s and "goto finish"es that can leave fp still open; while this patch does not hurt, it is probably somewhat insufficient.

Hi Junio,

On Wed, 26 Apr 2017, Junio C Hamano wrote:

You are absolutely correct, and I am somewhat surprised that Coverity did not complain more loudly.

TBH I really only tried to address these fixes on the cheap, as my main aim was to get to the critical bugs in the Windows-specific part (I did not find anything major, though). Therefore, I only looked at what Coverity labels as the "high impact" issues. The leaks in the loop that you pointed out may be labeled as minor by Coverity.

I also noticed that a couple of error messages have not been marked as translateable, I must have forgotten that before submitting the difftool patch series :-(

In any case, in the next iteration of this patch series, this patch will convert all returns to `ret = ...; goto finish;` calls, and fclose(fp) there unless it has been closed already.

Thanks, Dscho

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 http-backend.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Hmph. I find a "leak" of a resource acquired inside the main function and not released when the main function leaves a lot less interesting than the other ones this series covers.

When the "return" in the first hunk leaves the function and also the same block of the if () statement breaks out the loop, we also fail to call regfree(&re). Shouldn't we be cleaning it up as well?

Hi Junio,

On Wed, 26 Apr 2017, Junio C Hamano wrote:

Ah, I missed that this falls squarely into the "one-shot programs are allowed to be sloppy in their memory management, essentially using exit() as garbage collector" category.

Will drop, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

I actually think it was a good intention of yours and I would agree with the patch. The automated tools that find and complain about these issues do not know about our local house rules like "do not bother freeing immediately before exit" and will keep notifying us of leaks. One way to reduce the noise level so that we can focus on real issues they may find for us in the future is to squelch these "uninteresting" ones by "fixing" them, even if our local house rule says OK, like this patch does.

I said "a lot less interesting" and did not mean "it is bad for our code to do this".

Thanks.

Hi Junio,

On Sun, 30 Apr 2017, Junio C Hamano wrote:

In Coverity, I marked them as "Intentional", so that they can be dropped from the default view.

Unfortunately, the verdict "Intentional" does not percolate to other projects, so I will probably have to do the entire shebang in the `git` project again.

Of course, we could also decide that we want to shut up static analyzers by actually *not* leaking memory, by *not* using exit() as our garbage collector in one-shot commands, with the added benefit of making the code easier to libify/reuse.

But I fear that I distract the discussion away from a more focused attention to the actual patch series, which is actually still in flight and in actual need for review, as I actually want to get those patches integrated into git.git.

Ciao, Dscho

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wt-status.c | 1 +
 1 file changed, 1 insertion(+)

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/mailsplit.c | 2 +-
 mailinfo.c          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Good find.  I'd retitle with a prefix

	mailinfo & mailsplit: check for EOF while parsing

so that it is clear that this patch is about lower level machinery (as oppose to something like "git am").

Thanks.

Hi Junio,

On Wed, 26 Apr 2017, Junio C Hamano wrote:

True. Will be fixed in the next iteration, Dscho

Am 26.04.2017 um 22:20 schrieb Johannes Schindelin:

Why? isspace(EOF) is well-defined.

-- Hannes

Hi Hannes,

On Thu, 27 Apr 2017, Johannes Sixt wrote:

So let's look at the man page on Linux:

	These functions check whether c,  which  must  have  the  value  of  an
	unsigned char or EOF, [...]

That is the only mention of it. I find it highly unobvious whether EOF should be treated as a space or not. So let's look at the MSDN page (https://msdn.microsoft.com/en-us/library/y13z34da.aspx) whether they talk more about EOF:

	The behavior of isspace and _isspace_l is undefined if c is not
	EOF or in the range 0 through 0xFF, inclusive.

That's it. So I kind of *guess* that EOF is treated as not being a whitespace character (why does this make me think of politics now? Focus, Johannes, focus...). But the mathematician in me protests: why would we be able to decide the character class of a character that does not exist?

Technically, you are correct, of course. The specs of fgetc() specify quite clearly that either an unsigned char cast to an int is returned, or EOF on end-of-file *or error*. And a quick test verifies that isspace(EOF) returns 0.

But then, I guess I misunderstood what Coverity complained about: maybe the problem was not so much the isspace() call but that EOF is not being handled correctly. We pass it, unchecked, to ungetc().

It appears that I (or Coverity, if you will), missed another instance where we simply passed EOF unchecked to ungetc().

The next iteration will have it completely reworked: I no longer guard the isspace() behind an `!= EOF` check, but rather handle an early EOF as I think it should be handled. Extra eyes very welcome (this is the fixup! patch):

-- snip --

On Fri, Apr 28, 2017 at 12:41:02PM +0200, Johannes Schindelin wrote:

This behavior actually comes from the C standard. From C99, 7.4:

  1 The header <ctype.h> declares several functions useful for
    classifying and mapping characters. 166) In all cases the argument
    is an int, the value of which shall be representable as an unsigned
    char or shall equal the value of the macro EOF. If the argument has
    any other value, the behavior is undefined.

It doesn't explicitly say that EOF returns false for these functions, but it does seem implied. For instance:

  The isspace function tests for any character that is a standard
  white-space character or is one of a locale-specific set of characters
  for which isalnum is false. The standard white-space characters are
  the following: space (' '), form feed ('\f'), new-line ('\n'),
  carriage return ('\r'), horizontal tab ('\t'), and vertical tab
  ('\v'). In the "C" locale, isspace returns true only for the standard
  white-space characters.

So I think it is a reasonable thing to depend on. But as I said elsewhere in the thread, we don't actually use the standard isspace() anyway.

I think that is also fine according to the standard.

Do you happen to have the exact error from Coverity? I'm wondering if it is complaining about some aspect of our custom isspace() when used with EOF.

I do think handling EOF explicitly is probably a better strategy anyway, as it lets us tell when we have an empty patch.

-Peff

Hi Peff,

On Fri, 28 Apr 2017, Jeff King wrote:

Wow, that was unnecessarily hard. It is a major hassle to get to any scan other than the latest one.

But I did it. Call me tenatious.

The report says this:

233        do {
   2. negative_return_fn: Function mingw_fgetc(f) returns a negative number.
   3. var_assign: Assigning: signed variable peek = mingw_fgetc.
234                peek = fgetc(f);
   CID 1049734: Negative array index read (NEGATIVE_RETURNS)
   4.  negative_returns: Using variable peek as an index to array sane_ctype.
235        } while (isspace(peek));
236        ungetc(peek, f);

So part of the thing is that we use mingw_fgetc() instead of fgetc(). However, the return value is *still* the one from the "real" fgetc(), even if we intercept what appears to be a Ctrl+C from an interactive console.

That would appear to be the real issue, yes, and I should have double-checked the claim that POSIX isspace() handles EOF properly: we override isspace() with our own version, after all:

	#define isspace(x) sane_istest(x,GIT_SPACE)

where

	#define sane_istest(x,mask) \
		((sane_ctype[(unsigned char)(x)] & (mask)) != 0)

(rewrapped for readability)

As usual, EOF is defined as -1 in Git for Windows' context, meaning that we look at the last entry of the sane_ctype array, which returns 0 for any sane_istest(x,mask) test for x >= 0x80:

        /* Nothing in the 128.. range */

So it would appear that it happens to work, but I doubt that it was intentional.

Having said that, it is really curious why Coverity should get confused by the code and not realize that casting a negative number to (unsigned char) will make it valid as an index for the sane_ctype array.

I double-checked, and there is no override for the isspace() function in what Coverity calls a "model file" (i.e. pseudo code intended to helping Coverity realize where it can stop reporting false positives).

I agree, I came to the same conclusion independently.

Ciao, Dscho

On Fri, Apr 28, 2017 at 03:33:41PM +0200, Johannes Schindelin wrote:

Yeah, that was the same analysis I came to. Even though it works, it is a potential portability problem if a platform defines EOF in a weird way. The "right" thing in such a case is probably checking explicitly for EOF, but I'd hate to add an extra conditional to sane_istest(). It's a fairly hot code path. So I'm fine with punting on that until we see evidence of such a system.

Yeah, that is the part that confuses me, too. It seems like an easy case to get right.

Oh well. If you are tweaking it to handle EOF separately for other reasons, then the Coverity question goes away entirely.

-Peff

On Wed, Apr 26, 2017 at 10:20:16PM +0200, Johannes Schindelin wrote:

Are we guaranteed that EOF is a negative number? Also, what is the behavior of ungetc when we pass it EOF?

It looks like POSIX does what we want (pushing EOF is a noop, and the stream retains its feof() status), but I don't know if there are other implementations to worry about.

Perhaps:

  /* soak up whitespace */
  while ((peek = fgetc(f)) != EOF) {
	if (!isspace(peek)) {
		ungetc(peek, f);
		break;
	}
  }

would be more portable.

-Peff

Hi Peff,

On Thu, 27 Apr 2017, Jeff King wrote:

No, you're right.

According to the documentation, it would cast EOF to an unsigned char and push that back. Definitely incorrect.

That's not what my man page here says:

	ungetc()  pushes  c  back to stream, cast to unsigned char, where
	it is available for subsequent read operations.  Pushed-back
	characters will be returned in reverse order; only one pushback is
	guaranteed.

True. I changed it slightly differently, please see my reply to Hannes.

Thanks, Dscho

On Fri, Apr 28, 2017 at 12:44:52PM +0200, Johannes Schindelin wrote:

POSIX covers this this case explicitly:

  If the value of c equals that of the macro EOF, the operation shall
  fail and the input stream shall be left unchanged.

That comes straight from C99, which says:

  If the value of c equals that of the macro EOF, the operation fails
  and the input stream is unchanged.

I don't have a copy of C89 handy, but I didn't see any mention of the behavior in the "changes from the previous edition" section of C99.

So it's possible that there's an implementation that is unhappy with ungetc(EOF), but unless we know of one specifically, it seems pretty safe. Given that and the similar explicit rule for EOF via isspace(), I think the original code actually behaves fine.

Of course, we do not use the standard isspace() anyway. Our implementation will cast the EOF to an unsigned char. If it's "-1", that ends up as 255, which matches no classes. But if the platform has an oddball EOF like 288, that would confuse our isspace().

-Peff

Hi Peff,

On Fri, 28 Apr 2017, Jeff King wrote:

Yeah. I'd still be more comfortable with being explicit in this case, also because our documentation states explicitly that we do not necessarily live by the POSIX standard.

Ciao, Dscho

Discovered by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/cat-file.c | 1 +
 1 file changed, 1 insertion(+)

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

This is a border-line "Meh". Just like we do not free resources immediately before calling die(), we can leave this as-is as the only thing that happens after this is a return from cmd_cat_file() back to main() that exits.

The patch is not wrong per-se, so I've applied it anyway, though.

Thanks.

Hi Junio,

On Wed, 26 Apr 2017, Junio C Hamano wrote:

If you are mostly concerned about the status quo, that is true.

I am a lot more concerned with future changes, where we may easily decide that it is time to move a file-local function out of its hiding place and make it more usable.

From that perspective, it is one thing to have a blatant memory leak in a cmd_*() function, and it is an entirely different matter to have such a leak in a function that happens to be called only from cmd_*() functions: somebody familiar enough with Git's coding conventions (such as myself) will *expect* cmd_*() to have leaks left and right and pay attention when libifying that code, but be a lot less concerned about such leaks in other functions.

And of course this concerns me more than you, as I am still trying to drive forward the effort to convert more scripts into builtins.

So on my own behalf: thank you for accepting this patch.

Ciao, Dscho

Discovered via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/checkout.c | 1 +
 1 file changed, 1 insertion(+)

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Thanks for spotting this one and fixing it.  

In case anybody is wondering what the "only to leak" comment before this part of the code is about (which by the way may need to be updated, as the bulk of its reasoning still applies but at least we are no longer leaking with this patch), back when this code was written in 2008 or so it wasn't kosher to free cache_entry under certain conditions, but it has been a long time since it became OK to free any cache entries in later 2011---we should have done this a long time ago.

Hi Junio,

On Wed, 26 Apr 2017, Junio C Hamano wrote:

Thanks for the background. The next iteration will change that comment, too (simply removing "just to leak" and rewrapping to 74 columns/line.

Ciao, Dscho

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wt-status.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Wed, Apr 26, 2017 at 1:20 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

This alone doesn't fix the memleak if the code were called multiple times. As by having it static it will be init'd to NULL, we could introduce a

    static const char *gitdir; /* must live until the end of time because ... */
    if (!gitdir)
        die("BUG: called setup_bare_git_dir twice?");

I tried looking into setup_explicit_git_dir that is called with gitdir here and it looks like it duplicates the memory in most cases, so maybe we can even get away with no static variable here and instead either inline the gitdir computation in the arguments of setup_explicit_git_dir or have a variable here that holds the returns of setup_explicit_git_dir such that we can free after?

Thanks, Stefan

Hi Stefan,

On Wed, 26 Apr 2017, Stefan Beller wrote:

setup_bare_git_directory() is called by setup_git_directory(), and that latter function is not reentrant. If you call setup_git_directory() multiple times, it will wreak havoc, as e.g. the second time the prefix cannot be determined correctly because we chdir()ed up into the top-level directory.

So I am not worried about this new static, it is essentially a singleton.

Ciao, Dscho

Am 26.04.2017 um 22:20 schrieb Johannes Schindelin:

Whoa! Look what values are assigned to the variable: Either a static string or allocated memory. I suspect that this does not fix a memory leak at all, but only shuts up Coverity.

-- Hannes

PS: stopping here as I'm running out of time.

Hi Hannes,

On Thu, 27 Apr 2017, Johannes Sixt wrote:

Sorry, I meant to amend the commit message and forgot before sending the first time: setup_git_directory() (and consequently, setup_bare_git_dir()) can only be called once.

Yes, this patch is here to shut up Coverity, as it cannot know that we are talking about a singleton here.

Ciao, Dscho

Identified by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 setup.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Identified via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pack-redundant.c | 1 +
 1 file changed, 1 insertion(+)

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/mktree.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Reported by, you guessed it, Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/fast-export.c | 1 +
 1 file changed, 1 insertion(+)

Am 26.04.2017 um 22:21 schrieb Johannes Schindelin:

There is an early return in the function that is not covered by this patch. Look for "case DROP".

-- Hannes

Hi Hannes,

On Thu, 27 Apr 2017, Johannes Sixt wrote:

Thanks!

Or for "return" ;-)

I briefly looked into simply releasing the memory earlier, but the tagger variable used just before the inserted free(buf) actually points into the buffer, so I had to repeat the free(buf) for the early return.

Thank you! Dscho

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/receive-pack.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Discovered by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 line-log.c | 1 +
 1 file changed, 1 insertion(+)

Am 26.04.2017 um 22:21 schrieb Johannes Schindelin:

parent_range is of type struct line_log_data *, which needs more than a mere free(). I think it's free_line_log_data(parent_range).

-- Hannes

Hi Hannes,

On Thu, 27 Apr 2017, Johannes Sixt wrote:

Oh wow, thanks for pointing that out. Will be fixed in the next iteration.

Ciao, Dscho

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 shallow.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

We free()d the `log` buffer when dwim_log() returned 1, but not when it returned a larger value (which meant that it still allocated the buffer but we simply ignored it).

Identified by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 reflog-walk.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Am 26.04.2017 um 22:21 schrieb Johannes Schindelin:

Right after this hunk, there is another conditional that looks like it forgets to free reflogs.

-- Hannes

Hi Hannes,

On Thu, 27 Apr 2017, Johannes Sixt wrote:

Thanks! Seems I got too hung up with the line to which Coverity pointed and failed to see the bigger picture.

It seems that there are plenty of leaks, even further down. For one, the `branch` variable is not released at the very end of the function! One might think that read_complete_reflogs() takes custody of it, but no, it xstrdup()s the refname right at the beginning.

So there was a lot more memory leaking going on in that function...

Ciao, Dscho

The `guess_ref()` returns an allocated buffer of which `make_linked_ref()` does not take custody (`alloc_ref()` makes a copy), therefore we need to release the buffer afterwards.

Noticed via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 remote.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

There is really no reason why we would need to hold onto the allocated string longer than necessary.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 worktree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The buffer allocated by shorten_unambiguous_ref() needs to be released.

Discovered by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/worktree.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

When the `name_rev()` function is asked to dereference the tip name, it allocates memory. But when it turns out that another tip already described the commit better than the current one, we forgot to release the memory.

Pointed out by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/name-rev.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

On Wed, Apr 26, 2017 at 1:19 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

YAY! How do you trigger the coverity scan? (via travis or uploading a tarball manually every once in a while?) Can you talk about the setup? I'm interested in that as I run coverity for Junios pu branch, plus a couple patches on top[1].

[1] https://github.com/stefanbeller/git/commits/coverity

Thanks.

For the latter I think we could get away with maintaining patches on a dedicated coverity branch (which I do currently, just not in an extensive manner). For the first, I think we can just compile with FLEX_ARRAY defined as an insanely large number. I'll experiment with that.

Thanks, Stefan

Hi Stefan,

On Wed, 26 Apr 2017, Stefan Beller wrote:

For starters, I did everything manually. But I already have the grand plan to use VSTS build (I do not want to advertise this system too blatantly, but I have to say that I really love working with that system, it is so much more powerful than Travis or AppVeyor, Jenkins comes closer but not by far).

As you may have glimpsed in one of my mails, Git for Windows needs a very special and extensive build setup. I call it the "Git for Windows SDK", and what it is, in a nutshell, is a custom MSYS2 installation that has all the packages installed that are needed to build Git for Windows, its most important dependencies, and to bundle everything in an installer. Think of MSYS2 as something like Homebrew. But cozier, as it really lives inside its own directory tree, its /etc/ is not really visible to any other software, for example.

This makes Continuous Testing a bit of a challenge, as you cannot hammer the package repository of MSYS2 with tons of fresh full installations all the time (that package repository is run by a volunteer, and it is sometimes unreachable, too).

So after talking to a GitHub guy at the Developer Summit during GitMerge (Patrick asked a couple of questions and then said my plan was okay), I put two new repositories up that host the initialized Git for Windows SDKs (one for 32-bit, one for 64-bit).

And of course there is a job to keep them up-to-date, run once every 24h.

Almost all of the VSTS builds I maintain clone or update one or both of these repositories.

And so will also the Coverity job.

I still have to find the time to figure out one more detail: how to download and extract the Coverity tool (the .zip archive has a variable name for the top-level directory), and doing that only every once in a while, say, only when there is no previously unpacked tool, or it is already 4 weeks old.

And then I should probably try to figure out a way how to delay subsequent runs of that job so that we run it at most once every twenty-four hours.

Ciao, Dscho

Hi Johannes,

thanks for the reply.

On Thu, Apr 27, 2017 at 3:50 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

That is an interesting problem, which I ignored as the older versions of their tools still works once they release new versions. So I just manually check every once in a while if they have new versions out there.

So if you find a nice solution to that problem, let me know, please.

Thanks, Stefan

Hi Stefan,

On Fri, 28 Apr 2017, Stefan Beller wrote:

I think I have a working idea (jotting it down in the editor, untested):

	init_or_update_coverity_tool () {
		# check once per week whether there is a new version
		coverity_tool=.git/coverity-tool/
		test ! -d $coverity_tool ||
		test $(($(date +%s)-$(stat -c %Y $coverity_tool))) -gt
			$((7*24*60*60)) ||
		return

		curl --form "token=$(COVERITY.TOKEN)" \
			--form "project=git-for-windows" \
			--time-cond .git/coverity_tool.zip \
			-o .git/coverity_tool.zip.new \
			https://scan.coverity.com/download/win64 &&
		test -f .git/coverity_tool.zip.new || {
			# Try again in a week
			touch $coverity_tool
			return
		}

		mv -f .git/coverity_tool.zip.new .git/coverity_tool.zip ||
		die "Could not overwrite coverity_tool.zip"

		mkdir $coverity_tool.new &&
		(cd $coverity_tool.new &&
		 unzip ../coverity_tool.zip) ||
		die "Could not unpack coverity_tool.zip"

		rm -rf $coverity_tool &&
		mv $coverity_tool.new $coverity_tool ||
		die "Could not switch to new Coverity tool version"
	}

	init_or_update_coverity_tool
	PATH=$(echo $coverity_tool/*/bin):$PATH

I guess I will start from that snippet once I have time to work on that Coverity automation.

BTW I stumbled over an interesting tidbit today: if you define FLEX_ARRAY outside of git-compat-util.h, it will not be overridden by Git. That is, if you want to use 64kB flex arrays by default, you can call

	make CPPFLAGS=-DFLEX_ARRAY=65536

No need to patch the source code.

Ciao, Dscho

Looks like Coverity has TravisCI integration and I assume you wouldn't need to worry about downloading the tool in that setup: https://scan.coverity.com/travis_ci

I think we should be able to enable it without trouble for the 'master' branch?

Cheers, Lars

Hi Lars,

On Mon, 1 May 2017, Lars Schneider wrote:

Except for that tiny little fact that Travis CI does not support Windows backends (nor would they even respond to my offer to work with them).

Ciao, Dscho

Hi Stefan,

On Fri, 28 Apr 2017, Johannes Schindelin wrote:

And now I edited it and tested it. The code is now part of the script I use for pretty much all administrative (i.e. recurring and boring) tasks in the Git for Windows project:

	https://github.com/git-for-windows/build-extra/commit/05b5342128

May it be useful to you, too, at least as a starting point, Dscho

Hi Stefan,

On Fri, 5 May 2017, Johannes Schindelin wrote:

Oh, I completely forgot to mention that I tried to set the FLEX_ARRAY constant to something quite large (I used 64k), but apparently that does not work as expected, Coverity still insists on complaining about strbufs.

On a second thought, it is actually quite obvious why it does not fix those reports: STRBUF_INIT has nothing to do with FLEX_ARRAY. D'oh.

My next attempt to work around these bogus claims was to modify the "model file" by adding a line saying "char strbuf_slopbuf[64];", but that was sadly not picked up by Coverity either.

My current thinking is that I will simply patch strbuf.c via `sed 's/^\(char struct_slopbuf\[\)1\[/&64[/'`.

Ciao, Dscho

On Wed, May 10, 2017 at 12:48 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

D'oh. I must have been living in an alternate universe for quite some time as I seemed to have confused different things here.

Checkout this commit, https://github.com/stefanbeller/git/commit/977f81d6dec4461a1a12da2df6c5c919b41129b4 that is cherry-picked for the coverity build. That fixes it.

That is about the only patch I apply before sending it off to coverity. I am still contemplating a nice solution for FLEX_ARRAYs in other cases.

Thanks, Stefan

Hi Stefan,

On Wed, 10 May 2017, Stefan Beller wrote:

I saw that patch, and since Junio did not pick it up, I figured that it won't make it into git.git's source code. That's why I went with the `sed` approach, as the #ifndef __COVERITY__ guard is not even necessary if I have to patch the code before running the Coverity tool anyway.

For FLEX_ARRAYs, I use this:

	cov-build --dir cov-int \
                make -j15 DEVELOPER=1 CPPFLAGS=-DFLEX_ARRAY=65536

I have not had time to go over more than half a dozen CIDs, but I *think* it helps.

Ciao, Dscho

Am 26.04.2017 um 22:19 schrieb Johannes Schindelin:

Thanks for the fish. I'd looked at the series and had a few comments.

Hunting memory leaks is the way to insanity. Never again am I going to help with this. I prefer to rewrite this codebase to C++ and have leak-free code by design.

Thanks, -- Hannes

Hi Hannes,

On Thu, 27 Apr 2017, Johannes Sixt wrote:

I hear you. Loud and clear.

Awww? And here I thought I had your attention... *sniffle*

;-)

I had the pleasure of working with some software developers in 2004 who were experts at introducing memory leaks into C++ code.

The same bunch of people later produced Java code that sorted a string list in cubic time (carefully avoiding java.util.Collections.sort(), of course).

For every fool-proof system invented, somebody invents a better fool.

Ciao, Dscho

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;
  		}

In the (admittedly, concocted) case that PATH consists only of colons, we would leak the duplicated string.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

When stdout is not connected to a Win32 console, we incorrectly used an uninitialized value for the "plain" character attributes.

Detected by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 2 ++
 1 file changed, 2 insertions(+)

When we could not convert the UTF-8 sequence into Unicode for writing to the Console, we should not try to write an insanely-long sequence of invalid wide characters (mistaking the negative return value for an unsigned length).

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 5 +++++
 1 file changed, 5 insertions(+)

It would appear that we allocate (and forget to release) memory if the patch ID is not even defined.

Reported by the Coverity tool.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 patch-ids.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

In case of errors, we really want the file descriptor to be closed.

Discovered by a Coverity scan.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 config.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

This change plugs a couple of memory leaks and makes sure that the file descriptor is closed in run_dir_diff().

Spotted by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/difftool.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

When we fail to read, or parse, the file, we still want to close the file descriptor and release the strbuf.

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/am.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wt-status.c | 1 +
 1 file changed, 1 insertion(+)

While POSIX states that it is okay to pass EOF to isspace() (and it seems to be implied that EOF should *not* be treated as whitespace), and also to pass EOF to ungetc() (which seems to be intended to fail without buffering the character), it is much better to handle these cases explicitly. Not only does it reduce head-scratching (and helps static analysis avoid reporting false positives), it also lets us handle files containing nothing but whitespace by erroring out.

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/mailsplit.c |  3 ++-
 mailinfo.c          | 15 +++++++++++----
 2 files changed, 13 insertions(+), 5 deletions(-)

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

I agree more with the first sentence in the proposed log message than what this code actually does. I.e. breaking upon seeing an EOF explicitly would be nice, just like the change to mailinfo.c in this patch we see below.

The handling of EOF is improved, but is it correct to move the allocation of p/s_hdr_data down?

Among the two callers, builtin/am.c just dies when it sees mailinfo() returns an error, but builtin/mailinfo.c tries to be nicer and calls clear_mailinfo(). Wouldn't this make that codepath dereference a NULL pointer?

I think the moral of the story is that people tend to get sloppier when doing "while we are at it" than the main task, and a reviewer needs to be more careful while reviewing the "while we are at it" part of the change than the primary thing a patch wants to do ;-)

Hi Junio,

On Mon, 1 May 2017, Junio C Hamano wrote:

I changed it to error out with a (translated) "empty mbox: '%s'" as the other hunks.

Sorry, I *assumed* that the function was passed a zero-initialized mailinfo struct. And as you pointed out, that assumption was wrong.

My thinking was that I did not want to introduce another leakage by my patch, but as your careful analysis determined: the only caller that does not die() afterwards releases the data (and would not even be able to handle p_hdr_data == NULL...).

I reverted that move.

Ciao, Dscho

Discovered by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/cat-file.c | 1 +
 1 file changed, 1 insertion(+)

This change addresses part of the NEEDSWORK comment above the code, therefore the comment needs to be adjusted, too.

Discovered via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/checkout.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wt-status.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Coverity reported a memory leak in this function. However, it can only be called once, as setup_git_directory() changes global state and hence is not reentrant.

Mark the variable as static to indicate that this is a singleton.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Coverity reported a memory leak in this function. However, it can only be called once, as setup_git_directory() changes global state and hence is not reentrant.

Mark the variable as static to indicate that this is a singleton.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 setup.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Does something different from what is explained above.  Rebase gotcha?

Hi Junio,

On Mon, 1 May 2017, Junio C Hamano wrote:

That, or just a brain fart.

Thanks, will send an updated iteration.

Ciao, Dscho

Identified via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pack-redundant.c | 1 +
 1 file changed, 1 insertion(+)

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/mktree.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Reported by, you guessed it, Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/fast-export.c | 2 ++
 1 file changed, 2 insertions(+)

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/receive-pack.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Discovered by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 line-log.c | 1 +
 1 file changed, 1 insertion(+)

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 shallow.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

The `guess_ref()` returns an allocated buffer of which `make_linked_ref()` does not take custody (`alloc_ref()` makes a copy), therefore we need to release the buffer afterwards.

Noticed via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 remote.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

When the `name_rev()` function is asked to dereference the tip name, it allocates memory. But when it turns out that another tip already described the commit better than the current one, we forgot to release the memory.

Pointed out by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/name-rev.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Very well explained.

Junio C Hamano <gitster@pobox.com> writes:

I'll rename 'p' to 'to_free' while queuing, though. Without a descriptive name, it was confusing to view while resolving conflicts with another in-flight topic.

Hi Junio,

On Mon, 1 May 2017, Junio C Hamano wrote:

Good point. I also used `p` in builtin/mktree.c and setup.c. While you seem to have renamed it in builtin/mktree.c, I think setup.c also needs the same fixup.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Yes, no question about it. I just left it as-is as I saw it is likely to be rerolled anyway due to other issues ;-)

The buffer allocated by shorten_unambiguous_ref() needs to be released.

Discovered by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/worktree.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Yes.  Looks good.

There is really no reason why we would need to hold onto the allocated string longer than necessary.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 worktree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Yup. The longer we make the duration between the allocation and the standard release, the more likely future code would add early returns that forget to release the memory.

Looks correct; will queue.

We free()d the `log` buffer when dwim_log() returned 1, but not when it returned a larger value (which meant that it still allocated the buffer but we simply ignored it).

While in the vicinity, make sure that the `reflogs` structure as well as the `branch` variable are released properly, too.

Identified by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 reflog-walk.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

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;
  	}
  

In the (admittedly, concocted) case that PATH consists only of colons, we would leak the duplicated string.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Am 02.05.2017 um 18:00 schrieb Johannes Schindelin:

Nit: It's about semicolons, right?  At least that's what get_path_split
is searching for.  Or is there some kind of translation going on?

René

Hi René,

On Wed, 3 May 2017, René Scharfe wrote:

True, it is semicolons.

I'll reword the commit message to actually use `path delimiters`, to make it more understandable to the Unix folk (who would most likely be puzzled that PATH is separated by semicolons).

Thanks, Dscho

When stdout is not connected to a Win32 console, we incorrectly used an uninitialized value for the "plain" character attributes.

Detected by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 2 ++
 1 file changed, 2 insertions(+)

Am 02.05.2017 um 18:01 schrieb Johannes Schindelin:

So is_console is called with fd being 1 (stdout), 2 (stderr) and 0 (stdin), in that order. If the first two calls abort early for some reason we may end up here. The added code is for white text on black background. An alias for that combination, FOREGROUND_ALL, is defined a few lines down; for a minimal fix it's not worth moving it up. attr and plain_attr are both initialized to sbi.wAttributes.

That as a bit more complicated than it looked initially. The order of calls is important, "stdout" in the commit message includes stderr as well and it doesn't just affect plain_attr.

Would a value of 0 (black text on black background) suffice if only stdin is connected to a console? Colors don't matter if there is nothing to see, right?

Anyway, the change makes sense as is, but it took me a while to get it.

René

Hi René,

On Wed, 3 May 2017, René Scharfe wrote:

Correct. I guess this is the important part missing from the commit message.

Oh, I also saw that I talked about stdout in the commit message, but !fd tests for stdin!

Yep. "some reason" being: there is no console attached to stdout nor stderr.

Exactly.

Right, it also affects the "current" attributes.

I think that would make it both easier to understand the patch and to catch regressions in case anybody feels the order of the is_console() calls should be changed...

This is my current squash! commit (the original commit message will be replaced by the commit message body of this commit):

-- snipsnap --
Subject: [PATCH] squash! winansi: avoid use of uninitialized value

winansi: avoid use of uninitialized value

To initialize the foreground color attributes of "plain text", our ANSI emulation tries to infer them from the currently attached console while running the is_console() function. This function first tries to detect any console attached to stdout, then it is called with stderr.

If neither stdout nor stderr has any console attached, it does not actually matter what we use for "plain text" attributes, as we never need to output any text to any console in that case.

However, after working on stdout and stderr, is_console() is called with stdin, and it still tries to initialize the "plain text" attributes if they had not been initialized earlier. In this case, we cannot detect any attributes, and we used an uninitialized value for them.

Naturally, Coverity complained about this use case because it could not reason about the code deeply enough to figure out that we do not even use those attributes in that case.

Let's just initialize the value to 0 in that case, both to avoid future Coverity reports, and to help catch future regressions in case anybody changes the order of the is_console() calls (which would make the text black on black).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

When we could not convert the UTF-8 sequence into Unicode for writing to the Console, we should not try to write an insanely-long sequence of invalid wide characters (mistaking the negative return value for an unsigned length).

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 5 +++++
 1 file changed, 5 insertions(+)

It would appear that we allocate (and forget to release) memory if the patch ID is not even defined.

Reported by the Coverity tool.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 patch-ids.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

When we fail to read, or parse, the file, we still want to close the file descriptor and release the strbuf.

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/am.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

This change plugs a couple of memory leaks and makes sure that the file descriptor is closed in run_dir_diff().

Spotted by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/difftool.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

In case of errors, we really want the file descriptor to be closed.

Discovered by a Coverity scan.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 config.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wt-status.c | 1 +
 1 file changed, 1 insertion(+)

While POSIX states that it is okay to pass EOF to isspace() (and it seems to be implied that EOF should *not* be treated as whitespace), and also to pass EOF to ungetc() (which seems to be intended to fail without buffering the character), it is much better to handle these cases explicitly. Not only does it reduce head-scratching (and helps static analysis avoid reporting false positives), it also lets us handle files containing nothing but whitespace by erroring out.

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/mailsplit.c | 10 ++++++++++
 mailinfo.c          |  9 ++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

Discovered by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/cat-file.c | 1 +
 1 file changed, 1 insertion(+)

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wt-status.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Am 02.05.2017 um 18:02 schrieb Johannes Schindelin:

The return line could be replaced by "; else" to achieve the same result, without duplicating the free calls.

René

Hi René,

On Wed, 3 May 2017, René Scharfe wrote:

True. It is much harder to explain it that way, though: the context looks like this:

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"))
+           !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;
        }

        if (!s->amend && !s->nowarn && !s->workdir_dirty)
                split_in_progress = 0;

        free(head);
        free(orig_head);
        free(rebase_amend);
        free(rebase_orig_head);
        return split_in_progress;
 }

So as you see, if we make the change you suggested, the next if() is hit which possibly sets split_in_progress = 0.

The thing is: this variable has been initialized to 0 in the beginning! So this conditional assignment would be a noop, unless any of the code paths before this conditional modified split_in_progress.

After you fully wrapped your head around this code, I am sure you agree that the code is unnecessarily confusing to begin with: why do we go out of our way to allocate and read all those strings, compare them to figure out whether there is a split in progress, just to look at bits in the wt_status struct (that we had available from the beginning) to potentially undo all of that.

What's worse, I cannot even find a logical explanation why the code is so confusing, as it has been added as it is today in commit 2d1ccebae41 (status: better advices when splitting a commit (during rebase -i), 2012-06-10).

So I think this function really wants to be fixed more fully (although I feel bad for inserting such a non-trivial fix into a patch series otherwise populated by trivial memory/resource leak plugs):

-- snipsnap --
Subject: [PATCH] squash! split_commit_in_progress(): fix memory leak

split_commit_in_progress(): simplify & fix memory leak

This function did a whole lot of unnecessary work, such as reading in four files just to figure out that, oh, hey, we do not need to look at them after all because the HEAD is not detached.

Simplify the entire function to return early when possible, to read in the files only when necessary, and to release the allocated memory always (there was a leak, reported via Coverity, where we failed to release the allocated strings if the HEAD is not detached).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wt-status.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

Am 04.05.2017 um 12:59 schrieb Johannes Schindelin:

Right. It could have been used as a quick two-line fix.

The only function with a higher cyclomatic complexity in this file is wt_longstatus_print(), which spans more than 100 lines. Amazing.

Repeatedly I get the impression that defects or shortcomings tend to cluster. It pays to take a good look at the code surrounding a find of an automatic checker for other possible improvements.

It could be done in two steps.  Doing it in one is probably OK as well.

Further improvement idea (for a later series): Use rebase_path_amend() and rebase_path_orig_head() somehow, to build each path only once.

Accessing the files HEAD and ORIG_HEAD directly seems odd.

The second part of the function should probably be moved to sequencer.c.

You could set split_in_progress to zero here. Would save a comment without losing readability.

Isn't the patch big enough already without rearranging the else blocks and adding that blank line? :)

Hi René,

On Sat, 6 May 2017, René Scharfe wrote:

Sure. On all four accounts.

But that would confuse e.g. myself, 6 months down the road: why assign that variable when it already has been assigned? (And we have to assign it because there is still a code path entering none of these if/else if's arms).

The else blocks are not really rearranged; apart from the early return, the rest of the logic has been painstakingly kept in the same order.

Ciao, Dscho

This change addresses part of the NEEDSWORK comment above the code, therefore the comment needs to be adjusted, too.

Discovered via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/checkout.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/mktree.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Coverity reported a memory leak in this function. However, it can only be called once, as setup_git_directory() changes global state and hence is not reentrant.

Mark the variable as static to indicate that this is a singleton.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Identified via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pack-redundant.c | 1 +
 1 file changed, 1 insertion(+)

Reported by, you guessed it, Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/fast-export.c | 2 ++
 1 file changed, 2 insertions(+)

The setup_explicit_git_dir() function does not take custody of the string passed as first parameter; we have to release it if we turned the value of git_dir into an absolute path.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 setup.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

On Tue, May 2, 2017 at 9:02 AM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

As the original motivation was to shut up Coverity, this may not accomplish that goal, as in the path of taking the die_errno, we do not free `to_free`. But that is ok as the actual goal is to hav no memleaks in the good case. A memleak just before a die is no big deal.

And we free our pathdup from above, makes sense.

Thanks, Stefan

On Tue, May 02, 2017 at 10:20:29AM -0700, Stefan Beller wrote:

I think Coverity understands our NORETURN attributes, so this should be fine (and if it doesn't, then we should fix that in the model file; but from the general results I've seen, it does).

-Peff

Hi Peff,

On Tue, 2 May 2017, Jeff King wrote:

That matches my impression.

Note that Coverity cannot figure out that all of our cmd_*() functions are called in code paths that exit() with their return value. Otherwise we would not see reports about those tons of intentional "let's abuse exit() as our garbage collector" instances.

Ciao, Dscho

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/receive-pack.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

We free()d the `log` buffer when dwim_log() returned 1, but not when it returned a larger value (which meant that it still allocated the buffer but we simply ignored it).

While in the vicinity, make sure that the `reflogs` structure as well as the `branch` variable are released properly, too.

Identified by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 reflog-walk.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

The `guess_ref()` returns an allocated buffer of which `make_linked_ref()` does not take custody (`alloc_ref()` makes a copy), therefore we need to release the buffer afterwards.

Noticed via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 remote.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 shallow.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Discovered by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 line-log.c | 1 +
 1 file changed, 1 insertion(+)

The buffer allocated by shorten_unambiguous_ref() needs to be released.

Discovered by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/worktree.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

There is really no reason why we would need to hold onto the allocated string longer than necessary.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 worktree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

When the `name_rev()` function is asked to dereference the tip name, it allocates memory. But when it turns out that another tip already described the commit better than the current one, we forgot to release the memory.

Pointed out by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/name-rev.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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;
  }
  

It would appear that we allocate (and forget to release) memory if the patch ID is not even defined.

Reported by the Coverity tool.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 patch-ids.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

This change plugs a couple of memory leaks and makes sure that the file descriptor is closed in run_dir_diff().

Spotted by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/difftool.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

When we fail to read, or parse, the file, we still want to close the file descriptor and release the strbuf.

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/am.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

In case of errors, we really want the file descriptor to be closed.

Discovered by a Coverity scan.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 config.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

While POSIX states that it is okay to pass EOF to isspace() (and it seems to be implied that EOF should *not* be treated as whitespace), and also to pass EOF to ungetc() (which seems to be intended to fail without buffering the character), it is much better to handle these cases explicitly. Not only does it reduce head-scratching (and helps static analysis avoid reporting false positives), it also lets us handle files containing nothing but whitespace by erroring out.

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/mailsplit.c | 10 ++++++++++
 mailinfo.c          |  9 ++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wt-status.c | 1 +
 1 file changed, 1 insertion(+)

To initialize the foreground color attributes of "plain text", our ANSI emulation tries to infer them from the currently attached console while running the is_console() function. This function first tries to detect any console attached to stdout, then it is called with stderr.

If neither stdout nor stderr has any console attached, it does not actually matter what we use for "plain text" attributes, as we never need to output any text to any console in that case.

However, after working on stdout and stderr, is_console() is called with stdin, and it still tries to initialize the "plain text" attributes if they had not been initialized earlier. In this case, we cannot detect any attributes, and we used an uninitialized value for them.

Naturally, Coverity complained about this use case because it could not reason about the code deeply enough to figure out that we do not even use those attributes in that case.

Let's just initialize the value to 0 in that case, both to avoid future Coverity reports, and to help catch future regressions in case anybody changes the order of the is_console() calls (which would make the text black on black).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 7 +++++++
 1 file changed, 7 insertions(+)

In the (admittedly, concocted) case that PATH consists only of path delimiters, we would leak the duplicated string.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

When we could not convert the UTF-8 sequence into Unicode for writing to the Console, we should not try to write an insanely-long sequence of invalid wide characters (mistaking the negative return value for an unsigned length).

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 5 +++++
 1 file changed, 5 insertions(+)

Discovered by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/cat-file.c | 1 +
 1 file changed, 1 insertion(+)

This change addresses part of the NEEDSWORK comment above the code, therefore the comment needs to be adjusted, too.

Discovered via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/checkout.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Am 04.05.2017 um 15:56 schrieb Johannes Schindelin:

Random observation: Using pretend_sha1_file here would at least avoid writing the blob.

I wonder if that's safe. Why document a leak when it could have been plugged this easily instead? A leak is better than a use after free, so let's be extra careful here. Would it leave the index inconsistent? Or perhaps freeing it has become safe in the meantime?

@Junio: Do you remember the reason for the leaks in 0cf8581e330 (checkout -m: recreate merge when checking out of unmerged index).

And result_buf is still leaked here, right?

FWIW, after fixing a different issue (patch just sent separately) t*checkout*.sh complete successfully for me with --valgrind, with and without freeing both ce and result_buf.ptr. So it *seems* safe..

René

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

Yup, you should have told that to me back in Aug 2008 ;-) when I did 0cf8581e ("checkout -m: recreate merge when checking out of unmerged index", 2008-08-30); pretend_sha1_file() was available since early 2007, and there is no excuse that this codepath did not use it.

Yes.

In the very old days it was not allowed to free(3) contents of active_cache[] and this was an old brain fart that came out of inertia. We are manufacturing a brand new ce, only to feed it to checkout_entry() without even registering it to the active_cache[] array, and the ancient rule doesn't even apply to such a case.

So I think it was safe to free(3) even back then.

Good spotting. It typically would make a larger leak than a single ce, I would suppose ;-)

Hi Junio & René,

On Mon, 8 May 2017, Junio C Hamano wrote:

I hope y'all agree that this is outside the scope of my patch series...

So this patch is good, then?

I saw you added this as a fixup! commit. If you don't mind, and if no other changes are requested, would you mind rebase'ing this yourself?

Thanks, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Unless I from this year is failing to spot a breakage that will be caused in the codepath that I from year 2008 and René spotted, I think freeing ce after we are done updating the working tree file with it is safe. I'd need to find time to make sure, though.

I think it would be better to leave the fix as a separate patch. It wasn't spotted by Coverity in the first place ;-)

Reported via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/receive-pack.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Discovered by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 line-log.c | 1 +
 1 file changed, 1 insertion(+)

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 shallow.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

This function did a whole lot of unnecessary work, such as reading in four files just to figure out that, oh, hey, we do not need to look at them after all because the HEAD is not detached.

Simplify the entire function to return early when possible, to read in the files only when necessary, and to release the allocated memory always (there was a leak, reported via Coverity, where we failed to release the allocated strings if the HEAD is not detached).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wt-status.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

The setup_explicit_git_dir() function does not take custody of the string passed as first parameter; we have to release it if we turned the value of git_dir into an absolute path.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 setup.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Reported by, you guessed it, Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/fast-export.c | 2 ++
 1 file changed, 2 insertions(+)

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/mktree.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Coverity reported a memory leak in this function. However, it can only be called once, as setup_git_directory() changes global state and hence is not reentrant.

Mark the variable as static to indicate that this is a singleton.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

We free()d the `log` buffer when dwim_log() returned 1, but not when it returned a larger value (which meant that it still allocated the buffer but we simply ignored it).

While in the vicinity, make sure that the `reflogs` structure as well as the `branch` variable are released properly, too.

Identified by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 reflog-walk.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

The `guess_ref()` returns an allocated buffer of which `make_linked_ref()` does not take custody (`alloc_ref()` makes a copy), therefore we need to release the buffer afterwards.

Noticed via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 remote.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

The buffer allocated by shorten_unambiguous_ref() needs to be released.

Discovered by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/worktree.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

When the `name_rev()` function is asked to dereference the tip name, it allocates memory. But when it turns out that another tip already described the commit better than the current one, we forgot to release the memory.

Pointed out by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/name-rev.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

There is really no reason why we would need to hold onto the allocated string longer than necessary.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 worktree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Identified via Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pack-redundant.c | 1 +
 1 file changed, 1 insertion(+)