[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f3210267e30a45749b4645c4d661f149@sap.com>
Date: Fri, 13 Apr 2018 10:16:52 +0000
From: "Siebenborn, Axel" <axel.siebenborn@....com>
To: "musl@...ts.openwall.com" <musl@...ts.openwall.com>
Subject: RE: [PATCH] dl_addr: compare addr with sym->st_size.
> -----Original Message-----
> From: Rich Felker [mailto:dalias@...ifal.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@...ifal.cx] On Behalf Of Rich Felker
> > > Sent: Dienstag, 10. April 2018 16:23
> > > To: musl@...ts.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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
