This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] validate binary before use


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]