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

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