New NULL dereference in sequence.c

> Hi all,
>
> The last defect Sentry picked up has been removed, yay!  At the same
> time, a new NULL dereference bug appeared, in sequencer.c:
>
>      static int is_index_unchanged(void)
>      {
>          unsigned char head_sha1[20];
>          struct commit *head_commit;
>
>          if (!resolve_ref_unsafe("HEAD", head_sha1, 1, NULL))
>              return error(_("Could not resolve HEAD commit\n"));
>
>          head_commit = lookup_commit(head_sha1);
>          if (!head_commit || parse_commit(head_commit))
>              return error(_("could not parse commit %s\n"),
>                       sha1_to_hex(head_commit->object.sha1));
>
> In the last line quoted above, head_commit may be NULL, and it is
> dereferenced in the call to sha1_to_hex.  Assuming lookup_commit(head_sha1)
> can return NULL, this function will segfault.
>
> Introduced here:
> https://github.com/gitster/git/commit/b27cfb0#sequencer.c

> [cc:ing the author of this commit]
> 
> Am 02.05.2012 10:45, schrieb Michael Mueller:
> >Hi all,
> >
> >The last defect Sentry picked up has been removed, yay!  At the same
> >time, a new NULL dereference bug appeared, in sequencer.c:
> >
> >     static int is_index_unchanged(void)
> >     {
> >         unsigned char head_sha1[20];
> >         struct commit *head_commit;
> >
> >         if (!resolve_ref_unsafe("HEAD", head_sha1, 1, NULL))
> >             return error(_("Could not resolve HEAD commit\n"));
> >
> >         head_commit = lookup_commit(head_sha1);
> >         if (!head_commit || parse_commit(head_commit))
> >             return error(_("could not parse commit %s\n"),
> >                      sha1_to_hex(head_commit->object.sha1));
> >
> >In the last line quoted above, head_commit may be NULL, and it is
> >dereferenced in the call to sha1_to_hex.  Assuming lookup_commit(head_sha1)
> >can return NULL, this function will segfault.
> >
> >Introduced here:
> >https://github.com/gitster/git/commit/b27cfb0#sequencer.c
> 
> Similar code in builtin/commit.c just reports "could not parse HEAD
> commit" without any hash and thus no pointer dereference.
> 
> René
> 

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> >> >         if (!head_commit || parse_commit(head_commit))
> >> >             return error(_("could not parse commit %s\n"),
> >> >                      sha1_to_hex(head_commit->object.sha1));
> 
> [...]
> 
> > Have you actually seen this problem occur? It seems to me that the
> > only way head_commit could be NULL is in the event that HEAD wasn't a
> > commit object, whcih I don't think can be the case unless something
> > else is very wrong with your tree.
> 
> I don't know whether it can actually happen, but if it can't happen,
> then the if() condition should be rewritten. As it is, it explicitly
> allows head_commit to be NULL within the if body.
>