From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/12956 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, 27 Jun 2018 16:02:12 -0400 Message-ID: <20180627200212.GE1392@brightrain.aerifal.cx> References: <6a42ca4b6c9b4ea08925e232d7b57667@sap.com> <20180410142312.GE3094@brightrain.aerifal.cx> <20180413010152.GS3094@brightrain.aerifal.cx> <3e3e78b2fbbb49c2952434473756b994@sap.com> <20180516231643.GA1392@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 1530129624 32376 195.159.176.226 (27 Jun 2018 20:00:24 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Wed, 27 Jun 2018 20:00:24 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-12972-gllmg-musl=m.gmane.org@lists.openwall.com Wed Jun 27 22:00:20 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 1fYGc0-0008Ku-36 for gllmg-musl@m.gmane.org; Wed, 27 Jun 2018 22:00:20 +0200 Original-Received: (qmail 5306 invoked by uid 550); 27 Jun 2018 20:02:26 -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 5276 invoked from network); 27 Jun 2018 20:02:25 -0000 Content-Disposition: inline In-Reply-To: <20180516231643.GA1392@brightrain.aerifal.cx> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:12956 Archived-At: On Wed, May 16, 2018 at 07:16:43PM -0400, Rich Felker wrote: > 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. I'm finally doing this now, and will push the fix soon. Thanks for your work on this and sorry it took me so long to get back to it! 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