* New NULL dereference in sequence.c
@ 2012-05-02 8:45 Michael Mueller
2012-05-02 12:53 ` René Scharfe
0 siblings, 1 reply; 5+ messages in thread
From: Michael Mueller @ 2012-05-02 8:45 UTC (permalink / raw)
To: git
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/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: New NULL dereference in sequence.c
2012-05-02 8:45 New NULL dereference in sequence.c Michael Mueller
@ 2012-05-02 12:53 ` René Scharfe
2012-05-02 17:34 ` Neil Horman
0 siblings, 1 reply; 5+ messages in thread
From: René Scharfe @ 2012-05-02 12:53 UTC (permalink / raw)
To: Michael Mueller; +Cc: git, Neil Horman
[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é
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: New NULL dereference in sequence.c
2012-05-02 12:53 ` René Scharfe
@ 2012-05-02 17:34 ` Neil Horman
2012-05-02 17:39 ` Matthieu Moy
0 siblings, 1 reply; 5+ messages in thread
From: Neil Horman @ 2012-05-02 17:34 UTC (permalink / raw)
To: René Scharfe; +Cc: Michael Mueller, git
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: New NULL dereference in sequence.c
2012-05-02 17:34 ` Neil Horman
@ 2012-05-02 17:39 ` Matthieu Moy
2012-05-02 19:38 ` Neil Horman
0 siblings, 1 reply; 5+ messages in thread
From: Matthieu Moy @ 2012-05-02 17:39 UTC (permalink / raw)
To: Neil Horman; +Cc: René Scharfe, Michael Mueller, git
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/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: New NULL dereference in sequence.c
2012-05-02 17:39 ` Matthieu Moy
@ 2012-05-02 19:38 ` Neil Horman
0 siblings, 0 replies; 5+ messages in thread
From: Neil Horman @ 2012-05-02 19:38 UTC (permalink / raw)
To: Matthieu Moy; +Cc: René Scharfe, Michael Mueller, git
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/
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-05-02 19:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-02 8:45 New NULL dereference in sequence.c Michael Mueller
2012-05-02 12:53 ` René Scharfe
2012-05-02 17:34 ` Neil Horman
2012-05-02 17:39 ` Matthieu Moy
2012-05-02 19:38 ` Neil Horman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.