Re: [patch] validate binary before use

[Aleksandar Ristovski <aristovski@qnx.com>]

On 13-01-31 10:06 PM, Jan Kratochvil wrote:
> On Thu, 31 Jan 2013 15:23:07 +0100, Aleksandar Ristovski wrote:
>> 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).
>
> This is a bit nitpicking, primarily I do not see there much difference and we
> avoid dealing with target_verify_memory in this patch.
>
> target_read_memory is already always implemented by the target.
>
> With gdbserver <library-list-svr4/> protocol the build-id itself seems to me
> to be the natural way how to identify the library.  While it could also send
> a 32-bit CRC in the XML protocol the build-id looks as more universal even for
> other possible GDB extensions in the future.

My primary reason for 'verify' is not optimization but the semantics. 
But I see no point in insisting on it, I will use read_memory/memcmp.

>
> And thus optimizing local solib-svr4.c usage by 16 bytes saved by the 32-bit
> CRC seems off-topic to me, (1) it will work needlessly differently in both
> cases (vs. gdbserver) and (2) non-gdbserver usage is going to be deprecated so
> this is just a temporary code anyway.

No, the code is not going to be deprecated as long as core target is in 
the gdb and not implemented as e.g. external agent talking to gdb via 
remote protocol.

>
> Besides that target_verify_memory fallback should be put into
> default_verify_memory function and installed in to_verify_memory in all
> targets except that remote.c remote_verify_memory.  target_* functions should
> be just dispatchers, not the implementations.  (Yes, C++ would be easier.)

Sure - I will drop this part.

>
>
>> I find that l_addr_inferior
>
> l_addr should be used, not l_addr_inferior.  (Although one should not use
> rather either.)

In this case, in the approach you commented on, l_addr_inferior was 
appropriate as I wanted to "relocate" it directly (this is why I 
calculated offset from the base) as opposed to applying calculated 
offset l_addr. But it may not be applicable any more if asection->addr 
proves to be reliable.

>
>
>> 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).
>
> This behavior is correct.  Changing it would break all the tools around.

<offtopic>
If you say so. IMO it is less than ideal, it should specify l_addr as 
expected and make prelink transparent. Like this, it special cases 
meaning of this field, which is common and across many systems happens 
to always have the same meaning (except when 'successful' prelinking 
happens).
</offtopic>

> l_addr just has wrong comment in glibc.  I have pinged now the fix:
> 	[patch] Fix l_addr comment
> 	http://sourceware.org/ml/libc-alpha/2011-09/msg00151.html
>
>
>>> 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.
>
> There may be other issues but I am not aware of those.

I will be posting new patch soon.

---
Aleksandar