Re: [Xen-devel] [PATCH 10/16] libelf: check all pointer accesses

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

George Dunlap writes ("Re: [Xen-devel] [PATCH 10/16] libelf: check all pointer 
accesses"):
> On Tue, Jun 4, 2013 at 6:59 PM, Ian Jackson <ian.jackson@xxxxxxxxxxxxx> wrote:
> > +{
> > +    if ( elf->dest_size >= amount )
> > +    {
> > +        elf->dest_base += amount;
> > +        elf->dest_size -= amount;
> > +    }
> > +    else
> > +    {
> > +        elf->dest_size = 0;
> > +    }
> > +}
> 
> This is probably a minor thing, but is there a reason not to set
> elf->dest_base to NULL here?

Well, there's nothing really wrong with elf->dest_base apart from that
it points to a zero-length area.

But your comment makes me realise that we should call elf_mark_broken
in this case.

> I realize that technically you've said "dest_size=0 -> dest_base is
> also invalid", but it never hurts to have a little extra safety.

If there is any code which accesses elf->dest_base[] without checking
elf->dest_size then that code is already a problem.

Making the change you propose would raise questions about whether (eg)
some other code somewhere might think dest_base==0 means something
special.  (I don't think it does, but it's an argument against
changing things.)

> Other than that, I've tried to do a careful reading, and it seems like
> it does what it says on the tin.  I have mostly looked at the code
> that *has* changed, not the code that has *not* changed, but the
> renaming of key structures and functions should have helped the
> compiler do that for us.  It's good to go in with or without the above
> suggestion as far as I can tell.

Thanks.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel