git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] pack-revindex.c: don't close unopened file descriptors
@ 2021-02-26 16:31 Taylor Blau
  2021-02-26 17:55 ` Jeff King
  2021-02-26 22:41 ` Junio C Hamano
  0 siblings, 2 replies; 4+ messages in thread
From: Taylor Blau @ 2021-02-26 16:31 UTC (permalink / raw)
  To: git; +Cc: Johannes.Schindelin, gitster

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] pack-revindex.c: don't close unopened file descriptors
  2021-02-26 16:31 [PATCH] pack-revindex.c: don't close unopened file descriptors Taylor Blau
@ 2021-02-26 17:55 ` Jeff King
  2021-03-01 22:18   ` Johannes Schindelin
  2021-02-26 22:41 ` Junio C Hamano
  1 sibling, 1 reply; 4+ messages in thread
From: Jeff King @ 2021-02-26 17:55 UTC (permalink / raw)
  To: Taylor Blau; +Cc: git, Johannes.Schindelin, gitster

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] pack-revindex.c: don't close unopened file descriptors
  2021-02-26 16:31 [PATCH] pack-revindex.c: don't close unopened file descriptors Taylor Blau
  2021-02-26 17:55 ` Jeff King
@ 2021-02-26 22:41 ` Junio C Hamano
  1 sibling, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2021-02-26 22:41 UTC (permalink / raw)
  To: Taylor Blau; +Cc: git, Johannes.Schindelin

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] pack-revindex.c: don't close unopened file descriptors
  2021-02-26 17:55 ` Jeff King
@ 2021-03-01 22:18   ` Johannes Schindelin
  0 siblings, 0 replies; 4+ messages in thread
From: Johannes Schindelin @ 2021-03-01 22:18 UTC (permalink / raw)
  To: Jeff King; +Cc: Taylor Blau, git, gitster

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-03-01 22:26 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-26 16:31 [PATCH] pack-revindex.c: don't close unopened file descriptors Taylor Blau
2021-02-26 17:55 ` Jeff King
2021-03-01 22:18   ` Johannes Schindelin
2021-02-26 22:41 ` Junio C Hamano

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).