{"thread":{"id":"30391","subject":"New NULL dereference in sequence.c","startedAt":"2012-05-02T08:45:54Z","lastAt":"2012-05-02T19:38:19Z","messageCount":5,"participants":["Michael Mueller","René Scharfe","Neil Horman","Matthieu Moy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"190529","messageId":"CANV9Rr9PFR7SA_3jB-6PRF_V+4yB-FsYEuX5gs-LMpoTAxGDCQ@mail.gmail.com","threadId":"30391","inReplyTo":null,"subject":"New NULL dereference in sequence.c","fromName":"Michael Mueller","fromEmail":"mmueller@vigilantsw.com","sentAt":"2012-05-02T08:45:54Z","receivedAt":"2012-05-02T08:45:54Z","isPatch":false,"sender":{"key":"mmueller@vigilantsw.com","avatar":null},"body":"Hi all,\n\nThe last defect Sentry picked up has been removed, yay!  At the same\ntime, a new NULL dereference bug appeared, in sequencer.c:\n\n    static int is_index_unchanged(void)\n    {\n        unsigned char head_sha1[20];\n        struct commit *head_commit;\n\n        if (!resolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL))\n            return error(_(\"Could not resolve HEAD commit\\n\"));\n\n        head_commit = lookup_commit(head_sha1);\n        if (!head_commit || parse_commit(head_commit))\n            return error(_(\"could not parse commit %s\\n\"),\n                     sha1_to_hex(head_commit->object.sha1));\n\nIn the last line quoted above, head_commit may be NULL, and it is\ndereferenced in the call to sha1_to_hex.  Assuming lookup_commit(head_sha1)\ncan return NULL, this function will segfault.\n\nIntroduced here:\nhttps://github.com/gitster/git/commit/b27cfb0#sequencer.c\n\nMike\n\n-- \nMike Mueller\nPhone: (401) 405-1525\nEmail: mmueller@vigilantsw.com\n\nhttp://www.vigilantsw.com/\n"},{"id":"190537","messageId":"4FA12E42.201@lsrfire.ath.cx","threadId":"30391","inReplyTo":"CANV9Rr9PFR7SA_3jB-6PRF_V+4yB-FsYEuX5gs-LMpoTAxGDCQ@mail.gmail.com","subject":"Re: New NULL dereference in sequence.c","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-05-02T12:53:22Z","receivedAt":"2012-05-02T12:53:22Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"[cc:ing the author of this commit]\n\nAm 02.05.2012 10:45, schrieb Michael Mueller:\n> Hi all,\n>\n> The last defect Sentry picked up has been removed, yay!  At the same\n> time, a new NULL dereference bug appeared, in sequencer.c:\n>\n>      static int is_index_unchanged(void)\n>      {\n>          unsigned char head_sha1[20];\n>          struct commit *head_commit;\n>\n>          if (!resolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL))\n>              return error(_(\"Could not resolve HEAD commit\\n\"));\n>\n>          head_commit = lookup_commit(head_sha1);\n>          if (!head_commit || parse_commit(head_commit))\n>              return error(_(\"could not parse commit %s\\n\"),\n>                       sha1_to_hex(head_commit->object.sha1));\n>\n> In the last line quoted above, head_commit may be NULL, and it is\n> dereferenced in the call to sha1_to_hex.  Assuming lookup_commit(head_sha1)\n> can return NULL, this function will segfault.\n>\n> Introduced here:\n> https://github.com/gitster/git/commit/b27cfb0#sequencer.c\n\nSimilar code in builtin/commit.c just reports \"could not parse HEAD \ncommit\" without any hash and thus no pointer dereference.\n\nRené\n"},{"id":"190581","messageId":"20120502173436.GA26732@hmsreliant.think-freely.org","threadId":"30391","inReplyTo":"4FA12E42.201@lsrfire.ath.cx","subject":"Re: New NULL dereference in sequence.c","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2012-05-02T17:34:36Z","receivedAt":"2012-05-02T17:34:36Z","isPatch":false,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"On Wed, May 02, 2012 at 02:53:22PM +0200, René Scharfe wrote:\n> [cc:ing the author of this commit]\n> \n> Am 02.05.2012 10:45, schrieb Michael Mueller:\n> >Hi all,\n> >\n> >The last defect Sentry picked up has been removed, yay!  At the same\n> >time, a new NULL dereference bug appeared, in sequencer.c:\n> >\n> >     static int is_index_unchanged(void)\n> >     {\n> >         unsigned char head_sha1[20];\n> >         struct commit *head_commit;\n> >\n> >         if (!resolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL))\n> >             return error(_(\"Could not resolve HEAD commit\\n\"));\n> >\n> >         head_commit = lookup_commit(head_sha1);\n> >         if (!head_commit || parse_commit(head_commit))\n> >             return error(_(\"could not parse commit %s\\n\"),\n> >                      sha1_to_hex(head_commit->object.sha1));\n> >\n> >In the last line quoted above, head_commit may be NULL, and it is\n> >dereferenced in the call to sha1_to_hex.  Assuming lookup_commit(head_sha1)\n> >can return NULL, this function will segfault.\n> >\n> >Introduced here:\n> >https://github.com/gitster/git/commit/b27cfb0#sequencer.c\n> \n> Similar code in builtin/commit.c just reports \"could not parse HEAD\n> commit\" without any hash and thus no pointer dereference.\n> \n> René\n> \n\nHave you actually seen this problem occur? It seems to me that the only way\nhead_commit could be NULL is in the event that HEAD wasn't a commit object,\nwhcih I don't think can be the case unless something else is very wrong with\nyour tree.\n\nNeil\n"},{"id":"190582","messageId":"vpqd36mcwjc.fsf@bauges.imag.fr","threadId":"30391","inReplyTo":"20120502173436.GA26732@hmsreliant.think-freely.org","subject":"Re: New NULL dereference in sequence.c","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2012-05-02T17:39:19Z","receivedAt":"2012-05-02T17:39:19Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Neil Horman <nhorman@tuxdriver.com> writes:\n\n>> >         if (!head_commit || parse_commit(head_commit))\n>> >             return error(_(\"could not parse commit %s\\n\"),\n>> >                      sha1_to_hex(head_commit->object.sha1));\n\n[...]\n\n> Have you actually seen this problem occur? It seems to me that the\n> only way head_commit could be NULL is in the event that HEAD wasn't a\n> commit object, whcih I don't think can be the case unless something\n> else is very wrong with your tree.\n\nI don't know whether it can actually happen, but if it can't happen,\nthen the if() condition should be rewritten. As it is, it explicitly\nallows head_commit to be NULL within the if body.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"190589","messageId":"20120502193819.GB26732@hmsreliant.think-freely.org","threadId":"30391","inReplyTo":"vpqd36mcwjc.fsf@bauges.imag.fr","subject":"Re: New NULL dereference in sequence.c","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2012-05-02T19:38:19Z","receivedAt":"2012-05-02T19:38:19Z","isPatch":false,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"On Wed, May 02, 2012 at 07:39:19PM +0200, Matthieu Moy wrote:\n> Neil Horman <nhorman@tuxdriver.com> writes:\n> \n> >> >         if (!head_commit || parse_commit(head_commit))\n> >> >             return error(_(\"could not parse commit %s\\n\"),\n> >> >                      sha1_to_hex(head_commit->object.sha1));\n> \n> [...]\n> \n> > Have you actually seen this problem occur? It seems to me that the\n> > only way head_commit could be NULL is in the event that HEAD wasn't a\n> > commit object, whcih I don't think can be the case unless something\n> > else is very wrong with your tree.\n> \n> I don't know whether it can actually happen, but if it can't happen,\n> then the if() condition should be rewritten. As it is, it explicitly\n> allows head_commit to be NULL within the if body.\n> \nYou're right, it should be reduced to just if (parse_commit(head_commit)) {} and\nwe should call error with a different string dependent on weather head_commit is\nnull or not.\n\nI'll make a patch shortly.\nNeil\n\n> -- \n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n> \n"}]}