Re: [PATCH] dl_addr: compare addr with sym->st_size.

[Rich Felker <dalias@libc.org>]

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