Re: [PATCH v2] elf: Fix DFS sorting algorithm for LD_TRACE_LOADED_OBJECTS with missing libraries (BZ #28868)

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella via Libc-alpha:

> diff --git a/elf/Makefile b/elf/Makefile
> index b2bd03a9f6..07b27010f6 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -819,6 +819,11 @@ modules-names = \
>    tst-tlsmod8 \
>    tst-tlsmod9 \
>    tst-unique1mod1 \
> +  libtracemod1 \
> +  libtracemod2 \
> +  libtracemod3 \
> +  libtracemod4 \
> +  libtracemod5 \
>    tst-unique1mod2 \
>    tst-unique2mod1 \
>    tst-unique2mod2 \
> @@ -1072,6 +1077,8 @@ tests-special += \
>    $(objpfx)tst-initorder2-cmp.out \
>    $(objpfx)tst-unused-dep-cmp.out \
>    $(objpfx)tst-unused-dep.out \
> +  $(objpfx)tst-trace1.out \
> +  $(objpfx)tst-trace2.out \
>    # tests-special
>  endif

Hmm, the sorting is slightly weird?

> @@ -2733,3 +2740,57 @@ $(objpfx)tst-p_align3: $(objpfx)tst-p_alignmod3.so
>  $(objpfx)tst-p_align3.out: tst-p_align3.sh $(objpfx)tst-p_align3
>  	$(SHELL) $< $(common-objpfx) '$(test-program-prefix)'; \
>  	$(evaluate-test)
> +
> +
> +libtracemod-suffixes = 5 4 3 2 1
> +define libtracemod
> +$(objpfx)libtracemod$(1).stamp: $(objpfx)libtracemod$(1).so
> +	touch $(objpfx)libtracemod$(1).stamp
> +endef
> +$(foreach s,$(libtracemod-suffixes), $(eval $(call libtracemod,$(s))))
> +
> +# Move the library to a folder so it can be selected by --library-path
> +define libtracemod-mv
> +  test -d $(objpfx)libtracemod$(1) || mkdir $(objpfx)libtracemod$(1)
> +  test -f $(objpfx)libtracemod$(1).so \
> +	  && mv $(objpfx)libtracemod$(1).so $(objpfx)libtracemod$(1)
> +endef
> +libtracemod-mv: $(objpfx)libtracemod1.stamp
> +	$(call libtracemod-mv,2)
> +	$(call libtracemod-mv,3)
> +	$(call libtracemod-mv,4)
> +	$(call libtracemod-mv,5)

This constract is a bit weird.  Why is libtracemod-mv needed as a
separate makefile target?  Why do these stamp files exist?

> +LDFLAGS-libtracemod1.so = -Wl,--no-as-needed \
> +	-L$(objpfx) -ltracemod2 -ltracemod3
> +LDFLAGS-libtracemod2.so = -Wl,--no-as-needed \
> +	-L$(objpfx) -ltracemod4 -ltracemod5
> +$(objpfx)libtracemod2.so: $(objpfx)libtracemod4.stamp \
> +			  $(objpfx)libtracemod5.stamp
> +$(objpfx)libtracemod1.so: $(objpfx)libtracemod2.stamp \
> +			  $(objpfx)libtracemod3.stamp

It's surprising that this works because $(objpfx)libtracemod5.stamp are
empty files.  I would expect link failures here.

I must this is all a bit strange to me.  Usually, when we want
particular DT_NEEDED strings, we use stub link objects with the required
soname, I think, along with a regular/direct dependency on the shared
object pathname (no -l, but the $(objpfx) path).

> +define tst-trace-skeleton
> +$(objpfx)tst-trace$(1).out: $(..)scripts/tst-ld-trace.py \
> +			    $(objpfx)libtracemod1.so \
> +			    libtracemod-mv \
> +			    tst-trace$(1).exp
> +	( $(test-wrapper-env) \
> +	   LD_TRACE_LOADED_OBJECTS=1 \
> +	   $(elf-objpfx)$(rtld-installed-name) \
> +	   --library-path $(objpfx)..:$(strip $(2)) \
> +	   $(objpfx)libtracemod1.so > $$@T \
> +	  && $(PYTHON) $(..)scripts/tst-ld-trace.py $$@T tst-trace$(1).exp \
> +	) > $$@; $$(evaluate-test)
> +endef

$(objpfx).. is $(objpfx-common).  The empty $(2) evaluates to the
current directory below for case 1.  Is this what we want?  There is
also a divergence between default and hard-coded testing mode here, I
think.

We should probably add something to launch tests using Python, so that
the ld.so invocation can be folded into the Python helper script.

> +$(eval $(call tst-trace-skeleton,1,))
> +$(eval $(call tst-trace-skeleton,2,\
> +	$(objpfx)libtracemod2))
> +$(eval $(call tst-trace-skeleton,3,\
> +	$(objpfx)libtracemod2:$(objpfx)libtracemod3))
> +$(eval $(call tst-trace-skeleton,4,\
> +	$(objpfx)libtracemod2:$(objpfx)libtracemod3:$(objpfx)libtracemod4))
> +$(eval $(call tst-trace-skeleton,5,\
> +	$(objpfx)libtracemod2:$(objpfx)libtracemod3:$(objpfx)libtracemod4:\
> +	$(objpfx)libtracemod5))

> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> index c8bab5cad5..8b39383359 100644
> --- a/elf/dl-deps.c
> +++ b/elf/dl-deps.c
> @@ -489,6 +489,8 @@ _dl_map_object_deps (struct link_map *map,
>  
>    for (nlist = 0, runp = known; runp; runp = runp->next)
>      {
> +      /* _dl_sort_maps ignores l_faked object, so it is save to not considere
> +	 them for nlist.  */
>        if (__builtin_expect (trace_mode, 0) && runp->map->l_faked)
>  	/* This can happen when we trace the loading.  */
>  	--map->l_searchlist.r_nlist;
> diff --git a/elf/dl-sort-maps.c b/elf/dl-sort-maps.c
> index 9e9d53ec47..2ed62da7dd 100644
> --- a/elf/dl-sort-maps.c
> +++ b/elf/dl-sort-maps.c
> @@ -140,7 +140,9 @@ static void
>  dfs_traversal (struct link_map ***rpo, struct link_map *map,
>  	       bool *do_reldeps)
>  {
> -  if (map->l_visited)
> +  /* _dl_map_object_deps filter l_faked objects when calculating the
> +     number of maps before calling _dl_sort_maps, ignore them as well.  */
> +  if (map->l_visited || map->l_faked)
>      return;
>  
>    map->l_visited = 1;

Actually code changes look good. 8-)


> diff --git a/scripts/tst-ld-trace.py b/scripts/tst-ld-trace.py
> new file mode 100755
> index 0000000000..b45f406afb
> --- /dev/null
> +++ b/scripts/tst-ld-trace.py
> @@ -0,0 +1,75 @@
> +#!/usr/bin/python3
> +# Dump the output of LD_TRACE_LOADED_OBJECTS in architecture neutral format.
> +# Copyright (C) 2022 Free Software Foundation, Inc.
> +# Copyright The GNU Toolchain Authors.
> +# This file is part of the GNU C Library.
> +#
> +# The GNU C Library is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU Lesser General Public
> +# License as published by the Free Software Foundation; either
> +# version 2.1 of the License, or (at your option) any later version.
> +#
> +# The GNU C Library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +# Lesser General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with the GNU C Library; if not, see
> +# <https://www.gnu.org/licenses/>.
> +
> +import argparse
> +import os
> +import sys
> +
> +
> +def is_vdso(lib):
> +    return lib.startswith('linux-gate') or lib.startswith ('linux-vdso')


Superfluous space: lib.startswith  'linux-vdso')

> +def parse_trace(fin, fref):
> +    trace = []
> +    for line in fin:
> +        line = line.strip()
> +        if is_vdso(line):
> +            continue
> +        fields = line.split('=>' if '=>' in line else ' ')
> +        lib = os.path.basename(fields[0].strip())
> +        if lib.startswith('ld'):
> +            lib = 'ld'
> +        elif lib.startswith('libc'):
> +            lib = 'libc'
> +        found = 1 if fields[1].strip() != 'not found' else 0
> +        trace += ['{} {}'.format(lib, found)]
> +
> +    reference = sorted(line.replace('\n','') for line in fref.readlines())
> +
> +    ret = 0 if sorted(trace) == reference else 1
> +    if ret != 0:
> +        for i in reference:
> +            if i not in trace:
> +                print("Only in {}: {}".format(fref.name, i))
> +        for i in trace:
> +            if i not in reference:
> +                print("Only in {}: {}".format(fin.name, i))
> +    sys.exit (ret)

" could be '.

Thanks,
Florian