Re: [patch] validate binary before use

Aleksandar Ristovski
2013-01-31T14:23:17Z

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