{"thread":{"id":"44339","subject":"Fwd: New Defects reported by Coverity Scan for git","startedAt":"2016-10-20T17:05:43Z","lastAt":"2016-10-20T22:07:53Z","messageCount":9,"participants":["Stefan Beller","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"304481","messageId":"CAGZ79kabVPhp0_z-e_4jJOFq+jzSE2SsgmFuY-2RUgrEviGKyA@mail.gmail.com","threadId":"44339","inReplyTo":"580893d5a4736_4ed37b53181837@ss1435.mail","subject":"Fwd: New Defects reported by Coverity Scan for git","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-20T17:05:38Z","receivedAt":"2016-10-20T17:05:43Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Not sure what triggered the new finding of coverity as seen below as the\nparse_commit() was not touched. Junios series regarding the merge base\noptimization touches a bit of code nearby though.\n\nDo we want to replace the unchecked places of parse_commit with\nparse_commit_or_die ?\n\nThanks,\nStefan\n_________________________________________________________\n*** CID 1374088:  Error handling issues  (CHECKED_RETURN)\n/commit.c: 913 in mark_redundant()\n907\n908             work = xcalloc(cnt, sizeof(*work));\n909             redundant = xcalloc(cnt, 1);\n910             ALLOC_ARRAY(filled_index, cnt - 1);\n911\n912             for (i = 0; i < cnt; i++)\n>>>     CID 1374088:  Error handling issues  (CHECKED_RETURN)\n>>>     Calling \"parse_commit\" without checking return value (as is done elsewhere 37 out of 45 times).\n913                     parse_commit(array[i]);\n914             for (i = 0; i < cnt; i++) {\n915                     struct commit_list *common;\n916\n917                     if (redundant[i])\n918                             continue;\n"},{"id":"304486","messageId":"xmqqshrqhpua.fsf@gitster.mtv.corp.google.com","threadId":"44339","inReplyTo":"CAGZ79kabVPhp0_z-e_4jJOFq+jzSE2SsgmFuY-2RUgrEviGKyA@mail.gmail.com","subject":"Re: Fwd: New Defects reported by Coverity Scan for git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-20T17:50:37Z","receivedAt":"2016-10-20T17:50:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Not sure what triggered the new finding of coverity as seen below as the\n> parse_commit() was not touched. Junios series regarding the merge base\n> optimization touches a bit of code nearby though.\n>\n> Do we want to replace the unchecked places of parse_commit with\n> parse_commit_or_die ?\n\nThe reason parse_commit() would fail at this point would be because\nthe repository is corrupt, I do not think it would hurt to do such a\nchange.  \n\nI agree that it is curious why it shows up as a \"new defect\",\nthough.\n\nBy the way, do you know who is managing the service on our end\n(e.g. approving new people to be \"defect viewer\")?  The site seems\nto think I have the power to manage others' subscription, which I do\nnot think I have (I do not go to the site myself).  As it spewed\nquite a many false positives into my mailbox in the past, I do not\npay very close attention to these reports these days, but I still\nread the e-mailed reports every once in a while.\n\nThanks.\n"},{"id":"304487","messageId":"CAGZ79kYTwsWy+oxnPKghWKGLEaO9qWZbrQOOejeEBOe0c-o0Pg@mail.gmail.com","threadId":"44339","inReplyTo":"xmqqshrqhpua.fsf@gitster.mtv.corp.google.com","subject":"Re: Fwd: New Defects reported by Coverity Scan for git","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-20T17:58:39Z","receivedAt":"2016-10-20T17:58:59Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 20, 2016 at 10:50 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> Not sure what triggered the new finding of coverity as seen below as the\n>> parse_commit() was not touched. Junios series regarding the merge base\n>> optimization touches a bit of code nearby though.\n>>\n>> Do we want to replace the unchecked places of parse_commit with\n>> parse_commit_or_die ?\n>\n> The reason parse_commit() would fail at this point would be because\n> the repository is corrupt, I do not think it would hurt to do such a\n> change.\n>\n> I agree that it is curious why it shows up as a \"new defect\",\n> though.\n>\n> By the way, do you know who is managing the service on our end\n> (e.g. approving new people to be \"defect viewer\")?\n\nI do it most of the time, but I did not start managing it.\nAnd I have been pretty lax/liberal about handing out rights to do stuff,\nbecause I did not want to tip on anyone's toes giving to few rights\nand thereby annoying them.\n\nI see that some of these emails may be inconvenient to you, I can\nchange your role to defect viewer/contributor if you prefer.\n\nThanks,\nStefan\n"},{"id":"304490","messageId":"xmqqoa2ehp5u.fsf@gitster.mtv.corp.google.com","threadId":"44339","inReplyTo":"CAGZ79kYTwsWy+oxnPKghWKGLEaO9qWZbrQOOejeEBOe0c-o0Pg@mail.gmail.com","subject":"Re: Fwd: New Defects reported by Coverity Scan for git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-20T18:05:17Z","receivedAt":"2016-10-20T18:05:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> I do it most of the time, but I did not start managing it.\n> And I have been pretty lax/liberal about handing out rights to do stuff,\n> because I did not want to tip on anyone's toes giving to few rights\n> and thereby annoying them.\n\nGood to know that you have been managing it; I was mostly worried\nabout not having anybody managing it (i.e. imagining Coverity\nnominated/volunteered me as manager with everybody else as viewers)\nand the new viewer requests get totally ignored by the project as\nthe whole.\n\n> I see that some of these emails may be inconvenient to you, I can\n> change your role to defect viewer/contributor if you prefer.\n\nIt is not a huge inconvenience to me, because any piece of e-mail\nthat is addressed directly to gitster@ without CC'ing to git@vger\nand is not a follow-up to any earlier message goes to a separate\nlowest-priority mailbox that I rarely look at.  But if it is easy\nto recategorize me, please do so.\n\nThanks.\n"},{"id":"304491","messageId":"20161020180629.11955-1-sbeller@google.com","threadId":"44339","inReplyTo":"xmqqoa2ehp5u.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] commit parsing: replace unchecked parse_commit by parse_commit_or_die","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-20T18:06:29Z","receivedAt":"2016-10-20T18:06:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The reason parse_commit() would fail at these points would be because\nthe repository is corrupt.\n\nThis was noticed by coverity.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n developed on pu as that's where coverity spotted it.\n I have no overview if these areas are being worked on. (It may clash\n with at least jc/merge-base-fp-only)\n \n Thanks,\n Stefan\n\n builtin/blame.c       | 2 +-\n builtin/describe.c    | 4 ++--\n builtin/name-rev.c    | 2 +-\n builtin/show-branch.c | 4 ++--\n commit.c              | 2 +-\n fetch-pack.c          | 2 +-\n 6 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 992a79c..3b8564c 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1801,7 +1801,7 @@ static void assign_blame(struct scoreboard *sb, int opt)\n \t\t * so hold onto it in the meantime.\n \t\t */\n \t\torigin_incref(suspect);\n-\t\tparse_commit(commit);\n+\t\tparse_commit_or_die(commit);\n \t\tif (reverse ||\n \t\t    (!(commit->object.flags & UNINTERESTING) &&\n \t\t     !(revs->max_age != -1 && commit->date < revs->max_age)))\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 01490a1..8299b16 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -199,7 +199,7 @@ static unsigned long finish_depth_computation(\n \t\t\tbest->depth++;\n \t\twhile (parents) {\n \t\t\tstruct commit *p = parents->item;\n-\t\t\tparse_commit(p);\n+\t\t\tparse_commit_or_die(p);\n \t\t\tif (!(p->object.flags & SEEN))\n \t\t\t\tcommit_list_insert_by_date(p, list);\n \t\t\tp->object.flags |= c->object.flags;\n@@ -322,7 +322,7 @@ static void describe(const char *arg, int last_one)\n \t\t}\n \t\twhile (parents) {\n \t\t\tstruct commit *p = parents->item;\n-\t\t\tparse_commit(p);\n+\t\t\tparse_commit_or_die(p);\n \t\t\tif (!(p->object.flags & SEEN))\n \t\t\t\tcommit_list_insert_by_date(p, &list);\n \t\t\tp->object.flags |= c->object.flags;\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex cd89d48..92c3316 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -29,7 +29,7 @@ static void name_rev(struct commit *commit,\n \tstruct commit_list *parents;\n \tint parent_number = 1;\n \n-\tparse_commit(commit);\n+\tparse_commit_or_die(commit);\n \n \tif (commit->date < cutoff)\n \t\treturn;\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 974f340..fd911b5 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -218,7 +218,7 @@ static void join_revs(struct commit_list **list_p,\n \t\t\tparents = parents->next;\n \t\t\tif ((this_flag & flags) == flags)\n \t\t\t\tcontinue;\n-\t\t\tparse_commit(p);\n+\t\t\tparse_commit_or_die(p);\n \t\t\tif (mark_seen(p, seen_p) && !still_interesting)\n \t\t\t\textra--;\n \t\t\tp->object.flags |= flags;\n@@ -835,7 +835,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\tif (!commit)\n \t\t\tdie(_(\"cannot find commit %s (%s)\"),\n \t\t\t    ref_name[num_rev], oid_to_hex(&revkey));\n-\t\tparse_commit(commit);\n+\t\tparse_commit_or_die(commit);\n \t\tmark_seen(commit, &seen);\n \n \t\t/* rev#0 uses bit REV_SHIFT, rev#1 uses bit REV_SHIFT+1,\ndiff --git a/commit.c b/commit.c\nindex b9c0c81..5b23eaf 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -910,7 +910,7 @@ static void mark_redundant(struct commit **array, int cnt)\n \tALLOC_ARRAY(filled_index, cnt - 1);\n \n \tfor (i = 0; i < cnt; i++)\n-\t\tparse_commit(array[i]);\n+\t\tparse_commit_or_die(array[i]);\n \tfor (i = 0; i < cnt; i++) {\n \t\tstruct commit_list *common;\n \ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex cb45c34..8b4ab47 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -159,7 +159,7 @@ static const unsigned char *get_rev(void)\n \t\t\treturn NULL;\n \n \t\tcommit = prio_queue_get(&rev_list);\n-\t\tparse_commit(commit);\n+\t\tparse_commit_or_die(commit);\n \t\tparents = commit->parents;\n \n \t\tcommit->object.flags |= POPPED;\n-- \n2.10.1.448.g862ec83.dirty\n\n"},{"id":"304493","messageId":"CAGZ79kb0gfVQt-P7b8y2KN-vDvs1FKn=D0isHkTy=QO416amDA@mail.gmail.com","threadId":"44339","inReplyTo":"xmqqoa2ehp5u.fsf@gitster.mtv.corp.google.com","subject":"Re: Fwd: New Defects reported by Coverity Scan for git","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-20T18:13:40Z","receivedAt":"2016-10-20T18:13:46Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 20, 2016 at 11:05 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Good to know that you have been managing it; I was mostly worried\n> about not having anybody managing it (i.e. imagining Coverity\n> nominated/volunteered me as manager with everybody else as viewers)\n> and the new viewer requests get totally ignored by the project as\n> the whole.\n\nNo, I have been paying attention, but in case I suddenly stop contributing\nto the git project I thought it's better to have a couple of people there.\n\n>\n>> I see that some of these emails may be inconvenient to you, I can\n>> change your role to defect viewer/contributor if you prefer.\n>\n> It is not a huge inconvenience to me, because any piece of e-mail\n> that is addressed directly to gitster@ without CC'ing to git@vger\n> and is not a follow-up to any earlier message goes to a separate\n> lowest-priority mailbox that I rarely look at.  But if it is easy\n> to recategorize me, please do so.\n\ndone.\n"},{"id":"304531","messageId":"20161020214045.gpe352xnw5oc36ur@sigill.intra.peff.net","threadId":"44339","inReplyTo":"CAGZ79kabVPhp0_z-e_4jJOFq+jzSE2SsgmFuY-2RUgrEviGKyA@mail.gmail.com","subject":"Re: Fwd: New Defects reported by Coverity Scan for git","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-20T21:40:45Z","receivedAt":"2016-10-20T21:40:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 20, 2016 at 10:05:38AM -0700, Stefan Beller wrote:\n\n> Not sure what triggered the new finding of coverity as seen below as the\n> parse_commit() was not touched. Junios series regarding the merge base\n> optimization touches a bit of code nearby though.\n\nI have noticed that \"old\" problems sometimes reappear when nearby code\nchanges. I assume that they have some kind of heuristic to identify the\nlocation of a defect, that it probably includes the line number in the\nfile, and that it can be fooled by too much code appearing nearby.\n\n-Peff\n"},{"id":"304532","messageId":"20161020214213.juoxbspa7pba5vsi@sigill.intra.peff.net","threadId":"44339","inReplyTo":"CAGZ79kYTwsWy+oxnPKghWKGLEaO9qWZbrQOOejeEBOe0c-o0Pg@mail.gmail.com","subject":"Re: Fwd: New Defects reported by Coverity Scan for git","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-20T21:42:14Z","receivedAt":"2016-10-20T21:42:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 20, 2016 at 10:58:39AM -0700, Stefan Beller wrote:\n\n> > By the way, do you know who is managing the service on our end\n> > (e.g. approving new people to be \"defect viewer\")?\n> \n> I do it most of the time, but I did not start managing it.\n> And I have been pretty lax/liberal about handing out rights to do stuff,\n> because I did not want to tip on anyone's toes giving to few rights\n> and thereby annoying them.\n\nI also do this, though I admit with more urgency when I recognize the\nname as somebody who has showed up on the git list.\n\nI wish there was a flag to simply make the results public. There is no\npoint in anybody logging in just to view them, but I don't think\nCoverity makes that an option.\n\n-Peff\n"},{"id":"304537","messageId":"xmqqzilyeksu.fsf@gitster.mtv.corp.google.com","threadId":"44339","inReplyTo":"20161020214213.juoxbspa7pba5vsi@sigill.intra.peff.net","subject":"Re: Fwd: New Defects reported by Coverity Scan for git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-20T22:07:45Z","receivedAt":"2016-10-20T22:07:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Oct 20, 2016 at 10:58:39AM -0700, Stefan Beller wrote:\n>\n>> > By the way, do you know who is managing the service on our end\n>> > (e.g. approving new people to be \"defect viewer\")?\n>> \n>> I do it most of the time, but I did not start managing it.\n>> And I have been pretty lax/liberal about handing out rights to do stuff,\n>> because I did not want to tip on anyone's toes giving to few rights\n>> and thereby annoying them.\n>\n> I also do this, though I admit with more urgency when I recognize the\n> name as somebody who has showed up on the git list.\n\nWell, then huge thanks to both of you.\n"}]}