From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/12828 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: Wed, 16 May 2018 19:16:43 -0400 Message-ID: <20180516231643.GA1392@brightrain.aerifal.cx> References: <6a42ca4b6c9b4ea08925e232d7b57667@sap.com> <20180410142312.GE3094@brightrain.aerifal.cx> <20180413010152.GS3094@brightrain.aerifal.cx> <3e3e78b2fbbb49c2952434473756b994@sap.com> 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 1526512492 4995 195.159.176.226 (16 May 2018 23:14:52 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Wed, 16 May 2018 23:14:52 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-12844-gllmg-musl=m.gmane.org@lists.openwall.com Thu May 17 01:14:48 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 1fJ5d9-0001D2-Ud for gllmg-musl@m.gmane.org; Thu, 17 May 2018 01:14:48 +0200 Original-Received: (qmail 8162 invoked by uid 550); 16 May 2018 23:16:56 -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 8144 invoked from network); 16 May 2018 23:16:55 -0000 Content-Disposition: inline In-Reply-To: <3e3e78b2fbbb49c2952434473756b994@sap.com> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:12828 Archived-At: On Wed, May 16, 2018 at 08:16:57AM +0000, Siebenborn, Axel wrote: > Hi Rich, > > I wonder, when this patch will make it into the repository and when there is a released version with that patch. > > I'm not familiar with the release strategy of musl. > I'm not sure if I misunderstood something and I have to do something in order to get this patch in. Sorry, I've just been busy with other things. Thanks for reminding me to get back to this. Rich > > -----Original Message----- > > From: Siebenborn, Axel [mailto:axel.siebenborn@sap.com] > > Sent: Freitag, 13. April 2018 12:17 > > To: musl@lists.openwall.com > > Subject: [CAUTION] RE: [musl] [PATCH] dl_addr: compare addr with sym- > > >st_size. > > > > > -----Original Message----- > > > From: Rich Felker [mailto:dalias@aerifal.cx] On Behalf Of Rich Felker > > > 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. > > > > Thanks, that would be nice! > > > > > > > > > > > @@ -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 > > Thanks for looking at this and for considering the patch! > > Axel