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

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella:

> diff --git a/elf/Makefile b/elf/Makefile
> index bff94954c9..6e9c7c7167 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile

> @@ -1071,6 +1074,11 @@ 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 \
> +  $(objpfx)tst-trace3.out \
> +  $(objpfx)tst-trace4.out \
> +  $(objpfx)tst-trace5.out \
>    # tests-special
>  endif

This is covered by $(run-built-tests); good.

> @@ -2717,3 +2725,51 @@ $(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 = 1 2 3 4 5
> +# 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.so
> +	$(call libtracemod-mv,2)
> +	$(call libtracemod-mv,3)
> +	$(call libtracemod-mv,4)
> +	$(call libtracemod-mv,5)

.PHONY: libtracemod-mv?

Why do you have to use mv?  Would cp more reliable?

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

I think I get now what you meant by embedding the full path.  I think we
generally avoid that by setting a soname with -Wl,--soname=.  Then the
link editor will use the soname instead of the path.  This way, no -L
hackery with separate directories is necessary at build time.

I think libmarkermod* would be a better model here.

> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> index a2fc278256..2e3e739c5b 100644
> --- a/elf/dl-deps.c
> +++ b/elf/dl-deps.c
> @@ -473,6 +473,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;

Typo: sa[f]e

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

Typo: filter[s] (?)

Thanks,
Florian