{"thread":{"id":"57651","subject":"Covierty Integration / Improvement","startedAt":"2022-04-01T21:35:57Z","lastAt":"2022-05-10T17:47:11Z","messageCount":13,"participants":["Markus Vervier","Junio C Hamano","Theodore Ts'o","Ævar Arnfjörð Bjarmason","Johannes Schindelin","Derek Zimmer"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"452921","messageId":"10fd679a-eb94-5380-2070-699f1b56a7b1@x41-dsec.de","threadId":"57651","inReplyTo":null,"subject":"Covierty Integration / Improvement","fromName":"Markus Vervier","fromEmail":"markus.vervier@x41-dsec.de","sentAt":"2022-04-01T20:49:48Z","receivedAt":"2022-04-01T21:35:57Z","isPatch":false,"sender":{"key":"markus.vervier@x41-dsec.de","avatar":null},"body":"Dear git developer team,\n\nX41 is processing the current RfP and some questions came up regarding \nthe improvement / integration of Coverity Scans for git and the \nestimation of the required work:\n\n- Was there a special purpose for the Coverity integration (e.g. custom \nqueries for variant analysis or regression testing?) or did you try to \nintegrate it as a best practice / general security hygiene tool?\n- Could you tell us more about the amount and types of false positives \nand problems you've faced trying to eliminate them? This will help us to \nunderstand the expectations / requirements for a successful integration \nof Coverity.\n- Could we get access to a sample of the scan results?\n\nMany Thanks\n\nMarkus\n-- \nMarkus Vervier (Managing Director)\nX41 D-Sec GmbH, Dennewartstr. 25-27, D-52068 Aachen\nT: +49 241 9809418-0, Fax: -9\nUnternehmenssitz: Aachen, Amtsgericht Aachen: HRB19989\nGeschäftsführer: Markus Vervier"},{"id":"453007","messageId":"xmqqbkxhvoh5.fsf@gitster.g","threadId":"57651","inReplyTo":"10fd679a-eb94-5380-2070-699f1b56a7b1@x41-dsec.de","subject":"Re: Covierty Integration / Improvement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-03T21:36:22Z","receivedAt":"2022-04-03T21:36:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Markus Vervier writes:\n\n> - Was there a special purpose for the Coverity integration\n> (e.g. custom queries for variant analysis or regression testing?) or\n> did you try to integrate it as a best practice / general security\n> hygiene tool?\n> - Could you tell us more about the amount and types of false positives\n> and problems you've faced trying to eliminate them? This will help\n> us to understand the expectations / requirements for a successful\n> integration of Coverity.\n> - Could we get access to a sample of the scan results?\n\nNot sure what X41 RfP is and how it matters to us, but anyway.\n\nI have old e-mails from the scan-admin@coverity.com but the last one\nseems to be from late June 2018, which is ages ago in Git timescale.\nI do not recall us paying for such a service so I am guessing that\nthey had some program that open source projects can enroll, get our\npublic sources scanned and get the result sent back?\n\nhttps://scan.coverity.com/projects/git/ (visible without signing in)\nseems to match my recollection. They haven't been scanning since\nlate June 2018. I wasn't the primary developer who registered us or\nwho has been reading these reports but if I recall correctly, we\nweren't doing anything custom, and fell somewhere between just \"we\nare curious to see how well Coverity works\" and \"Yay, a free\noffering. We have nothing to lose, other than our time, to sign\nourselves up and if it comes up with useful scan result that would\nbe good\".\n\nBelow is a random one back from 2018 timeframe. Looking at the\nplaces around these three issues in the report in the current\ncodebase, I think the first two (i.e. calling die() with some\non-stack variable pointing at an allocated piece of memory in heap,\nletting exit() to eventually clean up after us) are something we do\nnot care about, and consider \"noise\", and the other one (i.e. have\nan open fd, leave the function without an intention to kill the\nprocess soon, forgetting to close it) has long been fixed. I am\nreasonably sure that the \"fix\" happend regardless of the Coverity\nreport.\n\n----- >8 --------- >8 --------- >8 --------- >8 -----\n\n\nHi,\n\nPlease find the latest report on new defect(s) introduced to git found with Coverity Scan.\n\n3 new defect(s) introduced to git found with Coverity Scan.\n\n\nNew defect(s) Reported-by: Coverity Scan\nShowing 3 of 3 defect(s)\n\n\n** CID 1437531: Resource leaks (RESOURCE_LEAK)\n/midx.c: 900 in clear_midx_file()\n\n\n________________________________________________________________________________________________________\n*** CID 1437531: Resource leaks (RESOURCE_LEAK)\n/midx.c: 900 in clear_midx_file()\n894 void clear_midx_file(const char *object_dir)\n895 {\n896 \tchar *midx = get_midx_filename(object_dir);\n897 \n898 \tif (remove_path(midx))\n899 \t\tdie(_(\"failed to clear multi-pack-index at %s\"), midx);\n>>> CID 1437531: Resource leaks (RESOURCE_LEAK)\n>>> Variable \"midx\" going out of scope leaks the storage it points to.\n\n** CID 1437530: Resource leaks (RESOURCE_LEAK)\n/midx.c: 891 in write_midx_file()\n\n\n________________________________________________________________________________________________________\n*** CID 1437530: Resource leaks (RESOURCE_LEAK)\n/midx.c: 891 in write_midx_file()\n885 \t}\n886 \n887 \tFREE_AND_NULL(packs.list);\n888 \tFREE_AND_NULL(packs.names);\n889 \tFREE_AND_NULL(entries);\n890 \tFREE_AND_NULL(pack_perm);\n>>> CID 1437530: Resource leaks (RESOURCE_LEAK)\n>>> Variable \"midx_name\" going out of scope leaks the storage it points to.\n891 \treturn 0;\n892 }\n893 \n894 void clear_midx_file(const char *object_dir)\n895 {\n896 \tchar *midx = get_midx_filename(object_dir);\n897 \n898 \tif (remove_path(midx))\n899 \t\tdie(_(\"failed to clear multi-pack-index at %s\"), midx);\n\n** CID 1437529: Resource leaks (RESOURCE_LEAK)\n/midx.c: 169 in load_multi_pack_index()\n\n\n________________________________________________________________________________________________________\n*** CID 1437529: Resource leaks (RESOURCE_LEAK)\n/midx.c: 169 in load_multi_pack_index()\n163 \t\t\t m->pack_names[i - 1],\n164 \t\t\t m->pack_names[i]);\n165 \t\t\tgoto cleanup_fail;\n166 \t\t}\n167 \t}\n168 \n>>> CID 1437529: Resource leaks (RESOURCE_LEAK)\n>>> Handle variable \"fd\" going out of scope leaks the handle.\n169 \treturn m;\n170 \n171 cleanup_fail:\n172 \tFREE_AND_NULL(m);\n173 \tFREE_AND_NULL(midx_name);\n174 \tmunmap(midx_map, midx_size);\n\n\n________________________________________________________________________________________________________\n"},{"id":"453014","messageId":"Ykoqxx40Fk0DiF9i@mit.edu","threadId":"57651","inReplyTo":"xmqqbkxhvoh5.fsf@gitster.g","subject":"Re: Covierty Integration / Improvement","fromName":"Theodore Ts'o","fromEmail":"tytso@mit.edu","sentAt":"2022-04-03T23:16:23Z","receivedAt":"2022-04-03T23:16:41Z","isPatch":false,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Sun, Apr 03, 2022 at 02:36:22PM -0700, Junio C Hamano wrote:\n> I have old e-mails from the scan-admin@coverity.com but the last one\n> seems to be from late June 2018, which is ages ago in Git timescale.\n> I do not recall us paying for such a service so I am guessing that\n> they had some program that open source projects can enroll, get our\n> public sources scanned and get the result sent back?\n\nYep, that's the way it works. Someone has to use tools provided by\nthem to build the open source project and upload the results for them\nto analyze. Coverity predates github, so it's not new-fangled enough\nto automatically pull sources from repositories; besides, their paying\ncustomers tend to be using their tool for their proprietary software,\nso they haven't had any incentive to create an auto-analyze tool that\npulls from an open source repository.\n\nSome folks at Red Hat do have scripts run out of crontab, that will\nmonitor git branches on projects that they are interested in and when\nthey notice that the branch has been updated, they will build and\nupload the raw material used by Coverity to their dashboard. Eric\nSandeen has been doing this for e2fsprogs, and a few other file system\nrelated repo's, and I suspect if someone asked, he would probably be\nwilling to provide the scripts that he uses.\n\nYou do need to be the project admin, or someone authorized by the\nproject admin, to upload new data for Coverity, or to look at the\nanalysis of the Coverity results. I have no idea who the project\nadmin is for git, but I'm sure if you, as the Git maintainer showed up\nand requested to be added as one of the project admin, the open source\nombudsperson (I don't remember the exact title, but they do have\nsomeone who interfaces with OSS projects), would be happy to oblige.\n\n> https://scan.coverity.com/projects/git/ (visible without signing in)\n> seems to match my recollection. They haven't been scanning since\n> late June 2018. I wasn't the primary developer who registered us or\n> who has been reading these reports but if I recall correctly, we\n> weren't doing anything custom, and fell somewhere between just \"we\n> are curious to see how well Coverity works\" and \"Yay, a free\n> offering. We have nothing to lose, other than our time, to sign\n> ourselves up and if it comes up with useful scan result that would\n> be good\".\n\nMy experience with e2fsprogs is that it does have a fair amount of\nfalse positives, but I've been willing to wade through the false\npositives, and mark them as such in their web dashboard, because the\nearly warnings it gives when we've pushed new code that has a\npotential security problem is worth it. But make no mistake, it\ndefinitely requires a certain amount of maintainer time work with the\ntool.\n\nCheers,\n\n\t\t\t\t\t\t- Ted\n"},{"id":"453032","messageId":"220404.86h779jfws.gmgdl@evledraar.gmail.com","threadId":"57651","inReplyTo":"Ykoqxx40Fk0DiF9i@mit.edu","subject":"Re: Covierty Integration / Improvement","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-04T10:14:54Z","receivedAt":"2022-04-04T10:34:35Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nCOVID19 is spreading via E-Mail now? It's $subject =~\ns/Covierty/Coverity/g :)\n\nOn Sun, Apr 03 2022, Theodore Ts'o wrote:\n\n> On Sun, Apr 03, 2022 at 02:36:22PM -0700, Junio C Hamano wrote:\n>> I have old e-mails from the scan-admin@coverity.com but the last one\n>> seems to be from late June 2018, which is ages ago in Git timescale.\n>> I do not recall us paying for such a service so I am guessing that\n>> they had some program that open source projects can enroll, get our\n>> public sources scanned and get the result sent back?\n>\n> Yep, that's the way it works. Someone has to use tools provided by\n> them to build the open source project and upload the results for them\n> to analyze. Coverity predates github, so it's not new-fangled enough\n> to automatically pull sources from repositories; besides, their paying\n> customers tend to be using their tool for their proprietary software,\n> so they haven't had any incentive to create an auto-analyze tool that\n> pulls from an open source repository.\n>\n> Some folks at Red Hat do have scripts run out of crontab, that will\n> monitor git branches on projects that they are interested in and when\n> they notice that the branch has been updated, they will build and\n> upload the raw material used by Coverity to their dashboard. Eric\n> Sandeen has been doing this for e2fsprogs, and a few other file system\n> related repo's, and I suspect if someone asked, he would probably be\n> willing to provide the scripts that he uses.\n>\n> You do need to be the project admin, or someone authorized by the\n> project admin, to upload new data for Coverity, or to look at the\n> analysis of the Coverity results. I have no idea who the project\n> admin is for git, but I'm sure if you, as the Git maintainer showed up\n> and requested to be added as one of the project admin, the open source\n> ombudsperson (I don't remember the exact title, but they do have\n> someone who interfaces with OSS projects), would be happy to oblige.\n\nPer\nhttps://lore.kernel.org/git/YarO3nkrutmWF7nb@coredump.intra.peff.net/\nJeff ran this from his fork, I'm not sure if that was because he set\nsomething up in the git/git organization, or if by project admin you\nmean that any fork of it can set this up on their own.\n\n>> https://scan.coverity.com/projects/git/ (visible without signing in)\n>> seems to match my recollection. They haven't been scanning since\n>> late June 2018. I wasn't the primary developer who registered us or\n>> who has been reading these reports but if I recall correctly, we\n>> weren't doing anything custom, and fell somewhere between just \"we\n>> are curious to see how well Coverity works\" and \"Yay, a free\n>> offering. We have nothing to lose, other than our time, to sign\n>> ourselves up and if it comes up with useful scan result that would\n>> be good\".\n>\n> My experience with e2fsprogs is that it does have a fair amount of\n> false positives, but I've been willing to wade through the false\n> positives, and mark them as such in their web dashboard, because the\n> early warnings it gives when we've pushed new code that has a\n> potential security problem is worth it. But make no mistake, it\n> definitely requires a certain amount of maintainer time work with the\n> tool.\n\nYes, also per the linked-above output it's quite noise, but there looked\nto be some legitimate and hard-to find issues in those reports. It would\nbe nice to get them running with some regularity on our main branches.\n"},{"id":"453145","messageId":"nycvar.QRO.7.76.6.2204060019300.379@tvgsbejvaqbjf.bet","threadId":"57651","inReplyTo":"Ykoqxx40Fk0DiF9i@mit.edu","subject":"Re: Covierty Integration / Improvement","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-04-05T22:22:16Z","receivedAt":"2022-04-06T05:07:46Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Theodore,\n\nOn Sun, 3 Apr 2022, Theodore Ts'o wrote:\n\n> You do need to be the project admin, or someone authorized by the\n> project admin, to upload new data for Coverity, or to look at the\n> analysis of the Coverity results.\n\nTo be precise, there are a couple of roles you can have, one being\nMaintainer/Owner, another being Contributor/Member, yet another being\nDefect Viewer. IIUC contributors can upload new data.\n\n> I have no idea who the project admin is for git, but I'm sure if you, as\n> the Git maintainer showed up and requested to be added as one of the\n> project admin, the open source ombudsperson (I don't remember the exact\n> title, but they do have someone who interfaces with OSS projects), would\n> be happy to oblige.\n\nJunio is already a member. In fact, I bumped him up to be admin :-)\n\nCiao,\nJohannes\n"},{"id":"453148","messageId":"nycvar.QRO.7.76.6.2204052352030.379@tvgsbejvaqbjf.bet","threadId":"57651","inReplyTo":"10fd679a-eb94-5380-2070-699f1b56a7b1@x41-dsec.de","subject":"Re: Covierty Integration / Improvement","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-04-05T22:17:12Z","receivedAt":"2022-04-06T05:07:46Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Markus,\n\nOn Fri, 1 Apr 2022, Markus Vervier wrote:\n\n> Dear git developer team,\n>\n> X41 is processing the current RfP\n\nwould you kindly provide a bit more context? This seems to come right out\nof left field. Is \"RfP\" a \"Request for Proposals\"? If so, I am not aware\nthat the git developer team submitted one...\n\n> and some questions came up regarding the\n> improvement / integration of Coverity Scans for git and the estimation of the\n> required work:\n>\n> - Was there a special purpose for the Coverity integration (e.g. custom\n> queries for variant analysis or regression testing?) or did you try to\n> integrate it as a best practice / general security hygiene tool?\n\nThere has been talk about integrating Coverity into our regular CI builds,\nbut nothing actionable has materialized yet.\n\nGit for Windows has such a CI build, but it is currently broken due to a\nbackwards-incompatible change in Coverity that will require a human to\nadjust the CI definition, which has not yet happened due to lack of time.\n\n> - Could you tell us more about the amount and types of false positives and\n> problems you've faced trying to eliminate them? This will help us to\n> understand the expectations / requirements for a successful integration of\n> Coverity.\n\nFrom the top of my head, I would estimate about 60-70% of the results to\nbe false positives.\n\nAs Junio pointed out, we do not consider memory to be leaked in one-shot\nprocesses where memory is allocated, once, in the equivalent of a `main()`\nfunction. Sure, we could add a slew of `free()` calls right before exiting\nthe process, but that's kind of pointless.\n\nAnother major source of false positives is our string data structure,\nwhich offers a small-ish static, read-only buffer to get started, but\nreplaces that with something `malloc()`ed/`realloc()`ed as soon as the\nstring is about to be manipulated. Yet Coverity insists that we're writing\ninto a read-only buffer, and get out of bounds, which is simply not true.\n\nSimilar issues are reported with our `strvec` data structure that has the\nsame allocation pattern.\n\nSince the false positives outnumber the valid issues reported by Coverity,\nwe have not been very eager to sift through new reports.\n\nThe list of categories of false positives listed above is not exhaustive,\nof course, but combined with how cumbersome it is to get access to the\nreports (they cannot be viewed anonymously), you get an idea why we do not\npay all that much attention to Coverity.\n\n> - Could we get access to a sample of the scan results?\n\nSure, if you direct your web browser to\nhttps://scan.coverity.com/projects/git, there is a button \"Add me to\nproject\".\n\nCiao,\nJohannes\n\n>\n> Many Thanks\n>\n> Markus\n> --\n> Markus Vervier (Managing Director)\n> X41 D-Sec GmbH, Dennewartstr. 25-27, D-52068 Aachen\n> T: +49 241 9809418-0, Fax: -9\n> Unternehmenssitz: Aachen, Amtsgericht Aachen: HRB19989\n> Geschäftsführer: Markus Vervier\n>\n"},{"id":"453183","messageId":"nycvar.QRO.7.76.6.2204061620110.379@tvgsbejvaqbjf.bet","threadId":"57651","inReplyTo":"nycvar.QRO.7.76.6.2204052352030.379@tvgsbejvaqbjf.bet","subject":"Re: Covierty Integration / Improvement","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-04-06T15:08:37Z","receivedAt":"2022-04-06T16:34:35Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Team,\n\nOn Wed, 6 Apr 2022, Johannes Schindelin wrote:\n\n> On Fri, 1 Apr 2022, Markus Vervier wrote:\n>\n> > - Could you tell us more about the amount and types of false positives\n> > and problems you've faced trying to eliminate them? This will help us\n> > to understand the expectations / requirements for a successful\n> > integration of Coverity.\n>\n> From the top of my head, I would estimate about 60-70% of the results to\n> be false positives.\n>\n> As Junio pointed out, we do not consider memory to be leaked in one-shot\n> processes where memory is allocated, once, in the equivalent of a\n> `main()` function. Sure, we could add a slew of `free()` calls right\n> before exiting the process, but that's kind of pointless.\n>\n> Another major source of false positives is our string data structure,\n> which offers a small-ish static, read-only buffer to get started, but\n> replaces that with something `malloc()`ed/`realloc()`ed as soon as the\n> string is about to be manipulated. Yet Coverity insists that we're\n> writing into a read-only buffer, and get out of bounds, which is simply\n> not true.\n>\n> Similar issues are reported with our `strvec` data structure that has\n> the same allocation pattern.\n>\n> Since the false positives outnumber the valid issues reported by\n> Coverity, we have not been very eager to sift through new reports.\n>\n> The list of categories of false positives listed above is not\n> exhaustive, of course, but combined with how cumbersome it is to get\n> access to the reports (they cannot be viewed anonymously), you get an\n> idea why we do not pay all that much attention to Coverity.\n\nI have fixed Git for Windows' Coverity build and started to sift through\nthe 154 new defects reported as of v2.36.0-rc0.\n\nSadly, there is now a new class of overwhelming false positives: Coverity\nclaims that \"strbuf_addstr does not [NUL-]terminate\", which is of course\nfalse. Specifically, Coverity explains that:\n\n/strbuf.c\n296 void strbuf_add(struct strbuf *sb, const void *data, size_t len)\n297 {\n298 strbuf_grow(sb, len);\n 1. string_copy: Calling memcpy copies a source string data to sb->buf.\n 2. string_null_source: The argument sb->buf will not be\n null[sic!]-terminated, because either the source string is not\n null-terminated, or the length of source string data is greater than\n or equal to the size argument len.\n299 memcpy(sb->buf + sb->len, data, len);\n300 strbuf_setlen(sb, sb->len + len);\n301 }\n\nIn other words, it misses the fact that `strbuf_setlen()` _does_ set\n`sb->buf[sb->len] = '\\0'` (I assume that Coverity gets confused by the\n`slopbuf` once again).\n\nI stopped after the first 30-40 instances of \"String not null terminated\"\nreports because my time is a bit too expensive to spend on reports like\nthat. Among the reported issues I looked at, there were two false\npositives where Coverity misinterpreted how much space was allocated (and\nthought we'd overrun, which we don't), the rest were those NUL-termination\nfalse positives.\n\nCiao,\nJohannes\n"},{"id":"453213","messageId":"Yk3UAz3sn9KhMnyf@mit.edu","threadId":"57651","inReplyTo":"nycvar.QRO.7.76.6.2204061620110.379@tvgsbejvaqbjf.bet","subject":"Re: Covierty Integration / Improvement","fromName":"Theodore Ts'o","fromEmail":"tytso@mit.edu","sentAt":"2022-04-06T17:55:15Z","receivedAt":"2022-04-06T20:12:38Z","isPatch":false,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Wed, Apr 06, 2022 at 05:08:37PM +0200, Johannes Schindelin wrote:\n> I have fixed Git for Windows' Coverity build and started to sift through\n> the 154 new defects reported as of v2.36.0-rc0.\n> \n> Sadly, there is now a new class of overwhelming false positives: Coverity\n> claims that \"strbuf_addstr does not [NUL-]terminate\", which is of course\n> false.\n\nIt should be possible to suppress this by uploading a Coverity model\nfile. See[1] for more details:\n\n[1] https://community.synopsys.com/s/article/practical-example-of-coverity-function-model\n\nI've suppressed a similar issue by using the attribute __nonstring,\nbut I don't think that will work for git, because strbuf->buf really\n*is* a NUL-terminated string, where as in ext4 we have some fields\nwhich are designed to be NUL padded, but it is *not* guaranteed to be\nNUL-terminated:\n\n#ifndef __nonstring\n#ifdef __has_attribute\n#if __has_attribute(__nonstring__)\n#define __nonstring __attribute__((__nonstring__))\n#else\n#define __nonstring\n#endif /* __has_attribute(__nonstring__) */\n#else\n# define __nonstring\n#endif /* __has_attribute */\n#endif /* __nonstring */\n\nstruct ext2_super_block {\n ...\n/*068*/\t__u8\ts_uuid[16] __nonstring;\t\t/* 128-bit uuid for volume */\n/*078*/\t__u8\ts_volume_name[EXT2_LABEL_LEN] __nonstring;\t/* volume name */\n ...\n};\n\n(This is needed to suppress warnings by Clang as well.)\n\nUsing __nonstring will result in attempts to use s_volume_name in \"C\"\nstring context to give a warning, which is why this isn't right for\nstrbuf->buf.\n\nCheers,\n\n\t\t\t\t\t\t- Ted\n"},{"id":"453225","messageId":"xmqqfsmqx8ud.fsf@gitster.g","threadId":"57651","inReplyTo":"Yk3UAz3sn9KhMnyf@mit.edu","subject":"Re: Covierty Integration / Improvement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-06T20:20:10Z","receivedAt":"2022-04-06T21:23:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Theodore Ts'o\" writes:\n\n> On Wed, Apr 06, 2022 at 05:08:37PM +0200, Johannes Schindelin wrote:\n>> I have fixed Git for Windows' Coverity build and started to sift through\n>> the 154 new defects reported as of v2.36.0-rc0.\n>> \n>> Sadly, there is now a new class of overwhelming false positives: Coverity\n>> claims that \"strbuf_addstr does not [NUL-]terminate\", which is of course\n>> false.\n>\n> It should be possible to suppress this by uploading a Coverity model\n> file. See[1] for more details:\n>\n> [1] https://community.synopsys.com/s/article/practical-example-of-coverity-function-model\n>\n> I've suppressed a similar issue by using the attribute __nonstring,\n> but I don't think that will work for git, because strbuf->buf really\n> *is* a NUL-terminated string, where as in ext4 we have some fields\n> which are designed to be NUL padded, but it is *not* guaranteed to be\n> NUL-terminated:\n\nThat is very much expected from filesystem code, for which strncmp()\nand friends were invented for ;-)\n\n> (This is needed to suppress warnings by Clang as well.)\n>\n> Using __nonstring will result in attempts to use s_volume_name in \"C\"\n> string context to give a warning, which is why this isn't right for\n> strbuf->buf.\n\nIndeed. We do aim to make reading .buf member as NUL-terminated\nstring safe, so it would make it very inconvenient to warn against\nsuch uses.\n\nThanks.\n\n\n"},{"id":"453245","messageId":"6cb10d5e-d8f2-0d7e-a15a-4728466e0c21@x41-dsec.de","threadId":"57651","inReplyTo":"nycvar.QRO.7.76.6.2204052352030.379@tvgsbejvaqbjf.bet","subject":"Re: Covierty Integration / Improvement","fromName":"Markus Vervier","fromEmail":"markus.vervier@x41-dsec.de","sentAt":"2022-04-07T07:21:55Z","receivedAt":"2022-04-07T07:22:28Z","isPatch":false,"sender":{"key":"markus.vervier@x41-dsec.de","avatar":null},"body":"Hi Johannes,\n\nOn 4/6/22 00:17, Johannes Schindelin wrote:\n> On Fri, 1 Apr 2022, Markus Vervier wrote:\n>> X41 is processing the current RfP\n> would you kindly provide a bit more context? This seems to come right out\n> of left field. Is \"RfP\" a \"Request for Proposals\"? If so, I am not aware\n> that the git developer team submitted one...\n\nthank you and everyone else for their comments. To clear up the context:\n\nThe OSTIF (https://ostif.org) is organizing a security audit for git\nand one of the questions was about Coverity and if the results it gave \nin the past could be verified and/or improved.\n\nMany Thanks\n\nMarkus\n-- \nMarkus Vervier (Managing Director)\nX41 D-Sec GmbH, Dennewartstr. 25-27, D-52068 Aachen\nT: +49 241 9809418-0, Fax: -9\nUnternehmenssitz: Aachen, Amtsgericht Aachen: HRB19989\nGeschäftsführer: Markus Vervier\n"},{"id":"453251","messageId":"nycvar.QRO.7.76.6.2204071344210.347@tvgsbejvaqbjf.bet","threadId":"57651","inReplyTo":"Yk3UAz3sn9KhMnyf@mit.edu","subject":"Re: Covierty Integration / Improvement","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-04-07T11:49:52Z","receivedAt":"2022-04-07T11:50:06Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Theodore,\n\nOn Wed, 6 Apr 2022, Theodore Ts'o wrote:\n\n> On Wed, Apr 06, 2022 at 05:08:37PM +0200, Johannes Schindelin wrote:\n> > I have fixed Git for Windows' Coverity build and started to sift through\n> > the 154 new defects reported as of v2.36.0-rc0.\n> >\n> > Sadly, there is now a new class of overwhelming false positives: Coverity\n> > claims that \"strbuf_addstr does not [NUL-]terminate\", which is of course\n> > false.\n>\n> It should be possible to suppress this by uploading a Coverity model\n> file. See[1] for more details:\n>\n> [1] https://community.synopsys.com/s/article/practical-example-of-coverity-function-model\n\nRight, I know about this, my apologies for being too succinct. For the\nrecord, here is how we submit the Coverity builds in Git for Windows:\n\nhttps://github.com/git-for-windows/build-extra/blob/bea0c37bdc3737843b7808269b077b30e51c67fb/please.sh#L1793-L1925\n\nUnfortunately, you cannot see the model file because that is not provided\nper build, but globally for the entire project. Here is the model file we\ncurrently use:\n\n-- snip --\n/* modelfile for git */\n\nchar strbuf_slopbuf[64];\n\nvoid *malloc(size_t);\nvoid *calloc(size_t, size_t);\nvoid *realloc(void *, size_t);\nvoid free(void *);\n\nvoid *xrealloc(void *ptr, size_t size)\n{\n\tvoid *ret = realloc(ptr, size);\n\tif (!ret) __coverity_panic__();\n\treturn ret;\n}\n\nvoid *xmalloc(size_t size)\n{\n\tvoid *mem = malloc(size);\n\tif (!mem) __coverity_panic__();\n\treturn mem;\n}\n\nvoid xcalloc(size_t num, size_t size)\n{\n\tvoid *ret = calloc(num, size);\n\tif (!ret) __coverity_panic__();\n\treturn ret;\n}\n\nvoid usage(const char *err) {\n __coverity_panic__();\n}\n\nvoid usagef(const char *err, ...) {\n __coverity_panic__();\n}\n\nvoid die(const char *err, ...) {\n __coverity_panic__();\n}\n\nvoid die_errno(const char *err, ...) {\n __coverity_panic__();\n}\n-- snap --\n\nI _guess_ we could \"help\" Coverity by providing some alternative\nimplementation of `strbuf_add()` that does not use `memcpy()` but instead\na loop that explicitly NUL-terminates the string.\n\nBut it feels wrong to do that because that would weaken the analysis\nbecause Coverity would not actually analyze the code that is executed\nanymore.\n\n> I've suppressed a similar issue by using the attribute __nonstring,\n> but I don't think that will work for git, because strbuf->buf really\n> *is* a NUL-terminated string, where as in ext4 we have some fields\n> which are designed to be NUL padded, but it is *not* guaranteed to be\n> NUL-terminated:\n>\n> #ifndef __nonstring\n> #ifdef __has_attribute\n> #if __has_attribute(__nonstring__)\n> #define __nonstring __attribute__((__nonstring__))\n> #else\n> #define __nonstring\n> #endif /* __has_attribute(__nonstring__) */\n> #else\n> # define __nonstring\n> #endif /* __has_attribute */\n> #endif /* __nonstring */\n>\n> struct ext2_super_block {\n> ...\n> /*068*/\t__u8\ts_uuid[16] __nonstring;\t\t/* 128-bit uuid for volume */\n> /*078*/\t__u8\ts_volume_name[EXT2_LABEL_LEN] __nonstring;\t/* volume name */\n> ...\n> };\n>\n> (This is needed to suppress warnings by Clang as well.)\n>\n> Using __nonstring will result in attempts to use s_volume_name in \"C\"\n> string context to give a warning, which is why this isn't right for\n> strbuf->buf.\n\nCorrect. I guess we could fool around with the model file until those\nfalse positives are gone, but I have to admit that I cannot justify the\ntime to work on this.\n\nCiao,\nJohannes\n"},{"id":"453252","messageId":"nycvar.QRO.7.76.6.2204071350080.347@tvgsbejvaqbjf.bet","threadId":"57651","inReplyTo":"6cb10d5e-d8f2-0d7e-a15a-4728466e0c21@x41-dsec.de","subject":"Re: Covierty Integration / Improvement","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-04-07T11:58:31Z","receivedAt":"2022-04-07T11:58:42Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Markus,\n\nOn Thu, 7 Apr 2022, Markus Vervier wrote:\n\n> On 4/6/22 00:17, Johannes Schindelin wrote:\n> > On Fri, 1 Apr 2022, Markus Vervier wrote:\n> > > X41 is processing the current RfP\n> > would you kindly provide a bit more context? This seems to come right out\n> > of left field. Is \"RfP\" a \"Request for Proposals\"? If so, I am not aware\n> > that the git developer team submitted one...\n>\n> thank you and everyone else for their comments. To clear up the context:\n>\n> The OSTIF (https://ostif.org) is organizing a security audit for git\n> and one of the questions was about Coverity and if the results it gave in the\n> past could be verified and/or improved.\n\nThank you for the context!\n\nIf OSTIF can help us get better support from Coverity (as you can see at\nhttps://github.com/git-for-windows/build-extra/commit/23eea104 I could\nhave wished for a better experience there), I am all for it!\n\nOut of curiosity: are you (or is OSTIF) affiliated with Synopsys somehow?\n\nIf not, have you considered if you could help us getting a comprehensive\nCodeQL coverage instead? Theoretically, CodeQL should be able to do the\nsame as Coverity, while allowing us to tweak the analysis in a lot more\npowerful ways than Coverity (most notably, it should allow us to reduce\nthe number of false positives rather dramatically).\n\nIt is the number of knobs CodeQL allows that has looked too daunting for\nme to give it more than a cursory try [*1*].\n\nThank you,\nJohannes\n\nFootnote *1*: I had played with CodeQL last year but was called away to a\nmore pressing project, therefore this is woefully incomplete:\nhttps://github.com/git-for-windows/git/compare/main...dscho:codeql\n"},{"id":"455097","messageId":"CAJY0qZJ5sUmXeDZG45fvotcvFo4PRQy1Fv64Y1J7OjTF-8s0Mg@mail.gmail.com","threadId":"57651","inReplyTo":"CAJY0qZJaBvwA19PN=Gm4c5gSVqYYBOoVwgF=1mZTNEjmXFSc7A@mail.gmail.com","subject":"Re: Covierty Integration / Improvement","fromName":"Derek Zimmer","fromEmail":"derek@ostif.org","sentAt":"2022-05-10T17:46:25Z","receivedAt":"2022-05-10T17:47:11Z","isPatch":false,"sender":{"key":"derek@ostif.org","avatar":null},"body":"Hello all, (this is a resend, google mail arbitrarily decides to\nswitch out of plaintext whenever it likes)\n\nThank you for the conversations about Coverity. After some internal\ndiscussions and negotiating with our security partners, we have\nsecured some engineers directly from Github who want to work on CodeQL\nfor Git. They will do the work of getting CodeQL working, do a scan,\nand then evaluate how much work getting CodeQL into a usable state for\nGit is by looking at the false positive rate and figuring out what can\nbe muted with rules, and the false negative rate vs Coverity / other\ncurrent tests and create some custom tests.\n\nThis should give us a good baseline on what is needed to get Git a\nsolid security scanner for the CI/CD pipeline. We are focusing on\nmaking the results useful and removing nags to save as much developer\ntime as possible when using it, so that you get the security benefits\nwithout significant drawbacks.\n\nDo we have anyone here that is interested in helping the team set up\nCodeQL? I'm sure the engineers will have some questions, especially\nregarding the current Coverity mess and what needs to improve in order\nto make this new setup more usable.\n\nDerek Zimmer\nExecutive Director\nOpen Source Technology Improvement Fund\n\nDerek Zimmer\nExecutive Director\nOpen Source Technology Improvement Fund\n\n\nOn Tue, May 10, 2022 at 12:43 PM Derek Zimmer wrote:\n>\n> Hello all,\n>\n> Thank you for the conversations about Coverity. After some internal discussions and negotiating with our security partners, we have secured some engineers directly from Github who want to work on CodeQL for Git. They will do the work of getting CodeQL working, do a scan, and then evaluate how much work getting CodeQL into a usable state for Git is by looking at the false positive rate and figuring out what can be muted with rules, and the false negative rate vs Coverity / other current tests and create some custom tests.\n>\n> This should give us a good baseline on what is needed to get Git a solid security scanner for the CI/CD pipeline. We are focusing on making the results useful and removing nags to save as much developer time as possible when using it, so that you get the security benefits without significant drawbacks.\n>\n> Do we have anyone here that is interested in helping the team set up CodeQL? I'm sure the engineers will have some questions, especially regarding the current Coverity mess and what needs to improve in order to make this new setup more usable.\n>\n> Derek Zimmer\n> Executive Director\n> Open Source Technology Improvement Fund\n>\n> On Mon, Apr 11, 2022 at 1:49 PM Derek Zimmer wrote:\n>>\n>> Hello all,\n>>\n>> Answers inline + more context\n>>\n>> > If OSTIF can help us get better support from Coverity (as you can see at\n>> > https://github.com/git-for-windows/build-extra/commit/23eea104 I could\n>> > have wished for a better experience there), I am all for it!\n>>\n>> We may be able to convince them to help based on the volume of work that we do with many open source projects. Not helping one open source project may seem like a small loss to them. Not getting recommended to hundreds of high profile projects because of lacking support is different. It is especially concerning that this particular bug likely affects a huge number of customers.\n>>\n>> > If not, have you considered if you could help us getting a comprehensive\n>> > CodeQL coverage instead? Theoretically, CodeQL should be able to do the\n>> > same as Coverity, while allowing us to tweak the analysis in a lot more\n>> > powerful ways than Coverity (most notably, it should allow us to reduce\n>> > the number of false positives rather dramatically).\n>>\n>> This is absolutely an option, although we may have to petition Google / OpenSSF / the Linux Foundation for a slight increase in funding, as setting up CodeQL from scratch is a much more laborious task than setting up rules for an existing Coverity setup. We absolutely can do this, but we'd have to split it into a second project with separate funding in order to keep the primary work moving forward while we work out the details.\n>>\n>> If you ultimately think that setting up CodeQL will yield better results overall for Git, I can get started on finding the resources to get it done immediately. (I have a meeting with the Linux Foundation tomorrow.)\n>>\n>> If we are going to go with CodeQL as a separate project, we can drop the Coverity work from the current SoW/Proposal and proceed with all of the other action items.\n>>\n>> Let me know your thoughts everyone on what best suits Git here. It sounds to me like CodeQL is the way to go but if there's a compelling argument for Coverity we can explore that.\n>>\n>> All the best,\n>>\n>> Derek Zimmer\n>> Executive Director\n>> Open Source Technology Improvement Fund\n>>\n>>\n>> On Thu, Apr 7, 2022 at 6:58 AM Johannes Schindelin wrote:\n>>>\n>>> Hi Markus,\n>>>\n>>> On Thu, 7 Apr 2022, Markus Vervier wrote:\n>>>\n>>> > On 4/6/22 00:17, Johannes Schindelin wrote:\n>>> > > On Fri, 1 Apr 2022, Markus Vervier wrote:\n>>> > > > X41 is processing the current RfP\n>>> > > would you kindly provide a bit more context? This seems to come right out\n>>> > > of left field. Is \"RfP\" a \"Request for Proposals\"? If so, I am not aware\n>>> > > that the git developer team submitted one...\n>>> >\n>>> > thank you and everyone else for their comments. To clear up the context:\n>>> >\n>>> > The OSTIF (https://ostif.org) is organizing a security audit for git\n>>> > and one of the questions was about Coverity and if the results it gave in the\n>>> > past could be verified and/or improved.\n>>>\n>>> Thank you for the context!\n>>>\n>>> If OSTIF can help us get better support from Coverity (as you can see at\n>>> https://github.com/git-for-windows/build-extra/commit/23eea104 I could\n>>> have wished for a better experience there), I am all for it!\n>>>\n>>> Out of curiosity: are you (or is OSTIF) affiliated with Synopsys somehow?\n>>>\n>>> If not, have you considered if you could help us getting a comprehensive\n>>> CodeQL coverage instead? Theoretically, CodeQL should be able to do the\n>>> same as Coverity, while allowing us to tweak the analysis in a lot more\n>>> powerful ways than Coverity (most notably, it should allow us to reduce\n>>> the number of false positives rather dramatically).\n>>>\n>>> It is the number of knobs CodeQL allows that has looked too daunting for\n>>> me to give it more than a cursory try [*1*].\n>>>\n>>> Thank you,\n>>> Johannes\n>>>\n>>> Footnote *1*: I had played with CodeQL last year but was called away to a\n>>> more pressing project, therefore this is woefully incomplete:\n>>> https://github.com/git-for-windows/git/compare/main...dscho:codeql\n"}]}