New NULL dereference in sequence.c

New NULL dereference in sequence.c

[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

Mike

-- 
Mike Mueller
Phone: (401) 405-1525
Email: mmueller@vigilantsw.com

http://www.vigilantsw.com/

Re: New NULL dereference in sequence.c

[René Scharfe]

[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é

Re: New NULL dereference in sequence.c

[Neil Horman]

On Wed, May 02, 2012 at 02:53:22PM +0200, René Scharfe wrote:
> [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é
> 

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.

Neil

Re: New NULL dereference in sequence.c

[Matthieu Moy]

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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: New NULL dereference in sequence.c

[Neil Horman]

On Wed, May 02, 2012 at 07:39:19PM +0200, Matthieu Moy wrote:
> 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.
> 
You're right, it should be reduced to just if (parse_commit(head_commit)) {} and
we should call error with a different string dependent on weather head_commit is
null or not.

I'll make a patch shortly.
Neil

> -- 
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
>