On Fri, May 16, 2025 at 03:22:52PM +0300, Andriy Gapon wrote:
> On 27/04/2025 17:26, Mark Johnston wrote:
> > On Thu, Apr 24, 2025 at 08:56:44AM +0300, Andriy Gapon wrote:
> > > On 23/04/2025 21:56, Andriy Gapon wrote:
> > > > BTW, I've been wondering how illumos avoids the problem even though they
> > > > do not use any special dlopen flags.
> > > > It turns out that they link almost all system shared libraries with
> > > > -Bdirect option (which is Solaris/illumos specific).
> > > > It's somewhat similar to, but different from, -Bsymbolic.
> > > > https://docs.oracle.com/cd/E23824_01/html/819-0690/aehzq.html#scrolltoc
> > > > https://docs.oracle.com/cd/E36784_01/html/E36857/gejfe.html
> > >
> > > Oh, and it looks like there is an even better explanation for illumos.
> > > There is a version map file for libdtrace which explicitly lists API
> > > functions and makes everything else local.
> > > https://github.com/illumos/illumos-gate/blob/master/usr/src/lib/libdtrace/common/mapfile-vers
> > >
> > > I wonder why we didn't do the same when porting.
> > > Maybe we should do that now?
> >
> > I don't have any objection, but I believe adding a version map after the
> > fact doesn't accomplish much, assuming that we care deeply about ABI
> > stability in libdtrace.so (I'm not sure we do though).
>
> My primary goal here is not ABI stability, but hiding symbols that really
> should not be exported. See more at the end.
>
> At the same time I am not sure why it could be too late to start caring
> about ABI stability now. Assuming we actually would want to do that.
I just mean that the version map helps only helps provide stability for
binaries linked to libdtrace.so after the version map is introduced.
> And I don't want to single out libtrace here.
> It seems that the story is the same for all libraries that have been ported
> from illumos.
> E.g., libzfs_core was supposed to be a library that cares greatly about its
> API / ABI stability.
>
> > > > I think that on FreeBSD we should use symbol visibility attributes or a
> > > > symbol map to hide (make local) symbols that are not expected to be
> > > > interposed or have a high chance to be interposed by accident.
> > > >
> > > > IMO, yyparse should definitely get that treatment.
> > > >
> > > > I think that approach would be better than magic rtld tricks.
> > > > Especially because the tricks do not work with the current rtld.
> > > > I'd rather make a change to libdtrace.so than to rtld.
> > >
> > > This, while not as nice as the illumos solution, fixes my specific issue:
> > > diff --git a/cddl/lib/libdtrace/Makefile b/cddl/lib/libdtrace/Makefile
> > > index d086fffb07bc..58054d129b49 100644
> > > --- a/cddl/lib/libdtrace/Makefile
> > > +++ b/cddl/lib/libdtrace/Makefile
> > > @@ -146,7 +146,8 @@ CFLAGS+= -fsanitize=address -fsanitize=undefined
> > > LDFLAGS+= -fsanitize=address -fsanitize=undefined
> > > .endif
> > >
> > > -LIBADD= ctf elf proc pthread rtld_db xo
> > > +VERSION_MAP= ${.CURDIR}/Symbol.map
> > > +LIBADD= ctf elf proc pthread rtld_db xo
> > >
> > > CLEANFILES= dt_errtags.c dt_names.c
> > >
> > > diff --git a/cddl/lib/libdtrace/Symbol.map b/cddl/lib/libdtrace/Symbol.map
> > > new file mode 100644
> > > index 000000000000..89ee9de65209
> > > --- /dev/null
> > > +++ b/cddl/lib/libdtrace/Symbol.map
> > > @@ -0,0 +1,4 @@
> > > +{
> > > + local:
> > > + yy*;
> > > +};
> >
> > This just gives the lexer/parser symbols in libdtrace.so local
> > visibility? I think that's fine.
> Yes, that's the intention.
>
> I tested locally two versions of Symbol.map for libdtrace.
> The basic one quoted here and a more extended one based on illumos
> lib/libdtrace/common/mapfile-vers.
> The latter version does not define any symbol versions, its purpose is only
> to be a white-list of things to make public / global:
> https://people.freebsd.org/~avg/libdtrace-Symbol.map
Do we really want to export _dtrace_debug?
> Comparing to illumos I only had to add 3 dtrace_oformat* symbols,
>
> Both versions worked equally well in my tests, but maybe I missed more of
> FreeBSD extensions.
>
> Which one would be better to get into the tree?
Having a full whitelist seems preferable to me. Did you test lockstat
as well? I believe it and dtrace(8) are the only users of libdtrace.so
in the base system.
