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

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

[Siebenborn, Axel]

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.

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.

Regards,
Axel

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)
                                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
+       }
+
+       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;

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

[Siebenborn, Axel]

Hi,
I just wanted to ask, if there something wrong with that patch, as there was no reaction.

Currently, I am fixing tests in the OpenJDK portola project (http://openjdk.java.net/projects/portola/).
Some tests fail, since we don't get correct stack traces due to the dladdr problem.

Regards,
Axel

> -----Original Message-----
> From: Siebenborn, Axel [mailto:axel.siebenborn@sap.com]
> Sent: Dienstag, 3. April 2018 15:06
> To: musl@lists.openwall.com
> Subject: [CAUTION] [musl] [PATCH] dl_addr: compare addr with sym-
> >st_size.
> 
> 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.
> 
> 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.
> 
> Regards,
> Axel
> 
> 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)
>                                 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
> +       }
> +
> +       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;

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

[Siebenborn, Axel]

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

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

[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.

> > > @@ -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

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

[Siebenborn, Axel]

> -----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

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

[Siebenborn, Axel]

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.

Thanks,
Axel 

> -----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

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

[Rich Felker]

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.

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

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

[William Pitcock]

Hi,

On Wed, May 16, 2018 at 6:16 PM, Rich Felker <dalias@libc.org> 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.

If you are testing against Alpine (as I understand it the Portola team
at Oracle is solely concentrating on Alpine), we could incorporate the
patch into the next musl package we publish in the meantime.
But I would like to make sure Rich says it is OK first.

William

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

[Rich Felker]

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

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

[Rich Felker]

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