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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.06.13 at 20:14, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote:
> 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.

But there's a point in what George says nevertheless: What is this
sizeof(int) (now sizeof(unsigned)) referring to anyway? Going
through the whole function at the end of the patch series, I had a
pretty hard time finding that this is being used in the if() portion of
the else body that gets modified here. Noticing right away that
there's possibly truncation here - the field ought to be size_t. And
that would make it desirable to switch to sizeof(size), making clear
which item it is that the size is needed of. Yet of course with this
kind of coding done originally, finding _all_ consumers of this is
going to be non-trivial - this should have been some sort of
structure to begin with. And there doesn't seem to be a load
equivalent of elf_store_val() (and also elf_store_field()) - am I
overlooking something? What's the point of storing things that
never get read?

There appears to be a similar problem in elf_load_bsdsyms(),
using uint32_t instead of elf_ptrval.

Jan


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