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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

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.c
> index 605b1e0..60000da 100644
> --- a/tools/libxc/xc_dom_elfloader.c
> +++ b/tools/libxc/xc_dom_elfloader.c
> @@ -82,7 +82,7 @@ static char *xc_dom_guest_type(struct xc_dom_image *dom,
>  /* ------------------------------------------------------------------------ 
> */
>  /* parse elf binary                                                         
> */
>
> -static int check_elf_kernel(struct xc_dom_image *dom, bool verbose)
> +static elf_negerrnoval check_elf_kernel(struct xc_dom_image *dom, bool 
> verbose)
>  {
>      if ( dom->kernel_blob == NULL )
>      {
> @@ -104,12 +104,12 @@ static int check_elf_kernel(struct xc_dom_image *dom, 
> bool verbose)
>      return 0;
>  }
>
> -static int xc_dom_probe_elf_kernel(struct xc_dom_image *dom)
> +static elf_negerrnoval xc_dom_probe_elf_kernel(struct xc_dom_image *dom)
>  {
>      return check_elf_kernel(dom, 0);
>  }
>
> -static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
> +static elf_errorstatus xc_dom_load_elf_symtab(struct xc_dom_image *dom,
>                                    struct elf_binary *elf, bool load)
>  {
>      struct elf_binary syms;
> @@ -117,7 +117,7 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image 
> *dom,
>      xen_vaddr_t symtab, maxaddr;
>      ELF_PTRVAL_CHAR hdr;
>      size_t size;
> -    int h, count, type, i, tables = 0;
> +    unsigned h, count, type, i, tables = 0;
>
>      if ( elf_swap(elf) )
>      {
> @@ -139,13 +139,13 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image 
> *dom,
>          elf->caller_xdest_size = page_size -
>              (dom->bsd_symtab_start & (page_size-1));
>          hdr = ELF_REALPTR2PTRVAL(hdr_ptr);
> -        elf_store_val(elf, int, hdr, size - sizeof(int));
> +        elf_store_val(elf, unsigned, hdr, size - sizeof(unsigned));
>      }
>      else
>      {
>          char *hdr_ptr;
>
> -        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?

(This goes for all such changes in this file)

> diff --git a/xen/common/libelf/libelf-dominfo.c 
> b/xen/common/libelf/libelf-dominfo.c
> index c4ced67..a9a5f41 100644
> --- a/xen/common/libelf/libelf-dominfo.c
> +++ b/xen/common/libelf/libelf-dominfo.c
[snip]
> @@ -246,12 +246,12 @@ static int elf_xen_parse_notes(struct elf_binary *elf,
>  /* ------------------------------------------------------------------------ 
> */
>  /* __xen_guest section                                                      
> */
>
> -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?

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

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...


> @@ -495,13 +495,13 @@ int elf_xen_parse(struct elf_binary *elf,
>          if (elf_uval(elf, phdr, p_offset) == 0)
>               continue;
>
> -        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?

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.

 -George

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