Re: [patch] validate binary before use

[Aleksandar Ristovski <aristovski at qnx dot com>]

Thanks for the review. Please see some of my comments inlined.

On 13-01-30 02:16 PM, Jan Kratochvil wrote:
> On Tue, 29 Jan 2013 17:15:13 +0100, Aleksandar Ristovski wrote:
> > +	      /* Section vma is unrelocated.  If SO_BASE_ADDR is zero, then
> > +	         use ASECT->VMA as-is.  If not, then use offset + base addr.  */
> > +	      res = target_verify_memory (data, (so_base_addr > 0)?
>
> I do not see why to use target_verify_memory in this case.

While your comment below is correct, I find, since we introduced 
target_verify_memory already, this to be "more correct". Well, it is 
equivalent to what you are suggesting and I was considering doing simply 
read_memory/memcmp here, but then figured, target_verify_memory is more 
semantically correct.

> target_verify_memory is there for large sections to compare only their 32-bit
> checksum.  But build-id is already only 20 bytes long, with the protocol
> overhead the 4 vs. 20 bytes do not make a difference.  And it needlessly
> weakens the check, it also does some patching of target_verify_memory.
> Just use target_read_memory and memcmp.

This is all true; however my opinion is that fallback in 
target_verify_memory is correct implementation as it allows using 
target_verify_memory where semantically suitable (like this place IMO) 
regardless of whether actual target implements it or not (e.g. core 
doesn't need to implement it; if it did, the implementation would 
probably be exactly the same as the fallback).

> > +					   so_base_addr + sect_vma_offset
> > +					   : asect->vma,
> > +					  size);
>
> so->abfd is not properly relocated (not sure why but it is so) but you can

I find that l_addr_inferior remains to be 0 if prelinked object was not 
relocated. I haven't looked at gnu ld, but I would expect it's missing 
to set it correctly (this is misfortunate).

> iterate so->sections..so->sections_end which contains relocated ADDR (=target
> VMA).  Then you can drop the svr4_unrelocated_vma and other calculations
> around.

Ok. I think this is what I tried first but then some testcases would 
fail. Will revisit.


I will be posting new patch with other comments addressed, and please 
provide feedback on the above.


Thanks,

Aleksandar