| Message ID | 34c017296a7c5fe4a2ea70f8a0b2d8586b34a4dc.1614357030.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pack-revindex.c: don't close unopened file descriptors | expand |
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
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
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 --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; }
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