{"thread":{"id":"55225","subject":"[PATCH] pack-revindex.c: don't close unopened file descriptors","startedAt":"2021-02-26T16:32:10Z","lastAt":"2021-03-01T22:25:08Z","messageCount":4,"participants":["Taylor Blau","Jeff King","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"417901","messageId":"34c017296a7c5fe4a2ea70f8a0b2d8586b34a4dc.1614357030.git.me@ttaylorr.com","threadId":"55225","inReplyTo":null,"subject":"[PATCH] pack-revindex.c: don't close unopened file descriptors","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-26T16:31:02Z","receivedAt":"2021-02-26T16:32:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When opening a reverse index, load_revindex_from_disk() jumps to the\n'cleanup' label in case something goes wrong: the reverse index had the\nwrong size, an unrecognized version, or similar.\n\nIt also jumps to this label when the reverse index couldn't be opened in\nthe first place, which will cause an error with the unguarded close()\ncall in the label.\n\nGuard this call with \"if (fd >= 0)\" to make sure that we have a valid\nfile descriptor to close before attempting to close it.\n\nReported-by: Johannes Schindelin \nSigned-off-by: Taylor Blau \n---\nDscho mentioned this to me privately when reviewing Coverity results for\n-rc0. This one is legitimate, and the fix is easy enough, too.\n\n pack-revindex.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 83fe4de773..4262530449 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -253,7 +253,8 @@ static int load_revindex_from_disk(char *revindex_name,\n \t\t*data_p = (const uint32_t *)data;\n \t}\n\n-\tclose(fd);\n+\tif (fd >= 0)\n+\t\tclose(fd);\n \treturn ret;\n }\n\n--\n2.30.0.667.g81c0cbc6fd\n"},{"id":"417904","messageId":"YDk1/y1CmsPYC88C@coredump.intra.peff.net","threadId":"55225","inReplyTo":"34c017296a7c5fe4a2ea70f8a0b2d8586b34a4dc.1614357030.git.me@ttaylorr.com","subject":"Re: [PATCH] pack-revindex.c: don't close unopened file descriptors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-26T17:55:11Z","receivedAt":"2021-02-26T17:56:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 26, 2021 at 11:31:02AM -0500, Taylor Blau wrote:\n\n> When opening a reverse index, load_revindex_from_disk() jumps to the\n> 'cleanup' label in case something goes wrong: the reverse index had the\n> wrong size, an unrecognized version, or similar.\n> \n> It also jumps to this label when the reverse index couldn't be opened in\n> the first place, which will cause an error with the unguarded close()\n> call in the label.\n> \n> Guard this call with \"if (fd >= 0)\" to make sure that we have a valid\n> file descriptor to close before attempting to close it.\n\nMakes sense. Running close(-1) wasn't really hurting much in practice,\nbut it's cleaner not to do so.\n\nThis would go on top of tb/pack-revindex-on-disk.\n\n> Dscho mentioned this to me privately when reviewing Coverity results for\n> -rc0. This one is legitimate, and the fix is easy enough, too.\n\nI'm excited that we might get Coverity results again. There were a lot\nof false positives, but I found its signal-to-noise ratio higher than\nalmost every other static analysis tool I've looked at.\n\n-Peff\n"},{"id":"417919","messageId":"xmqqtupyqwnz.fsf@gitster.g","threadId":"55225","inReplyTo":"34c017296a7c5fe4a2ea70f8a0b2d8586b34a4dc.1614357030.git.me@ttaylorr.com","subject":"Re: [PATCH] pack-revindex.c: don't close unopened file descriptors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-26T22:41:36Z","receivedAt":"2021-02-26T22:42:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> When opening a reverse index, load_revindex_from_disk() jumps to the\n> 'cleanup' label in case something goes wrong: the reverse index had the\n> wrong size, an unrecognized version, or similar.\n>\n> It also jumps to this label when the reverse index couldn't be opened in\n> the first place, which will cause an error with the unguarded close()\n> call in the label.\n>\n> Guard this call with \"if (fd >= 0)\" to make sure that we have a valid\n> file descriptor to close before attempting to close it.\n>\n> Reported-by: Johannes Schindelin \n> Signed-off-by: Taylor Blau \n> ---\n> Dscho mentioned this to me privately when reviewing Coverity results for\n> -rc0. This one is legitimate, and the fix is easy enough, too.\n\nThanks, both.\n\n>\n> pack-revindex.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/pack-revindex.c b/pack-revindex.c\n> index 83fe4de773..4262530449 100644\n> --- a/pack-revindex.c\n> +++ b/pack-revindex.c\n> @@ -253,7 +253,8 @@ static int load_revindex_from_disk(char *revindex_name,\n> \t\t*data_p = (const uint32_t *)data;\n> \t}\n>\n> -\tclose(fd);\n> +\tif (fd >= 0)\n> +\t\tclose(fd);\n> \treturn ret;\n> }\n>\n> --\n> 2.30.0.667.g81c0cbc6fd\n"},{"id":"418083","messageId":"nycvar.QRO.7.76.6.2103012312230.57@tvgsbejvaqbjf.bet","threadId":"55225","inReplyTo":"YDk1/y1CmsPYC88C@coredump.intra.peff.net","subject":"Re: [PATCH] pack-revindex.c: don't close unopened file descriptors","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-03-01T22:18:53Z","receivedAt":"2021-03-01T22:25:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 26 Feb 2021, Jeff King wrote:\n\n> On Fri, Feb 26, 2021 at 11:31:02AM -0500, Taylor Blau wrote:\n>\n> > Dscho mentioned this to me privately when reviewing Coverity results for\n> > -rc0. This one is legitimate, and the fix is easy enough, too.\n>\n> I'm excited that we might get Coverity results again. There were a lot\n> of false positives, but I found its signal-to-noise ratio higher than\n> almost every other static analysis tool I've looked at.\n\nIndeed, the signal:noise ratio is pretty bad, mainly because of all the\nfalse positives (Coverity _really_ hates what we do with `strbuf_slopbuf`,\nit simply doesn't understand that we allocate `buf` only when needing to\nwrite characters into that buffer) and the \"intentional\" issues (we leak\nmemory left and right in `builtin/`).\n\nIt does not help at all that Coverity has a bug for a pretty long while\nnow where it simply throws up its digital hands in the air when it sees a\nGCC v10.x. I did find a work-around for Git for Windows' automated\nCoverity run, a work-around that is somewhat ugly yet necessary, sadly:\nhttps://github.com/git-for-windows/build-extra/commit/23eea104d53\n\nCiao,\nDscho\n"}]}