From 849274d48fc59bfa6db3c713c8ced8026b20f3b7 Mon Sep 17 00:00:00 2001 From: Florian Weimer Date: Thu, 16 Nov 2023 19:55:35 +0100 Subject: [PATCH] elf: Fix force_first handling in dlclose (bug 30981) 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. Reviewed-by: Adhemerval Zanella --- 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 1c7a861..a97a1ef 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 5616c8a..5c846c7 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 4bf9052..cf6453e 100644 --- a/elf/dso-sort-tests-1.def +++ b/elf/dso-sort-tests-1.def @@ -56,14 +56,16 @@ output: b>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[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[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[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[