Hi,
Third version of fix for dlsym hang when reloading DSOs.
Thx, -Leonid
Leonid Lisovskiy (2): ldso: Consistently set & use DL_OPENED flag in both ld.so and libdl [v3] ldso: fix dlsym hang when reloading DSOs
ldso/include/dl-hash.h | 2 +- ldso/ldso/dl-elf.c | 3 +++ ldso/ldso/ldso.c | 8 ++++--- ldso/libdl/libdl.c | 8 +++---- test/dlopen/Makefile.in | 4 +++- test/dlopen/nodelete1.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 74 insertions(+), 10 deletions(-) create mode 100644 test/dlopen/nodelete1.c
Previously, DL_OPENED flag was set in libdl only and never used. Set it centralized in _dl_load_elf_shared_library() & use it in both ld.so and libdl. Additionally, rename it to DL_OPENED2 for clarity.
Signed-off-by: Leonid Lisovskiy lly.dev@gmail.com --- ldso/include/dl-hash.h | 2 +- ldso/ldso/dl-elf.c | 1 + ldso/ldso/ldso.c | 8 +++++--- ldso/libdl/libdl.c | 4 +--- 4 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/ldso/include/dl-hash.h b/ldso/include/dl-hash.h index d6282bb..bdb999a 100644 --- a/ldso/include/dl-hash.h +++ b/ldso/include/dl-hash.h @@ -153,7 +153,7 @@ struct elf_resolve { #define JMP_RELOCS_DONE 0x000002 #define INIT_FUNCS_CALLED 0x000004 #define FINI_FUNCS_CALLED 0x000008 -#define DL_OPENED 0x000010 +#define DL_OPENED2 0x000010 #define DL_RESERVED 0x000020
extern struct dyn_elf * _dl_symbol_tables; diff --git a/ldso/ldso/dl-elf.c b/ldso/ldso/dl-elf.c index 04e8c60..8f71aeb 100644 --- a/ldso/ldso/dl-elf.c +++ b/ldso/ldso/dl-elf.c @@ -546,6 +546,7 @@ struct elf_resolve *_dl_load_elf_shared_library(unsigned int rflags, if (tpnt->st_dev == st.st_dev && tpnt->st_ino == st.st_ino) { /* Already loaded */ tpnt->usage_count++; + tpnt->init_flag |= DL_OPENED2; _dl_close(infile); return tpnt; } diff --git a/ldso/ldso/ldso.c b/ldso/ldso/ldso.c index 7bb6a34..4e8a49e 100644 --- a/ldso/ldso/ldso.c +++ b/ldso/ldso/ldso.c @@ -905,7 +905,7 @@ of this helper program; chances are you did not intend to run this program.\n\
#ifdef __LDSO_LDD_SUPPORT__ if (trace_loaded_objects && !_dl_trace_prelink && - tpnt1->usage_count == 1) { + !(tpnt1->init_flag & DL_OPENED2)) { /* This is a real hack to make * ldd not print the library * itself when run on a @@ -997,7 +997,7 @@ of this helper program; chances are you did not intend to run this program.\n\
# ifdef __LDSO_LDD_SUPPORT__ if (trace_loaded_objects && !_dl_trace_prelink && - tpnt1->usage_count == 1) { + !(tpnt1->init_flag & DL_OPENED2)) { _dl_dprintf(1, "\t%s => %s (%x)\n", cp2, tpnt1->libname, DL_LOADADDR_BASE(tpnt1->loadaddr)); @@ -1034,6 +1034,8 @@ of this helper program; chances are you did not intend to run this program.\n\ /* Insert the ld.so only once */ ldso_tpnt = add_ldso(tpnt, load_addr, ldso_mapaddr, auxvt, rpnt); + } else { + ldso_tpnt->init_flag |= DL_OPENED2; } ldso_tpnt->usage_count++; tpnt1 = ldso_tpnt; @@ -1064,7 +1066,7 @@ of this helper program; chances are you did not intend to run this program.\n\
#ifdef __LDSO_LDD_SUPPORT__ if (trace_loaded_objects && !_dl_trace_prelink && - tpnt1->usage_count == 1) { + !(tpnt1->init_flag & DL_OPENED2)) { _dl_dprintf(1, "\t%s => %s (%x)\n", lpntstr, tpnt1->libname, DL_LOADADDR_BASE(tpnt1->loadaddr)); diff --git a/ldso/libdl/libdl.c b/ldso/libdl/libdl.c index 42a09a8..489c787 100644 --- a/ldso/libdl/libdl.c +++ b/ldso/libdl/libdl.c @@ -396,7 +396,7 @@ static void *do_dlopen(const char *libname, int flag, ElfW(Addr) from) dyn_chain->next_handle = _dl_handles; _dl_handles = dyn_ptr = dyn_chain;
- if (tpnt->usage_count > 1) { + if (tpnt->init_flag & DL_OPENED2) { _dl_if_debug_print("Lib: %s already opened\n", libname); /* see if there is a handle from a earlier dlopen */ for (handle = _dl_handles->next_handle; handle; handle = handle->next_handle) { @@ -412,8 +412,6 @@ static void *do_dlopen(const char *libname, int flag, ElfW(Addr) from) return dyn_chain; }
- tpnt->init_flag |= DL_OPENED; - _dl_if_debug_print("Looking for needed libraries\n"); nlist = 0; runp = alloca(sizeof(*runp));
Hi Leonid, Leonid Lisovskiy wrote,
Previously, DL_OPENED flag was set in libdl only and never used. Set it centralized in _dl_load_elf_shared_library() & use it in both ld.so and libdl. Additionally, rename it to DL_OPENED2 for clarity.
What needs to be clarified? A DL_OPENED2 without DL_OPENED looks strange to me. Isn't git log enough to see what has changed?
best regards Waldemar
On Tue, Jun 21, 2016 at 10:10 PM, Waldemar Brodkorb wbx@uclibc-ng.org wrote:
Hi Leonid, Leonid Lisovskiy wrote,
Previously, DL_OPENED flag was set in libdl only and never used. Set it centralized in _dl_load_elf_shared_library() & use it in both ld.so and libdl. Additionally, rename it to DL_OPENED2 for clarity.
What needs to be clarified? A DL_OPENED2 without DL_OPENED looks strange to me. Isn't git log enough to see what has changed?
At now, logic of use of DL_OPENED flag lost in space. Many years ago, it was used to mark that fact what library was opened. It destroyed by commits in 2005-2006 years, so it is near impossible to get additional information about it.
Currently, we need just to know that library was opened more than once. Sorry if DL_OPENED2 name introduces misunderstanding, I could easily rename it to any better variant.
Regards, Leonid
Hi, Leonid Lisovskiy wrote,
On Tue, Jun 21, 2016 at 10:10 PM, Waldemar Brodkorb wbx@uclibc-ng.org wrote:
Hi Leonid, Leonid Lisovskiy wrote,
Previously, DL_OPENED flag was set in libdl only and never used. Set it centralized in _dl_load_elf_shared_library() & use it in both ld.so and libdl. Additionally, rename it to DL_OPENED2 for clarity.
What needs to be clarified? A DL_OPENED2 without DL_OPENED looks strange to me. Isn't git log enough to see what has changed?
At now, logic of use of DL_OPENED flag lost in space. Many years ago, it was used to mark that fact what library was opened. It destroyed by commits in 2005-2006 years, so it is near impossible to get additional information about it.
Currently, we need just to know that library was opened more than once. Sorry if DL_OPENED2 name introduces misunderstanding, I could easily rename it to any better variant.
Than may be we should rename it to a better variant. Could you resend v2 with that fixed?
thanks Waldemar
On Wed, Jun 22, 2016 at 9:18 PM, Waldemar Brodkorb wbx@uclibc-ng.org wrote:
Currently, we need just to know that library was opened more than once. Sorry if DL_OPENED2 name introduces misunderstanding, I could easily rename it to any better variant.
Than may be we should rename it to a better variant. Could you resend v2 with that fixed?
Of course, yes. What name is the best from your point of view?
regards, Leonid
Hi, Leonid Lisovskiy wrote,
On Wed, Jun 22, 2016 at 9:18 PM, Waldemar Brodkorb wbx@uclibc-ng.org wrote:
Currently, we need just to know that library was opened more than once. Sorry if DL_OPENED2 name introduces misunderstanding, I could easily rename it to any better variant.
Than may be we should rename it to a better variant. Could you resend v2 with that fixed?
Of course, yes. What name is the best from your point of view?
No idea. I just dislike DL_OPENED2 if no DL_OPENED is in use :)
best regards Waldemar
Hi,
On Wed, Jun 22, 2016 at 10:20 PM, Waldemar Brodkorb wbx@uclibc-ng.org wrote:
Hi, Leonid Lisovskiy wrote,
On Wed, Jun 22, 2016 at 9:18 PM, Waldemar Brodkorb wbx@uclibc-ng.org
wrote:
Currently, we need just to know that library was opened more than once. Sorry if DL_OPENED2 name introduces misunderstanding, I could easily rename it to any better variant.
Than may be we should rename it to a better variant. Could you resend v2 with that fixed?
Of course, yes. What name is the best from your point of view?
No idea. I just dislike DL_OPENED2 if no DL_OPENED is in use :)
Sorry for chiming in, but what about 'DL_REOPENED'? This exactly implies that the lib has already been twice at the least, and no misleading '2' suffix :)
It can happen under certain cases that the DSO had refcount 0, but was already loaded. (NODELETE flag is set, or it is pulled in via both NEEDED dependency and explicit dlopen()).
Add extra reference count for NODELETE objects, this will ensure that the reference count never drops below one.
It is improved version of http://lists.busybox.net/pipermail/uclibc/2013-June/047826.html
Signed-off-by: Leonid Lisovskiy lly.dev@gmail.com --- ldso/ldso/dl-elf.c | 2 ++ ldso/libdl/libdl.c | 4 ++-- test/dlopen/Makefile.in | 4 +++- test/dlopen/nodelete1.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 66 insertions(+), 3 deletions(-) create mode 100644 test/dlopen/nodelete1.c
diff --git a/ldso/ldso/dl-elf.c b/ldso/ldso/dl-elf.c index 8f71aeb..a046aeb 100644 --- a/ldso/ldso/dl-elf.c +++ b/ldso/ldso/dl-elf.c @@ -884,6 +884,8 @@ struct elf_resolve *_dl_load_elf_shared_library(unsigned int rflags, #endif (*rpnt)->dyn = tpnt; tpnt->usage_count++; + if (tpnt->rtld_flags & RTLD_NODELETE) + tpnt->usage_count++; #ifdef __LDSO_STANDALONE_SUPPORT__ tpnt->libtype = (epnt->e_type == ET_DYN) ? elf_lib : elf_executable; #else diff --git a/ldso/libdl/libdl.c b/ldso/libdl/libdl.c index 489c787..0cf3b70 100644 --- a/ldso/libdl/libdl.c +++ b/ldso/libdl/libdl.c @@ -818,7 +818,7 @@ static int do_dlclose(void *vhandle, int need_fini) _dl_handles = rpnt->next_handle; _dl_if_debug_print("%s: usage count: %d\n", handle->dyn->libname, handle->dyn->usage_count); - if (handle->dyn->usage_count != 1 || (handle->dyn->rtld_flags & RTLD_NODELETE)) { + if (handle->dyn->usage_count != 1) { handle->dyn->usage_count--; free(handle); return 0; @@ -840,7 +840,7 @@ static int do_dlclose(void *vhandle, int need_fini) for (j = 0; j < handle->init_fini.nlist; ++j) { tpnt = handle->init_fini.init_fini[j]; tpnt->usage_count--; - if (tpnt->usage_count == 0 && !(tpnt->rtld_flags & RTLD_NODELETE)) { + if (tpnt->usage_count == 0) { if ((tpnt->dynamic_info[DT_FINI] || tpnt->dynamic_info[DT_FINI_ARRAY]) && need_fini diff --git a/test/dlopen/Makefile.in b/test/dlopen/Makefile.in index 0bed0f7..740453a 100644 --- a/test/dlopen/Makefile.in +++ b/test/dlopen/Makefile.in @@ -5,7 +5,7 @@ export UCLIBC_ONLY := 1
TESTS := dltest dltest2 dlstatic test1 test2 test3 dlundef dlafk dladdr \ - testscope nodelete tst-origin + testscope nodelete nodelete1 tst-origin
ifneq ($(HAVE_SHARED),y) TESTS_DISABLED := test3 @@ -80,3 +80,5 @@ LDFLAGS_nodelete := -rdynamic -ldl LDFLAGS_nodelmod1.so := -Wl,-z,nodelete LDFLAGS_nodelmod3.so := ./nodelmod4.so LDFLAGS_nodelmod4.so := -Wl,-z,nodelete +nodelete1: nodelmod1.so nodelmod2.so +LDFLAGS_nodelete1 := -rdynamic -ldl diff --git a/test/dlopen/nodelete1.c b/test/dlopen/nodelete1.c new file mode 100644 index 0000000..720e37f --- /dev/null +++ b/test/dlopen/nodelete1.c @@ -0,0 +1,59 @@ +#include <dlfcn.h> +#include <stdio.h> +#include <stdlib.h> + + +int fini_ran; + +#define LIBNAME1 "nodelmod1.so" + +static int +do_test (void) +{ + /* Verify ability to reload RTLD_NODELETE libraries. + */ + void *p; + + p = dlopen (LIBNAME1, RTLD_NOW); + if (p == NULL) + { + printf ("failed to load "LIBNAME1": %s\n", dlerror ()); + exit (1); + } + + if (dlclose (p) != 0) + { + puts ("failed to close "LIBNAME1""); + exit (1); + } + + p = dlopen (LIBNAME1, RTLD_LAZY); + if (p == NULL) + { + printf ("failed to load "LIBNAME1": %s\n", dlerror ()); + exit (1); + } + + if (dlclose (p) != 0) + { + puts ("failed to close "LIBNAME1""); + exit (1); + } + + p = dlopen ("nodelmod2.so", RTLD_LAZY); + if (p == NULL) + { + printf ("failed to load "nodelmod2.so": %s\n", dlerror ()); + exit (1); + } + if (dlclose (p) != 0) + { + puts ("failed to close "nodelmod2.so""); + exit (1); + } + + return 0; +} + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c"
Hi Leonid, Leonid Lisovskiy wrote,
Hi,
Third version of fix for dlsym hang when reloading DSOs.
Thx, -Leonid
Leonid Lisovskiy (2): ldso: Consistently set & use DL_OPENED flag in both ld.so and libdl [v3] ldso: fix dlsym hang when reloading DSOs
ldso/include/dl-hash.h | 2 +- ldso/ldso/dl-elf.c | 3 +++ ldso/ldso/ldso.c | 8 ++++--- ldso/libdl/libdl.c | 8 +++---- test/dlopen/Makefile.in | 4 +++- test/dlopen/nodelete1.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 74 insertions(+), 10 deletions(-) create mode 100644 test/dlopen/nodelete1.c
Applied and pushed, I think the DL_OPENED2 vs. sth else doesn't matter so much :) thx Waldemar
On Sun, Jun 26, 2016 at 1:00 PM, Waldemar Brodkorb wbx@uclibc-ng.org wrote:
Applied and pushed, I think the DL_OPENED2 vs. sth else doesn't matter so much :)
Sorry, I didn't prepare 'DL_REOPENED' variant due to weekend. I'm ready to do replacement, just mail to me.
Regards, Leonid