From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/12712 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] dl_addr: compare addr with sym->st_size. Date: Thu, 12 Apr 2018 21:01:52 -0400 Message-ID: <20180413010152.GS3094@brightrain.aerifal.cx> References: <6a42ca4b6c9b4ea08925e232d7b57667@sap.com> <20180410142312.GE3094@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: blaine.gmane.org 1523581203 8778 195.159.176.226 (13 Apr 2018 01:00:03 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 13 Apr 2018 01:00:03 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-12728-gllmg-musl=m.gmane.org@lists.openwall.com Fri Apr 13 02:59:59 2018 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1f6n4H-00027B-9t for gllmg-musl@m.gmane.org; Fri, 13 Apr 2018 02:59:57 +0200 Original-Received: (qmail 32130 invoked by uid 550); 13 Apr 2018 01:02:04 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 32109 invoked from network); 13 Apr 2018 01:02:04 -0000 Content-Disposition: inline In-Reply-To: Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:12712 Archived-At: On Wed, Apr 11, 2018 at 08:07:38AM +0000, Siebenborn, Axel wrote: > Hi, > > -----Original Message----- > > From: Rich Felker [mailto:dalias@aerifal.cx] On Behalf Of Rich Felker > > Sent: Dienstag, 10. April 2018 16:23 > > To: musl@lists.openwall.com > > Subject: Re: [musl] [PATCH] dl_addr: compare addr with sym->st_size. > > > > On Tue, Apr 03, 2018 at 01:06:09PM +0000, Siebenborn, Axel wrote: > > > Hi, > > > this patch fixes a problem with dl_addr. > > > > > > We found symbols, in cases we should not find a symbol, since the > > > comparison with sym->st_size is missing. > > > > This was intentional, as my understanding of the historical behavior > > on other implementations was that it would do this. If that's > > incorrect we should investigate and document (or find existing > > documentation of) what they really do. > I don't know how the historical behavior was. Maybe you could point me to > some resources. > However, I found that st_size might be 0, if the symbol has no or an unknown > size. How about comparing st_size to zero? > > - if (symaddr > addr || symaddr < best) > + if (symaddr > addr || ((sym->st_size != 0) && ((void*) ((uint8_t*) symaddr + sym->st_size) < addr)) || symaddr < best) I think this should be <= not <. symaddr+sym->st_size is one past the end of the object/function, not part of it. Aside from that, a couple style issues. This line is very long (well over 80) after the change, and in musl we generally don't use !=0 or excessive parens. Changing those things would help the length too. Should also be char * rather than uint8_t*. With these changes I think it looks like: if (symaddr > addr || (sym->st_size && ((void*)((char *)symaddr + sym->st_size) < addr)) || symaddr < best) which is still really long. We could eliminate all the cast mess by changing the addresses all to uintptr_t, which really should be done (as a separate patch) anyway, since relational operators on pointers that don't point into the same arrays is UB. But it still leaves the line well over 80 chars. If may be best to write it as: if (symaddr > addr || symaddr < best || (sym->st_size && symaddr+sym->st_size < addr)) continue; or even (simple patch): if (symaddr > addr || symaddr < best) continue; + if (sym->st_size && symaddr+sym->st_size < addr) + continue; I can handle the independent UB fix and reformatting if you like. > > > @@ -1967,13 +1967,16 @@ int dladdr(const void *addr, Dl_info *info) > > > } > > > } > > > > > > - if (!best) return 0; > > > - > > > - if (DL_FDPIC && (bestsym->st_info&0xf) == STT_FUNC) > > > - best = p->funcdescs + (bestsym - p->syms); > > > - > > > info->dli_fname = p->name; > > > info->dli_fbase = p->map; > > > + if (!best) { > > > + info->dli_sname = 0; > > > + info->dli_saddr = 0; > > > + return 0 > > > > This is missing a ; so it seems you tested a slightly different patch..? > Sorry, that's embarrassing. I slightly refactored after testing. > This line should be: > + return 1; OK, that looks right. Rich