Re: [Xen-devel] [PATCH 14/16] libelf: use only unsigned integers

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

George Dunlap writes ("Re: [Xen-devel] [PATCH 14/16] libelf: use only unsigned 
integers"):
> On Tue, Jun 4, 2013 at 7:00 PM, Ian Jackson <ian.jackson@xxxxxxxxxxxxx> wrote:
> > diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.
> > -        size = sizeof(int) + elf_size(elf, elf->ehdr) +
> > +        size = sizeof(unsigned) + elf_size(elf, elf->ehdr) +
> >              elf_shdr_count(elf) * elf_size(elf, shdr);
> 
> Something about this s/sizeof(int)/sizeof(unsigned)/; bit seems a bit
> weird to me.  This is reading a standard data format, not just
> communicating between two bits of code we control, right?
> 
> Can we be absolutely certain that forevermore, sizeof(int) and
> sizeof(unsigned) are the same (and will be the same size as the
> elements of the elf binary)?  Alternately, was the old code wrong and
> the new code right?

Yes, we can be sure that sizeof(int)==sizeof(unsigned), forever and
always.  If they weren't the right size as the things in the elf then
the code was broken before :-).

I'm changing this so you can grep for \bint\b afterwards and see a
very small number of hits left.

> > -int elf_xen_parse_guest_info(struct elf_binary *elf,
> > +elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
> >                               struct elf_dom_parms *parms)
> >  {
> >      ELF_PTRVAL_CONST_CHAR h;
> > -    char name[32], value[128];
> > -    int len;
> > +    unsigned char name[32], value[128];
> > +    unsigned len;
> 
> This function has comparisons like the following:
> 
>     if ( len >= sizeof(value)-1 )
>         break;
> 
> Are we certain that, for instance, sizeof() cannot be 0?

Yes.  sizeof(value) cannot be 0 unless someone changes value[128] to
value[0].

> According to this stackoverfow article, gcc allows empty structs which
> will have a sizeof() zero:
> 
> http://stackoverflow.com/questions/2632021/can-sizeof-return-0-zero

But value is a char array of nonzero size.

> In this case of course we've hard-coded the values in there, so in
> this particular case it's OK.  Something to keep an eye out for...

Indeed.

> > -        rc = elf_xen_parse_notes(elf, parms,
> > +        more_notes = elf_xen_parse_notes(elf, parms,
> >                                   elf_segment_start(elf, phdr),
> >                                   elf_segment_end(elf, phdr));
> > -        if ( rc == -1 )
> > +        if ( more_notes == ~0U )
> >              return -1;
> 
> Rather than have this somewhat magical "~0U" value copied all over,
> would it make more sense to have a #define?  Maybe
> ELF_PARSE_NOTES_INVAL or something?

OK, I can do that.

> Other than that, I've tried to take a look at all of the changed
> variables and I think making them unsigned should be fine.

Good, thanks.

Ian.

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