Re: [PATCH] revision.c: fix possible null pointer access

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Dec 3, 2015 at 9:06 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Stefan Naewe <stefan.naewe@xxxxxxxxx> writes:
>
> > Two functions dereference a tree pointer before checking
>
> Reading them a bit carefully, a reader would notice that they
> actually do not dereference the pointer at all.  It just computes
> another pointer and that is done by adding the offset of object
> member in the tree struct.
>
> > if the pointer is valid. Fix that by doing the check first.
> >
> > Signed-off-by: Stefan Naewe <stefan.naewe@xxxxxxxxx>
> > ---
> > This has been reported through the CppHints newsletter (http://cpphints.com/hints/40)
> > but doesn't seem to have made its way to the ones who care (the git list
> > that is...)
>
> Nobody would be surprised, unless the newsletter was sent to this
> list, which I do not think it was (but if it was sent while I was
> away, then it is very possible that I didn't see it).
>
> >  revision.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/revision.c b/revision.c
> > index 0fbb684..bb40179 100644
> > --- a/revision.c
> > +++ b/revision.c
> > @@ -104,7 +104,12 @@ static void mark_tree_contents_uninteresting(struct tree *tree)
> >  {
> >       struct tree_desc desc;
> >       struct name_entry entry;
> > -     struct object *obj = &tree->object;
> > +     struct object *obj;
> > +
> > +     if (!tree)
> > +             return;
> > +
> > +     obj = &tree->object;
>
> This is questionable; if you check all the callers of this function
> (there are two of them, I think), you would notice that they both
> know that tree cannot be NULL here.

OK.

>
> >
> >       if (!has_sha1_file(obj->sha1))
> >               return;
> > @@ -135,10 +140,13 @@ static void mark_tree_contents_uninteresting(struct tree *tree)
> >
> >  void mark_tree_uninteresting(struct tree *tree)
> >  {
> > -     struct object *obj = &tree->object;
> > +     struct object *obj;
> >
> >       if (!tree)
> >               return;
> > +
> > +     obj = &tree->object;
> > +
> >       if (obj->flags & UNINTERESTING)
> >               return;
>
> This one is not wrong per-se, but an unnecessary change, because no
> deferencing is involved.

But 'tree->object' is dereferencing tree, isn't it ? Like '(*tree).object'.

??

> At least, please lose the blank line after
> the new assignment.

Will do, if you want this patch at all.

> >       obj->flags |= UNINTERESTING;
>
> Thanks.

Thanks,
  Stefan
-- 
----------------------------------------------------------------
python -c "print '73746566616e2e6e6165776540676d61696c2e636f6d'.decode('hex')"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]