From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18971 invoked by alias); 1 Feb 2013 03:06:28 -0000 Received: (qmail 18951 invoked by uid 22791); 1 Feb 2013 03:06:26 -0000 X-SWARE-Spam-Status: No, hits=-6.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 01 Feb 2013 03:06:18 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r1136FEo010717 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 31 Jan 2013 22:06:15 -0500 Received: from host2.jankratochvil.net (ovpn-116-88.ams2.redhat.com [10.36.116.88]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r1136Alh030661 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 31 Jan 2013 22:06:13 -0500 Date: Fri, 01 Feb 2013 03:06:00 -0000 From: Jan Kratochvil To: Aleksandar Ristovski Cc: "gdb-patches@sourceware.org" Subject: Re: [patch] validate binary before use Message-ID: <20130201030610.GA12800@host2.jankratochvil.net> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <510A7E4B.4040608@qnx.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes 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/msg00000.txt.bz2 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. 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. 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.) > I find that l_addr_inferior l_addr should be used, not l_addr_inferior. (Although one should not use rather either.) > 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. 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. Thanks, Jan