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

[Rich Felker <dalias@libc.org>]

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