[v2] elf: Fix force_first handling in dlclose (bug 30785)

The force_first parameter was ineffective because the dlclose'd
object was not necessarily the first in the maps array.  Also
enable force_first handling unconditionally, regardless of namespace.
The initial object in a namespace should be destructed first, too.

The _dl_sort_maps_dfs function had early returns for relocation
dependency processing which broke force_first handling, too, and
this is fixed in this change as well.

---
v2: Update l_idx when maps[0] is swapped with map.  This fixes the
    reported regression in the patch.

 elf/dl-close.c           | 23 +++++++++++++++++++----
 elf/dl-sort-maps.c       |  7 +++----
 elf/dso-sort-tests-1.def | 12 +++++++-----
 3 files changed, 29 insertions(+), 13 deletions(-)


base-commit: 5dd3bda59c2d9da138f0d98808d087cdb95cdc17

On 07/11/23 13:12, Florian Weimer wrote:
> The force_first parameter was ineffective because the dlclose'd
> object was not necessarily the first in the maps array.  Also
> enable force_first handling unconditionally, regardless of namespace.
> The initial object in a namespace should be destructed first, too.
> 
> The _dl_sort_maps_dfs function had early returns for relocation
> dependency processing which broke force_first handling, too, and
> this is fixed in this change as well.

Would you re-open 30785 or this patch is intended for 30981?  Also, as 
a side note, it is really not helpful to link a RH bug report [1] that
is not easily accessible (it requires to log with a RH account and
ask for extra permission).  

I think to link external trackers, it must not require additional steps
to query for the report information; otherwise I see no point in keeping 
such links on the sourceware bugzilla.

The patch itself looks good, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

[1] https://bugzilla.redhat.com/show_bug.cgi?id=2233338

> 
> ---
> v2: Update l_idx when maps[0] is swapped with map.  This fixes the
>     reported regression in the patch.
> 
>  elf/dl-close.c           | 23 +++++++++++++++++++----
>  elf/dl-sort-maps.c       |  7 +++----
>  elf/dso-sort-tests-1.def | 12 +++++++-----
>  3 files changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/elf/dl-close.c b/elf/dl-close.c
> index 1c7a861db1..a97a1efa45 100644
> --- a/elf/dl-close.c
> +++ b/elf/dl-close.c
> @@ -153,6 +153,16 @@ _dl_close_worker (struct link_map *map, bool force)
>      }
>    assert (idx == nloaded);
>  
> +  /* Put the dlclose'd map first, so that its destructor runs first.
> +     The map variable is NULL after a retry.  */
> +  if (map != NULL)
> +    {
> +      maps[map->l_idx] = maps[0];
> +      maps[map->l_idx]->l_idx = map->l_idx;
> +      maps[0] = map;
> +      maps[0]->l_idx = 0;
> +    }
> +
>    /* Keep track of the lowest index link map we have covered already.  */
>    int done_index = -1;
>    while (++done_index < nloaded)
> @@ -226,9 +236,10 @@ _dl_close_worker (struct link_map *map, bool force)
>  	  }
>      }
>  
> -  /* Sort the entries.  We can skip looking for the binary itself which is
> -     at the front of the search list for the main namespace.  */
> -  _dl_sort_maps (maps, nloaded, (nsid == LM_ID_BASE), true);
> +  /* Sort the entries.  Unless retrying, the maps[0] object (the
> +     original argument to dlclose) needs to remain first, so that its
> +     destructor runs first.  */
> +  _dl_sort_maps (maps, nloaded, /* force_first */ map != NULL, true);
>  
>    /* Call all termination functions at once.  */
>    bool unload_any = false;
> @@ -732,7 +743,11 @@ _dl_close_worker (struct link_map *map, bool force)
>    /* Recheck if we need to retry, release the lock.  */
>   out:
>    if (dl_close_state == rerun)
> -    goto retry;
> +    {
> +      /* The map may have been deallocated.  */
> +      map = NULL;
> +      goto retry;
> +    }
>  
>    dl_close_state = not_pending;
>  }
> diff --git a/elf/dl-sort-maps.c b/elf/dl-sort-maps.c
> index 5616c8a6a3..5c846c7c6f 100644
> --- a/elf/dl-sort-maps.c
> +++ b/elf/dl-sort-maps.c
> @@ -255,13 +255,12 @@ _dl_sort_maps_dfs (struct link_map **maps, unsigned int nmaps,
>  	     The below memcpy is not needed in the do_reldeps case here,
>  	     since we wrote back to maps[] during DFS traversal.  */
>  	  if (maps_head == maps)
> -	    return;
> +	    break;
>  	}
>        assert (maps_head == maps);
> -      return;
>      }
> -
> -  memcpy (maps, rpo, sizeof (struct link_map *) * nmaps);
> +  else
> +    memcpy (maps, rpo, sizeof (struct link_map *) * nmaps);
>  
>    /* Skipping the first object at maps[0] is not valid in general,
>       since traversing along object dependency-links may "find" that
> diff --git a/elf/dso-sort-tests-1.def b/elf/dso-sort-tests-1.def
> index 4bf9052db1..cf6453e9eb 100644
> --- a/elf/dso-sort-tests-1.def
> +++ b/elf/dso-sort-tests-1.def
> @@ -56,14 +56,16 @@ output: b>a>{}<a<b
>  # relocation(dynamic) dependencies. While this is technically unspecified, the
>  # presumed reasonable practical behavior is for the destructor order to respect
>  # the static DT_NEEDED links (here this means the a->b->c->d order).
> -# The older dynamic_sort=1 algorithm does not achieve this, while the DFS-based
> -# dynamic_sort=2 algorithm does, although it is still arguable whether going
> -# beyond spec to do this is the right thing to do.
> +# The older dynamic_sort=1 algorithm originally did not achieve this,
> +# but this was a bug in the way _dl_sort_maps was called from _dl_close_worker,
> +# effectively disabling proper force_first handling.
> +# The new dynamic_sort=2 algorithm shows the effect of the simpler force_first
> +# handling: the a object is simply moved to the front.
>  # The below expected outputs are what the two algorithms currently produce
>  # respectively, for regression testing purposes.
>  tst-bz15311: {+a;+e;+f;+g;+d;%d;-d;-g;-f;-e;-a};a->b->c->d;d=>[ba];c=>a;b=>e=>a;c=>f=>b;d=>g=>c
> -output(glibc.rtld.dynamic_sort=1): {+a[d>c>b>a>];+e[e>];+f[f>];+g[g>];+d[];%d(b(e(a()))a()g(c(a()f(b(e(a()))))));-d[];-g[];-f[];-e[];-a[<a<c<d<g<f<b<e];}
> -output(glibc.rtld.dynamic_sort=2): {+a[d>c>b>a>];+e[e>];+f[f>];+g[g>];+d[];%d(b(e(a()))a()g(c(a()f(b(e(a()))))));-d[];-g[];-f[];-e[];-a[<g<f<a<b<c<d<e];}
> +output(glibc.rtld.dynamic_sort=1): {+a[d>c>b>a>];+e[e>];+f[f>];+g[g>];+d[];%d(b(e(a()))a()g(c(a()f(b(e(a()))))));-d[];-g[];-f[];-e[];-a[<a<b<c<d<g<f<e];}
> +output(glibc.rtld.dynamic_sort=2): {+a[d>c>b>a>];+e[e>];+f[f>];+g[g>];+d[];%d(b(e(a()))a()g(c(a()f(b(e(a()))))));-d[];-g[];-f[];-e[];-a[<a<g<f<b<c<d<e];}
>  
>  # Test that even in the presence of dependency loops involving dlopen'ed
>  # object, that object is initialized last (and not unloaded prematurely).
> 
> base-commit: 5dd3bda59c2d9da138f0d98808d087cdb95cdc17
>

* Adhemerval Zanella Netto:

> On 07/11/23 13:12, Florian Weimer wrote:
>> The force_first parameter was ineffective because the dlclose'd
>> object was not necessarily the first in the maps array.  Also
>> enable force_first handling unconditionally, regardless of namespace.
>> The initial object in a namespace should be destructed first, too.
>> 
>> The _dl_sort_maps_dfs function had early returns for relocation
>> dependency processing which broke force_first handling, too, and
>> this is fixed in this change as well.
>
> Would you re-open 30785 or this patch is intended for 30981?

Good point, I should reference bug 30981 instead.

> Also, as a side note, it is really not helpful to link a RH bug report
> [1] that is not easily accessible (it requires to log with a RH
> account and ask for extra permission).

> [1] https://bugzilla.redhat.com/show_bug.cgi?id=2233338

I think at the time, I still had hope that it could be made public, but
that didn't happen.  The bug doesn't even list our many failed attempts
at fixing it, most of these problems occurred in Fedora.

> I think to link external trackers, it must not require additional steps
> to query for the report information; otherwise I see no point in keeping 
> such links on the sourceware bugzilla.
>
> The patch itself looks good, thanks.
>
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

Thanks, I'll push it after updating the bug reference.

Thanks,
Florian