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

["Siebenborn, Axel" <axel.siebenborn@sap.com>]

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)

> 
> > According to the spec, dl_addr should not return an error in this
> > case. Instead dli_sname and dli_addr should be set to NULL.
> 
> OK, I found that part in the man page.
> 
> > diff --git a/ldso/dynlink.c b/ldso/dynlink.c
> > index 9bf6924..cc87dc0 100644
> > --- a/ldso/dynlink.c
> > +++ b/ldso/dynlink.c
> > @@ -1958,7 +1958,7 @@ int dladdr(const void *addr, Dl_info *info)
> >                  && (1<<(sym->st_info&0xf) & OK_TYPES)
> >                  && (1<<(sym->st_info>>4) & OK_BINDS)) {
> >                         void *symaddr = laddr(p, sym->st_value);
> > -                       if (symaddr > addr || symaddr < best)
> > +                       if (symaddr > addr || (void*) ((uint8_t*) symaddr + sym-
> >st_size) < addr || symaddr < best)
> 
> Not all symbols have st_size set. In that case the old "best match"
> behavior should probably be kept unless there's a strong reason not
> to.
> 
> >                                 continue;
> >                         best = symaddr;
> >                         bestsym = sym;
> > @@ -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;

> 
> > +       }
> > +
> > +       if ( DL_FDPIC && (bestsym->st_info&0xf) == STT_FUNC)
> > +               best = p->funcdescs + (bestsym - p->syms);
> >         info->dli_sname = strings + bestsym->st_name;
> >         info->dli_saddr = best;
> 
> Otherwise this looks ok but I haven't tested it.
> 
> Rich

Here is the complete patch:

diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index 9bf6924..2801416 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -1958,7 +1958,7 @@ int dladdr(const void *addr, Dl_info *info)
                 && (1<<(sym->st_info&0xf) & OK_TYPES)
                 && (1<<(sym->st_info>>4) & OK_BINDS)) {
                        void *symaddr = laddr(p, sym->st_value);
-                       if (symaddr > addr || symaddr < best)
+                       if (symaddr > addr || ((sym->st_size != 0) && ((void*) ((uint8_t*) symaddr + sym->st_size) < addr)) || symaddr < best)
                                continue;
                        best = symaddr;
                        bestsym = sym;
@@ -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 1;
+       }
+
+       if ( DL_FDPIC && (bestsym->st_info&0xf) == STT_FUNC)
+               best = p->funcdescs + (bestsym - p->syms);
        info->dli_sname = strings + bestsym->st_name;
        info->dli_saddr = best;

Regards,
Axel