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

[Adhemerval Zanella]

On 25/02/2022 14:23, Florian Weimer wrote:
> * 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?

Ack.

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

The library must be present only on the specified folder, since the base
directory will used as well for tst-trace1.

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

Yeah, it is simpler.  I will change to use -soname.

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

Ack.

>> 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] (?)

Ack.