diff mbox series

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

Message ID 34c017296a7c5fe4a2ea70f8a0b2d8586b34a4dc.1614357030.git.me@ttaylorr.com (mailing list archive)
State New
Headers show
Series pack-revindex.c: don't close unopened file descriptors | expand

Commit Message

Taylor Blau Feb. 26, 2021, 4:31 p.m. UTC
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(-)

--
2.30.0.667.g81c0cbc6fd

Comments

Jeff King Feb. 26, 2021, 5:55 p.m. UTC | #1
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
Junio C Hamano Feb. 26, 2021, 10:41 p.m. UTC | #2
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
Johannes Schindelin March 1, 2021, 10:18 p.m. UTC | #3
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
diff mbox series

Patch

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