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

[Rich Felker <dalias@libc.org>]

On Wed, Jun 27, 2018 at 04:02:12PM -0400, Rich Felker wrote:
> 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!

I made an additional fix to avoid matching symbols without a recorded
size when there's an intervening symbol with a recorded size -- the
case like:

0x12340020 - sym_with_no_size
0x12340028 - sym_with_size_4
0x12340030 - addr

where addr should not match sym_with_no_size.

Unfortunately the fix only works conditionally depending on the order
of the symbol table. I'm preparing a fix now which I'll push soon.

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