Re: [Xen-devel] [PATCH 4/4] libelf: treat phdr and shdr similarly

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Dec 06, 2016 at 06:37:30AM -0700, Jan Beulich wrote:
> >>> On 06.12.16 at 14:27, <konrad.wilk@xxxxxxxxxx> wrote:
> >>  unsigned elf_shdr_count(struct elf_binary *elf)
> >>  {
> >>      unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
> >> -    uint64_t max = elf->size / sizeof(Elf32_Shdr);
> >> +    uint64_t max;
> >>  
> >> +    if ( !count )
> >> +        return 0;
> >> +    max = elf->size / elf_uval(elf, elf->ehdr, e_shentsize);
> > 
> > Seems incorrect. The elf->size is the size of the image - including
> > the ELF and the data it contains.
> > 
> > But I presume the check is rather to make sure that if there
> > is no data, just an ELF sections  - that we don't roll over it.
> > 
> > In which case perhaps adding a comment saying:
> > 
> > /* If file has nothing but ELF this will catch us from rolling over the 
> > end.*/
> > 
> > or such?
> 
> Well - there was no such comment before on the equivalent (but
> slightly wrong) check which is being replaced here. If you really
> think such a comment should be added now, I can certainly (albeit
> a little reluctantly) do so - let me know.

It is up to you. I was thinking more in terms of somebody reading this
code and not realizing what we are checking for.

But maybe it is just me not having yet my coffee and other folks
looking at this would spot this right away.

> 
> Jan
> 

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