[PATCH] pack-revindex.c: don't close unopened file descriptors

[PATCH] pack-revindex.c: don't close unopened file descriptors

[Taylor Blau]

When opening a reverse index, load_revindex_from_disk() jumps to the
'cleanup' label in case something goes wrong: the reverse index had the
wrong size, an unrecognized version, or similar.

It also jumps to this label when the reverse index couldn't be opened in
the first place, which will cause an error with the unguarded close()
call in the label.

Guard this call with "if (fd >= 0)" to make sure that we have a valid
file descriptor to close before attempting to close it.

Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
Dscho mentioned this to me privately when reviewing Coverity results for
-rc0. This one is legitimate, and the fix is easy enough, too.

 pack-revindex.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/pack-revindex.c b/pack-revindex.c
index 83fe4de773..4262530449 100644
--- a/pack-revindex.c
+++ b/pack-revindex.c
@@ -253,7 +253,8 @@ static int load_revindex_from_disk(char *revindex_name,
 		*data_p = (const uint32_t *)data;
 	}

-	close(fd);
+	if (fd >= 0)
+		close(fd);
 	return ret;
 }

--
2.30.0.667.g81c0cbc6fd

Re: [PATCH] pack-revindex.c: don't close unopened file descriptors

[Jeff King]

On Fri, Feb 26, 2021 at 11:31:02AM -0500, Taylor Blau wrote:

> When opening a reverse index, load_revindex_from_disk() jumps to the
> 'cleanup' label in case something goes wrong: the reverse index had the
> wrong size, an unrecognized version, or similar.
> 
> It also jumps to this label when the reverse index couldn't be opened in
> the first place, which will cause an error with the unguarded close()
> call in the label.
> 
> Guard this call with "if (fd >= 0)" to make sure that we have a valid
> file descriptor to close before attempting to close it.

Makes sense. Running close(-1) wasn't really hurting much in practice,
but it's cleaner not to do so.

This would go on top of tb/pack-revindex-on-disk.

> Dscho mentioned this to me privately when reviewing Coverity results for
> -rc0. This one is legitimate, and the fix is easy enough, too.

I'm excited that we might get Coverity results again. There were a lot
of false positives, but I found its signal-to-noise ratio higher than
almost every other static analysis tool I've looked at.

-Peff

Re: [PATCH] pack-revindex.c: don't close unopened file descriptors

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> When opening a reverse index, load_revindex_from_disk() jumps to the
> 'cleanup' label in case something goes wrong: the reverse index had the
> wrong size, an unrecognized version, or similar.
>
> It also jumps to this label when the reverse index couldn't be opened in
> the first place, which will cause an error with the unguarded close()
> call in the label.
>
> Guard this call with "if (fd >= 0)" to make sure that we have a valid
> file descriptor to close before attempting to close it.
>
> Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> Dscho mentioned this to me privately when reviewing Coverity results for
> -rc0. This one is legitimate, and the fix is easy enough, too.

Thanks, both.

>
>  pack-revindex.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/pack-revindex.c b/pack-revindex.c
> index 83fe4de773..4262530449 100644
> --- a/pack-revindex.c
> +++ b/pack-revindex.c
> @@ -253,7 +253,8 @@ static int load_revindex_from_disk(char *revindex_name,
>  		*data_p = (const uint32_t *)data;
>  	}
>
> -	close(fd);
> +	if (fd >= 0)
> +		close(fd);
>  	return ret;
>  }
>
> --
> 2.30.0.667.g81c0cbc6fd

Re: [PATCH] pack-revindex.c: don't close unopened file descriptors

[Johannes Schindelin]

Hi Peff,

On Fri, 26 Feb 2021, Jeff King wrote:

> On Fri, Feb 26, 2021 at 11:31:02AM -0500, Taylor Blau wrote:
>
> > Dscho mentioned this to me privately when reviewing Coverity results for
> > -rc0. This one is legitimate, and the fix is easy enough, too.
>
> I'm excited that we might get Coverity results again. There were a lot
> of false positives, but I found its signal-to-noise ratio higher than
> almost every other static analysis tool I've looked at.

Indeed, the signal:noise ratio is pretty bad, mainly because of all the
false positives (Coverity _really_ hates what we do with `strbuf_slopbuf`,
it simply doesn't understand that we allocate `buf` only when needing to
write characters into that buffer) and the "intentional" issues (we leak
memory left and right in `builtin/`).

It does not help at all that Coverity has a bug for a pretty long while
now where it simply throws up its digital hands in the air when it sees a
GCC v10.x. I did find a work-around for Git for Windows' automated
Coverity run, a work-around that is somewhat ugly yet necessary, sadly:
https://github.com/git-for-windows/build-extra/commit/23eea104d53

Ciao,
Dscho