From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12918 invoked by alias); 1 Feb 2013 14:31:39 -0000 Received: (qmail 12886 invoked by uid 22791); 1 Feb 2013 14:31:37 -0000 X-SWARE-Spam-Status: No, hits=-2.7 required=5.0 tests=AWL,BAYES_00,KHOP_SPAMHAUS_DROP,KHOP_THREADED,RCVD_IN_HOSTKARMA_YE X-Spam-Check-By: sourceware.org Received: from na3sys009aog137.obsmtp.com (HELO na3sys009aog137.obsmtp.com) (74.125.149.18) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 01 Feb 2013 14:31:30 +0000 Received: from mx20.qnx.com ([72.1.200.103]) (using TLSv1) by na3sys009aob137.postini.com ([74.125.148.12]) with SMTP ID DSNKUQvRwEFr7zM3bKVkmL1W+8Cdu3SYcHFr@postini.com; Fri, 01 Feb 2013 06:31:29 PST Received: by mx20.qnx.com (Postfix, from userid 500) id E924E21114; Fri, 1 Feb 2013 09:31:27 -0500 (EST) Received: from exhts.ott.qnx.com (exch2 [10.222.2.136]) (using TLSv1 with cipher AES128-SHA (128/128 bits)) (No client certificate requested) by mx20.qnx.com (Postfix) with ESMTPS id 87AFE2110A; Fri, 1 Feb 2013 09:31:27 -0500 (EST) Received: from [10.222.96.215] (10.222.2.5) by EXCH2.ott.qnx.com (10.222.2.136) with Microsoft SMTP Server (TLS) id 14.2.318.4; Fri, 1 Feb 2013 09:31:27 -0500 Message-ID: <510BD1BF.2050209@qnx.com> Date: Fri, 01 Feb 2013 14:31:00 -0000 From: Aleksandar Ristovski User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130106 Thunderbird/17.0.2 MIME-Version: 1.0 To: Jan Kratochvil CC: "gdb-patches@sourceware.org" Subject: Re: [patch] validate binary before use References: <50D8B37A.20001@qnx.com> <20121225073709.GA11349@host2.jankratochvil.net> <50DCAA5C.3000301@qnx.com> <20121227205924.GA5109@host2.jankratochvil.net> <50DCB787.6020601@qnx.com> <20121227211328.GA5739@host2.jankratochvil.net> <50DCBBD1.7000707@qnx.com> <5107F591.304@qnx.com> <20130130191646.GA1034@host2.jankratochvil.net> <510A7E4B.4040608@qnx.com> <20130201030610.GA12800@host2.jankratochvil.net> In-Reply-To: <20130201030610.GA12800@host2.jankratochvil.net> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2013-02/txt/msg00007.txt.bz2 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 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. 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). > 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