* [review] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
@ 2019-10-31 19:20 Florian Weimer (Code Review)
2019-10-31 20:18 ` Christian Häggström (Code Review)
` (5 more replies)
0 siblings, 6 replies; 12+ messages in thread
From: Florian Weimer (Code Review) @ 2019-10-31 19:20 UTC (permalink / raw)
To: libc-alpha; +Cc: Florian Weimer
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471
......................................................................
Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
This introduces a “pending NODELETE” state in the link map, which is
flipped to the persistent NODELETE state late in dlopen, via
activate_nodelete. During initial relocation, symbol binding
records pending NODELETE state only. dlclose ignores pending NODELETE
state. Taken together, this results that a partially completed dlopen
is rolled back completely because new NODELETE mappings are unloaded.
Tested on x86_64-linux-gnu and i386-linux-gnu.
Change-Id: Ib2a3d86af6f92d75baca65431d74783ee0dbc292
---
M elf/Makefile
M elf/dl-close.c
M elf/dl-lookup.c
M elf/dl-open.c
M elf/get-dynamic-info.h
A elf/tst-dlopenfail.c
A elf/tst-dlopenfaillinkmod.c
A elf/tst-dlopenfailmod1.c
A elf/tst-dlopenfailmod2.c
M include/link.h
10 files changed, 327 insertions(+), 38 deletions(-)
diff --git a/elf/Makefile b/elf/Makefile
index a09b650..a443050a 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -193,7 +193,7 @@
tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
tst-unwind-ctor tst-unwind-main tst-audit13 \
tst-sonamemove-link tst-sonamemove-dlopen tst-dlopen-tlsmodid \
- tst-dlopen-self tst-initfinilazyfail
+ tst-dlopen-self tst-initfinilazyfail tst-dlopenfail
# reldep9
tests-internal += loadtest unload unload2 circleload1 \
neededtest neededtest2 neededtest3 neededtest4 \
@@ -282,7 +282,8 @@
tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib \
tst-audit13mod1 tst-sonamemove-linkmod1 \
tst-sonamemove-runmod1 tst-sonamemove-runmod2 \
- tst-initlazyfailmod tst-finilazyfailmod
+ tst-initlazyfailmod tst-finilazyfailmod \
+ tst-dlopenfailmod1 tst-dlopenfaillinkmod tst-dlopenfailmod2
# Most modules build with _ISOMAC defined, but those filtered out
# depend on internal headers.
modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\
@@ -1580,3 +1581,13 @@
-Wl,-z,lazy -Wl,--unresolved-symbols=ignore-all
LDFLAGS-tst-finilazyfailmod.so = \
-Wl,-z,lazy -Wl,--unresolved-symbols=ignore-all
+
+$(objpfx)tst-dlopenfail: $(libdl)
+$(objpfx)tst-dlopenfail.out: \
+ $(objpfx)tst-dlopenfailmod1.so $(objpfx)tst-dlopenfailmod2.so
+# Order matters here. tst-dlopenfaillinkmod.so's soname ensures
+# a run-time loader failure.
+$(objpfx)tst-dlopenfailmod1.so: \
+ $(shared-thread-library) $(objpfx)tst-dlopenfaillinkmod.so
+LDFLAGS-tst-dlopenfaillinkmod.so = -Wl,-soname,tst-dlopenfail-missingmod.so
+$(objpfx)tst-dlopenfailmod2.so: $(shared-thread-library)
diff --git a/elf/dl-close.c b/elf/dl-close.c
index 2e38943..b919a03 100644
--- a/elf/dl-close.c
+++ b/elf/dl-close.c
@@ -168,14 +168,6 @@
char done[nloaded];
struct link_map *maps[nloaded];
- /* Clear DF_1_NODELETE to force object deletion. We don't need to touch
- l_tls_dtor_count because forced object deletion only happens when an
- error occurs during object load. Destructor registration for TLS
- non-POD objects should not have happened till then for this
- object. */
- if (force)
- map->l_flags_1 &= ~DF_1_NODELETE;
-
/* Run over the list and assign indexes to the link maps and enter
them into the MAPS array. */
int idx = 0;
@@ -205,7 +197,7 @@
/* Check whether this object is still used. */
if (l->l_type == lt_loaded
&& l->l_direct_opencount == 0
- && (l->l_flags_1 & DF_1_NODELETE) == 0
+ && l->l_nodelete != link_map_nodelete_active
/* See CONCURRENCY NOTES in cxa_thread_atexit_impl.c to know why
acquire is sufficient and correct. */
&& atomic_load_acquire (&l->l_tls_dtor_count) == 0
@@ -288,7 +280,7 @@
if (!used[i])
{
assert (imap->l_type == lt_loaded
- && (imap->l_flags_1 & DF_1_NODELETE) == 0);
+ && imap->l_nodelete != link_map_nodelete_active);
/* Call its termination function. Do not do it for
half-cooked objects. Temporarily disable exception
@@ -828,7 +820,7 @@
before we took the lock. There is no way to detect this (see below)
so we proceed assuming this isn't the case. First see whether we
can remove the object at all. */
- if (__glibc_unlikely (map->l_flags_1 & DF_1_NODELETE))
+ if (__glibc_unlikely (map->l_nodelete == link_map_nodelete_active))
{
/* Nope. Do nothing. */
__rtld_lock_unlock_recursive (GL(dl_load_lock));
diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
index aaaf437..a2e85a5 100644
--- a/elf/dl-lookup.c
+++ b/elf/dl-lookup.c
@@ -192,9 +192,10 @@
Return the matching symbol in RESULT. */
static void
do_lookup_unique (const char *undef_name, uint_fast32_t new_hash,
- const struct link_map *map, struct sym_val *result,
+ struct link_map *map, struct sym_val *result,
int type_class, const ElfW(Sym) *sym, const char *strtab,
- const ElfW(Sym) *ref, const struct link_map *undef_map)
+ const ElfW(Sym) *ref, const struct link_map *undef_map,
+ int flags)
{
/* We have to determine whether we already found a symbol with this
name before. If not then we have to add it to the search table.
@@ -222,7 +223,7 @@
copy from the copy addressed through the
relocation. */
result->s = sym;
- result->m = (struct link_map *) map;
+ result->m = map;
}
else
{
@@ -311,9 +312,19 @@
new_hash, strtab + sym->st_name, sym, map);
if (map->l_type == lt_loaded)
- /* Make sure we don't unload this object by
- setting the appropriate flag. */
- ((struct link_map *) map)->l_flags_1 |= DF_1_NODELETE;
+ {
+ /* Make sure we don't unload this object by
+ setting the appropriate flag. */
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_BINDINGS)
+ && map->l_nodelete == link_map_nodelete_inactive)
+ _dl_debug_printf ("\
+marking %s [%lu] as NODELETE due to unique symbol\n",
+ map->l_name, map->l_ns);
+ if (flags & DL_LOOKUP_FOR_RELOCATE)
+ map->l_nodelete = link_map_nodelete_pending;
+ else
+ map->l_nodelete = link_map_nodelete_active;
+ }
}
++tab->n_elements;
@@ -525,8 +536,9 @@
return 1;
case STB_GNU_UNIQUE:;
- do_lookup_unique (undef_name, new_hash, map, result, type_class,
- sym, strtab, ref, undef_map);
+ do_lookup_unique (undef_name, new_hash, (struct link_map *) map,
+ result, type_class, sym, strtab, ref,
+ undef_map, flags);
return 1;
default:
@@ -568,9 +580,13 @@
if (undef_map == map)
return 0;
- /* Avoid references to objects which cannot be unloaded anyway. */
+ /* Avoid references to objects which cannot be unloaded anyway. We
+ do not need to record dependencies if this object goes away
+ during dlopen failure, either. IFUNC resolvers with relocation
+ dependencies may pick an dependency which can be dlclose'd, but
+ such IFUNC resolvers are undefined anyway. */
assert (map->l_type == lt_loaded);
- if ((map->l_flags_1 & DF_1_NODELETE) != 0)
+ if (map->l_nodelete != link_map_nodelete_inactive)
return 0;
struct link_map_reldeps *l_reldeps
@@ -678,16 +694,33 @@
/* Redo the NODELETE check, as when dl_load_lock wasn't held
yet this could have changed. */
- if ((map->l_flags_1 & DF_1_NODELETE) != 0)
+ if (map->l_nodelete != link_map_nodelete_inactive)
goto out;
/* If the object with the undefined reference cannot be removed ever
just make sure the same is true for the object which contains the
definition. */
if (undef_map->l_type != lt_loaded
- || (undef_map->l_flags_1 & DF_1_NODELETE) != 0)
+ || (undef_map->l_nodelete != link_map_nodelete_inactive))
{
- map->l_flags_1 |= DF_1_NODELETE;
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_BINDINGS)
+ && map->l_nodelete == link_map_nodelete_inactive)
+ {
+ if (undef_map->l_name[0] == '\0')
+ _dl_debug_printf ("\
+marking %s [%lu] as NODELETE due to reference to main program\n",
+ map->l_name, map->l_ns);
+ else
+ _dl_debug_printf ("\
+marking %s [%lu] as NODELETE due to reference to %s [%lu]\n",
+ map->l_name, map->l_ns,
+ undef_map->l_name, undef_map->l_ns);
+ }
+
+ if (flags & DL_LOOKUP_FOR_RELOCATE)
+ map->l_nodelete = link_map_nodelete_pending;
+ else
+ map->l_nodelete = link_map_nodelete_active;
goto out;
}
@@ -712,7 +745,18 @@
no fatal problem. We simply make sure the referenced object
cannot be unloaded. This is semantically the correct
behavior. */
- map->l_flags_1 |= DF_1_NODELETE;
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_BINDINGS)
+ && map->l_nodelete == link_map_nodelete_inactive)
+ _dl_debug_printf ("\
+marking %s [%lu] as NODELETE due to memory allocation failure\n",
+ map->l_name, map->l_ns);
+ if (flags & DL_LOOKUP_FOR_RELOCATE)
+ /* In case of non-lazy binding, we could actually
+ report the memory allocation error, but for now, we
+ use the conservative approximation as well. */
+ map->l_nodelete = link_map_nodelete_pending;
+ else
+ map->l_nodelete = link_map_nodelete_active;
goto out;
}
else
diff --git a/elf/dl-open.c b/elf/dl-open.c
index a305fef..9b42f2b 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -410,6 +410,40 @@
}
}
+/* Mark the objects as NODELETE if required. This is delayed until
+ after dlopen failure is not possible, so that _dl_close can clean
+ up objects if necessary. */
+static void
+activate_nodelete (struct link_map *new, int mode)
+{
+ if (mode & RTLD_NODELETE || new->l_nodelete == link_map_nodelete_pending)
+ {
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_FILES))
+ _dl_debug_printf ("activating NODELETE for %s [%lu]\n",
+ new->l_name, new->l_ns);
+ new->l_nodelete = link_map_nodelete_active;
+ }
+
+ for (unsigned int i = 0; i < new->l_searchlist.r_nlist; ++i)
+ {
+ struct link_map *imap = new->l_searchlist.r_list[i];
+ if (imap->l_nodelete == link_map_nodelete_pending)
+ {
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_FILES))
+ _dl_debug_printf ("activating NODELETE for %s [%lu]\n",
+ imap->l_name, imap->l_ns);
+
+ /* Only new objects should have set
+ link_map_nodelete_pending. Existing objects should not
+ have gained any new dependencies and therefore cannot
+ reach NODELETE status. */
+ assert (!imap->l_init_called || imap->l_type != lt_loaded);
+
+ imap->l_nodelete = link_map_nodelete_active;
+ }
+ }
+}
+
/* struct dl_init_args and call_dl_init are used to call _dl_init with
exception handling disabled. */
struct dl_init_args
@@ -479,12 +513,6 @@
return;
}
- /* Mark the object as not deletable if the RTLD_NODELETE flags was passed.
- Do this early so that we don't skip marking the object if it was
- already loaded. */
- if (__glibc_unlikely (mode & RTLD_NODELETE))
- new->l_flags_1 |= DF_1_NODELETE;
-
if (__glibc_unlikely (mode & __RTLD_SPROF))
/* This happens only if we load a DSO for 'sprof'. */
return;
@@ -500,19 +528,37 @@
_dl_debug_printf ("opening file=%s [%lu]; direct_opencount=%u\n\n",
new->l_name, new->l_ns, new->l_direct_opencount);
- /* If the user requested the object to be in the global namespace
- but it is not so far, add it now. */
+ /* If the user requested the object to be in the global
+ namespace but it is not so far, prepare to add it now. This
+ can raise an exception to do a malloc failure. */
if ((mode & RTLD_GLOBAL) && new->l_global == 0)
+ add_to_global_resize (new);
+
+ /* Mark the object as not deletable if the RTLD_NODELETE flags
+ was passed. */
+ if (__glibc_unlikely (mode & RTLD_NODELETE))
{
- add_to_global_resize (new);
- add_to_global_update (new);
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_FILES)
+ && new->l_nodelete == link_map_nodelete_inactive)
+ _dl_debug_printf ("marking %s [%lu] as NODELETE\n",
+ new->l_name, new->l_ns);
+ new->l_nodelete = link_map_nodelete_active;
}
+ /* Finalize the addition to the global scope. */
+ if ((mode & RTLD_GLOBAL) && new->l_global == 0)
+ add_to_global_update (new);
+
assert (_dl_debug_initialize (0, args->nsid)->r_state == RT_CONSISTENT);
return;
}
+ /* Schedule NODELETE marking for the directly loaded object if
+ requested. */
+ if (__glibc_unlikely (mode & RTLD_NODELETE))
+ new->l_nodelete = link_map_nodelete_pending;
+
/* Load that object's dependencies. */
_dl_map_object_deps (new, NULL, 0, 0,
mode & (__RTLD_DLOPEN | RTLD_DEEPBIND | __RTLD_AUDIT));
@@ -584,6 +630,14 @@
int relocation_in_progress = 0;
+ /* Perform relocation. This can trigger lazy binding in IFUNC
+ resolvers. For NODELETE mappings, these dependencies are not
+ recorded because the flag has not been applied to the newly
+ loaded objects. This means that upon dlopen failure, these
+ NODELETE objects can be unloaded despite existing references to
+ them. However, such relocation dependencies in IFUNC resolvers
+ are undefined anyway, so this is not a problem. */
+
for (unsigned int i = nmaps; i-- > 0; )
{
l = maps[i];
@@ -613,7 +667,7 @@
_dl_start_profile ();
/* Prevent unloading the object. */
- GL(dl_profile_map)->l_flags_1 |= DF_1_NODELETE;
+ GL(dl_profile_map)->l_nodelete = link_map_nodelete_active;
}
}
else
@@ -644,6 +698,8 @@
All memory allocations for new objects must have happened
before. */
+ activate_nodelete (new, mode);
+
/* Second stage after resize_scopes: Actually perform the scope
update. After this, dlsym and lazy binding can bind to new
objects. */
@@ -802,6 +858,10 @@
GL(dl_tls_dtv_gaps) = true;
_dl_close_worker (args.map, true);
+
+ /* All link_map_nodelete_pending objects should have been
+ deleted at this point, which is why it is not necessary
+ to reset the flag here. */
}
assert (_dl_debug_initialize (0, args.nsid)->r_state == RT_CONSISTENT);
diff --git a/elf/get-dynamic-info.h b/elf/get-dynamic-info.h
index af9dd57..075683d 100644
--- a/elf/get-dynamic-info.h
+++ b/elf/get-dynamic-info.h
@@ -163,6 +163,8 @@
if (info[VERSYMIDX (DT_FLAGS_1)] != NULL)
{
l->l_flags_1 = info[VERSYMIDX (DT_FLAGS_1)]->d_un.d_val;
+ if (l->l_flags_1 & DF_1_NODELETE)
+ l->l_nodelete = link_map_nodelete_pending;
/* Only DT_1_SUPPORTED_MASK bits are supported, and we would like
to assert this, but we can't. Users have been setting
diff --git a/elf/tst-dlopenfail.c b/elf/tst-dlopenfail.c
new file mode 100644
index 0000000..ce3140c
--- /dev/null
+++ b/elf/tst-dlopenfail.c
@@ -0,0 +1,79 @@
+/* Test dlopen rollback after failures involving NODELETE objects (bug 20839).
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <dlfcn.h>
+#include <errno.h>
+#include <gnu/lib-names.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/xdlfcn.h>
+
+static int
+do_test (void)
+{
+ /* This test uses libpthread as the canonical NODELETE module. If
+ libpthread is no longer NODELETE because it has been merged into
+ libc, the test needs to be updated. */
+ TEST_VERIFY (dlsym (NULL, "pthread_create") == NULL);
+
+ /* This is expected to fail because of the missing dependency. */
+ puts ("info: attempting to load tst-dlopenfailmod1.so");
+ TEST_VERIFY (dlopen ("tst-dlopenfailmod1.so", RTLD_LAZY) == NULL);
+ const char *message = dlerror ();
+ TEST_COMPARE_STRING (message,
+ "tst-dlopenfail-missingmod.so:"
+ " cannot open shared object file:"
+ " No such file or directory");
+
+ /* Do not probe for the presence of libpthread at this point because
+ that might trigger relocation if bug 20839 is present, obscuring
+ a subsequent crash. */
+
+ /* This is expected to succeed. */
+ puts ("info: loading tst-dlopenfailmod2.so");
+ void *handle = xdlopen ("tst-dlopenfailmod2.so", RTLD_NOW);
+ xdlclose (handle);
+
+ /* libpthread should remain loaded. */
+ TEST_VERIFY (dlopen (LIBPTHREAD_SO, RTLD_LAZY | RTLD_NOLOAD) != NULL);
+ TEST_VERIFY (dlsym (NULL, "pthread_create") == NULL);
+
+ /* We can make libpthread global, and then the symbol should become
+ available. */
+ TEST_VERIFY (dlopen (LIBPTHREAD_SO, RTLD_LAZY | RTLD_GLOBAL) != NULL);
+ TEST_VERIFY (dlsym (NULL, "pthread_create") != NULL);
+
+ /* sem_open is sufficiently complex to depend on relocations. */
+ void *(*sem_open_ptr) (const char *, int flag, ...)
+ = dlsym (NULL, "sem_open");
+ if (sem_open_ptr == NULL)
+ /* Hurd does not implement sem_open. */
+ puts ("warning: sem_open not found, further testing not possible");
+ else
+ {
+ errno = 0;
+ TEST_VERIFY (sem_open_ptr ("/", 0) == NULL);
+ TEST_COMPARE (errno, EINVAL);
+ }
+
+ return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/elf/tst-dlopenfaillinkmod.c b/elf/tst-dlopenfaillinkmod.c
new file mode 100644
index 0000000..3b14b02
--- /dev/null
+++ b/elf/tst-dlopenfaillinkmod.c
@@ -0,0 +1,17 @@
+/* Empty module with a soname which is not available at run time.
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
diff --git a/elf/tst-dlopenfailmod1.c b/elf/tst-dlopenfailmod1.c
new file mode 100644
index 0000000..6ef4882
--- /dev/null
+++ b/elf/tst-dlopenfailmod1.c
@@ -0,0 +1,36 @@
+/* Module which depends on two modules: one NODELETE, one missing.
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+/* Note: Due to the missing second module, this object cannot be
+ loaded at run time. */
+
+#include <pthread.h>
+#include <stdio.h>
+#include <unistd.h>
+
+/* Force linking against libpthread. */
+void *pthread_create_reference = pthread_create;
+
+/* The constructor will never be executed because the module cannot be
+ loaded. */
+static void __attribute__ ((constructor))
+init (void)
+{
+ puts ("tst-dlopenfailmod1 constructor executed");
+ _exit (1);
+}
diff --git a/elf/tst-dlopenfailmod2.c b/elf/tst-dlopenfailmod2.c
new file mode 100644
index 0000000..7d60038
--- /dev/null
+++ b/elf/tst-dlopenfailmod2.c
@@ -0,0 +1,29 @@
+/* Module which depends on on a NODELETE module, and can be loaded.
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <pthread.h>
+#include <stdio.h>
+
+/* Force linking against libpthread. */
+void *pthread_create_reference = pthread_create;
+
+static void __attribute__ ((constructor))
+init (void)
+{
+ puts ("info: tst-dlopenfailmod2.so constructor invoked");
+}
diff --git a/include/link.h b/include/link.h
index 1184201..2bf6339 100644
--- a/include/link.h
+++ b/include/link.h
@@ -79,6 +79,21 @@
int malloced;
};
+/* Type used by the l_nodelete member. */
+enum link_map_nodelete
+{
+ /* This link map can be deallocated. */
+ link_map_nodelete_inactive,
+
+ /* This link map cannot be deallocated. */
+ link_map_nodelete_active,
+
+ /* This link map cannot be deallocated after dlopen has succeded.
+ dlopen turns this into link_map_nodelete_active. dlclose treats
+ this intermediate state as link_map_nodelete_active. */
+ link_map_nodelete_pending,
+};
+
/* Structure describing a loaded shared object. The `l_next' and `l_prev'
members form a chain of all the shared objects loaded at startup.
@@ -203,6 +218,10 @@
freed, ie. not allocated with
the dummy malloc in ld.so. */
+ /* Actually of type enum link_map_nodelete. Separate byte due to
+ concurrent access. Only valid for l_type == lt_loaded. */
+ unsigned char l_nodelete;
+
#include <link_map.h>
/* Collected information about own RPATH directories. */
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ib2a3d86af6f92d75baca65431d74783ee0dbc292
Gerrit-Change-Number: 471
Gerrit-PatchSet: 1
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-MessageType: newchange
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [review] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
2019-10-31 19:20 [review] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839] Florian Weimer (Code Review)
@ 2019-10-31 20:18 ` Christian Häggström (Code Review)
2019-11-13 12:56 ` [review v2] " Florian Weimer (Code Review)
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Christian Häggström (Code Review) @ 2019-10-31 20:18 UTC (permalink / raw)
To: Florian Weimer, libc-alpha
Christian Häggström has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471
......................................................................
Patch Set 1:
(3 comments)
I haven't tested the patch, just some small remarks on the code.
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471/1/elf/dl-lookup.c
File elf/dl-lookup.c:
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471/1/elf/dl-lookup.c@322
PS1, Line 322:
317 | setting the appropriate flag. */
318 | if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_BINDINGS)
319 | && map->l_nodelete == link_map_nodelete_inactive)
320 | _dl_debug_printf ("\
321 | marking %s [%lu] as NODELETE due to unique symbol\n",
322 > map->l_name, map->l_ns);
323 | if (flags & DL_LOOKUP_FOR_RELOCATE)
324 | map->l_nodelete = link_map_nodelete_pending;
325 | else
326 | map->l_nodelete = link_map_nodelete_active;
327 | }
Please make a macro for the debug printouts to reduce the clutter.
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471/1/include/link.h
File include/link.h:
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471/1/include/link.h@94
PS1, Line 94:
83 | enum link_map_nodelete
| ...
89 | link_map_nodelete_active,
90 |
91 | /* This link map cannot be deallocated after dlopen has succeded.
92 | dlopen turns this into link_map_nodelete_active. dlclose treats
93 | this intermediate state as link_map_nodelete_active. */
94 > link_map_nodelete_pending,
95 | };
96 |
97 |
98 | /* Structure describing a loaded shared object. The `l_next' and `l_prev'
99 | members form a chain of all the shared objects loaded at startup.
I would prefer if the constants were in caps, like LINK_MAP_NODELETE_INACTIVE, right now they look like global variables when reading the code.
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471/1/include/link.h@223
PS1, Line 223:
107 | struct link_map
| ...
218 | freed, ie. not allocated with
219 | the dummy malloc in ld.so. */
220 |
221 | /* Actually of type enum link_map_nodelete. Separate byte due to
222 | concurrent access. Only valid for l_type == lt_loaded. */
223 > unsigned char l_nodelete;
224 |
225 | #include <link_map.h>
226 |
227 | /* Collected information about own RPATH directories. */
228 | struct r_search_path_struct l_rpath_dirs;
Is this variable accessed from different threads? In that case, please mention which lock that needs to be held in order to access this variable.
Please initialize it explicitly to link_map_nodelete_inactive in _dl_new_object(). Or write link_map_nodelete_inactive = 0 in the enum definition to clarify that we depend on calloc's zero initialization.
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ib2a3d86af6f92d75baca65431d74783ee0dbc292
Gerrit-Change-Number: 471
Gerrit-PatchSet: 1
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-CC: Christian Häggström <gnugerrit@kalvdans.no-ip.org>
Gerrit-Comment-Date: Thu, 31 Oct 2019 20:18:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 12+ messages in thread
* [review v2] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
2019-10-31 19:20 [review] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839] Florian Weimer (Code Review)
2019-10-31 20:18 ` Christian Häggström (Code Review)
@ 2019-11-13 12:56 ` Florian Weimer (Code Review)
2019-11-13 14:28 ` Christian Häggström (Code Review)
` (3 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Florian Weimer (Code Review) @ 2019-11-13 12:56 UTC (permalink / raw)
To: Florian Weimer, Florian Weimer, libc-alpha; +Cc: Christian Häggström
Florian Weimer has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471
......................................................................
Patch Set 2:
(3 comments)
Thanks, I pushed a new version.
| --- elf/dl-lookup.c
| +++ elf/dl-lookup.c
| @@ -315,6 +315,17 @@ #define INITIAL_NUNIQUE_SYM_TABLE 31
| - setting the appropriate flag. */
| - ((struct link_map *) map)->l_flags_1 |= DF_1_NODELETE;
| + {
| + /* Make sure we don't unload this object by
| + setting the appropriate flag. */
| + if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_BINDINGS)
| + && map->l_nodelete == link_map_nodelete_inactive)
| + _dl_debug_printf ("\
| +marking %s [%lu] as NODELETE due to unique symbol\n",
| + map->l_name, map->l_ns);
PS1, Line 322:
Which part do you mean? The current code uses this style
(_dl_debug_printf in an if statement with a flag check). Changing that
would be a separate cleanup.
| + if (flags & DL_LOOKUP_FOR_RELOCATE)
| + map->l_nodelete = link_map_nodelete_pending;
| + else
| + map->l_nodelete = link_map_nodelete_active;
| + }
| }
| ++tab->n_elements;
|
| #ifdef SHARED
| --- include/link.h
| +++ include/link.h
| @@ -82,7 +85,19 @@ enum link_map_nodelete
| + /* This link map can be deallocated. */
| + link_map_nodelete_inactive,
| +
| + /* This link map cannot be deallocated. */
| + link_map_nodelete_active,
| +
| + /* This link map cannot be deallocated after dlopen has succeded.
| + dlopen turns this into link_map_nodelete_active. dlclose treats
| + this intermediate state as link_map_nodelete_active. */
| + link_map_nodelete_pending,
PS1, Line 94:
The existing code uses both styles, see lt_executable (also in struct
link_map) and DL_LOOKUP_ADD_DEPENDENCY. So I'm not sure what to do
here.
| +};
| +
|
| /* Structure describing a loaded shared object. The `l_next' and `l_prev'
| members form a chain of all the shared objects loaded at startup.
|
| These data structures exist in space used by the run-time dynamic linker;
| modifying them may have disastrous results.
|
...
| @@ -199,15 +214,19 @@ struct link_map
| during LD_TRACE_PRELINKING=1
| contains any DT_SYMBOLIC
| libraries. */
| unsigned int l_free_initfini:1; /* Nonzero if l_initfini can be
| freed, ie. not allocated with
| the dummy malloc in ld.so. */
|
| + /* Actually of type enum link_map_nodelete. Separate byte due to
| + concurrent access. Only valid for l_type == lt_loaded. */
| + unsigned char l_nodelete;
PS1, Line 223:
There is a read of this variable in add_dependency in elf/dl-lookup.c,
for a double-checked locking pattern (sort of). Link map updates
happen under the loader lock, but there are some cases where we read
things outside the lock (see l_scope, THREAD_GSCOPE_* etc.). This
concurrency pattern is not new, before this change we used l_flags_1
in a similar, technically unsound way.
Anyway, I updated the comment.
I could switch this access to a relaxed MO load if I made the variable
uint32_t. (We do not have one-byte atomics.)
I added a comment an initializer to link_map_nodelete_inactive.
Thanks.
| +
| #include <link_map.h>
|
| /* Collected information about own RPATH directories. */
| struct r_search_path_struct l_rpath_dirs;
|
| /* Collected results of relocation while profiling. */
| struct reloc_result
| {
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ib2a3d86af6f92d75baca65431d74783ee0dbc292
Gerrit-Change-Number: 471
Gerrit-PatchSet: 2
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Florian Weimer <fw@deneb.enyo.de>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-CC: Christian Häggström <gnugerrit@kalvdans.no-ip.org>
Gerrit-Comment-Date: Wed, 13 Nov 2019 12:56:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Christian Häggström <gnugerrit@kalvdans.no-ip.org>
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 12+ messages in thread
* [review v2] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
2019-10-31 19:20 [review] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839] Florian Weimer (Code Review)
2019-10-31 20:18 ` Christian Häggström (Code Review)
2019-11-13 12:56 ` [review v2] " Florian Weimer (Code Review)
@ 2019-11-13 14:28 ` Christian Häggström (Code Review)
2019-11-15 16:02 ` [review v3] " Florian Weimer (Code Review)
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Christian Häggström (Code Review) @ 2019-11-13 14:28 UTC (permalink / raw)
To: Florian Weimer, Florian Weimer, libc-alpha
Christian Häggström has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471
......................................................................
Patch Set 2: Code-Review+1
(3 comments)
| --- elf/dl-lookup.c
| +++ elf/dl-lookup.c
| @@ -315,6 +315,17 @@ #define INITIAL_NUNIQUE_SYM_TABLE 31
| - setting the appropriate flag. */
| - ((struct link_map *) map)->l_flags_1 |= DF_1_NODELETE;
| + {
| + /* Make sure we don't unload this object by
| + setting the appropriate flag. */
| + if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_BINDINGS)
| + && map->l_nodelete == link_map_nodelete_inactive)
| + _dl_debug_printf ("\
| +marking %s [%lu] as NODELETE due to unique symbol\n",
| + map->l_name, map->l_ns);
PS1, Line 322:
Yes, I mean to wrap both the if statement and _dl_debug_printf in a
macro. But I agree it is a separate cleanup. Marking as resolved.
| + if (flags & DL_LOOKUP_FOR_RELOCATE)
| + map->l_nodelete = link_map_nodelete_pending;
| + else
| + map->l_nodelete = link_map_nodelete_active;
| + }
| }
| ++tab->n_elements;
|
| #ifdef SHARED
| --- include/link.h
| +++ include/link.h
| @@ -82,7 +85,19 @@ enum link_map_nodelete
| + /* This link map can be deallocated. */
| + link_map_nodelete_inactive,
| +
| + /* This link map cannot be deallocated. */
| + link_map_nodelete_active,
| +
| + /* This link map cannot be deallocated after dlopen has succeded.
| + dlopen turns this into link_map_nodelete_active. dlclose treats
| + this intermediate state as link_map_nodelete_active. */
| + link_map_nodelete_pending,
PS1, Line 94:
Ok, leave it in lowercase and we can make a separate patch changing
all existing constants to caps. Resolving thread.
| +};
| +
|
| /* Structure describing a loaded shared object. The `l_next' and `l_prev'
| members form a chain of all the shared objects loaded at startup.
|
| These data structures exist in space used by the run-time dynamic linker;
| modifying them may have disastrous results.
|
...
| @@ -199,15 +214,19 @@ struct link_map
| during LD_TRACE_PRELINKING=1
| contains any DT_SYMBOLIC
| libraries. */
| unsigned int l_free_initfini:1; /* Nonzero if l_initfini can be
| freed, ie. not allocated with
| the dummy malloc in ld.so. */
|
| + /* Actually of type enum link_map_nodelete. Separate byte due to
| + concurrent access. Only valid for l_type == lt_loaded. */
| + unsigned char l_nodelete;
PS1, Line 223:
Uh, reads to concurrent variables with unspecified memory ordering is
a ticking bomb as the compilers gets smarter. But that's a separate
issue. Glad you have updated the comment.
Thanks for commenting the initializer value. Marking resolved.
| +
| #include <link_map.h>
|
| /* Collected information about own RPATH directories. */
| struct r_search_path_struct l_rpath_dirs;
|
| /* Collected results of relocation while profiling. */
| struct reloc_result
| {
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ib2a3d86af6f92d75baca65431d74783ee0dbc292
Gerrit-Change-Number: 471
Gerrit-PatchSet: 2
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Christian Häggström <gnugerrit@kalvdans.no-ip.org>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Comment-Date: Wed, 13 Nov 2019 14:28:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Florian Weimer <fweimer@redhat.com>
Comment-In-Reply-To: Christian Häggström <gnugerrit@kalvdans.no-ip.org>
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 12+ messages in thread
* [review v3] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
2019-10-31 19:20 [review] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839] Florian Weimer (Code Review)
` (2 preceding siblings ...)
2019-11-13 14:28 ` Christian Häggström (Code Review)
@ 2019-11-15 16:02 ` Florian Weimer (Code Review)
2019-12-02 16:07 ` Florian Weimer
2019-11-21 12:57 ` Carlos O'Donell (Code Review)
2019-11-27 20:30 ` [pushed] " Sourceware to Gerrit sync (Code Review)
5 siblings, 1 reply; 12+ messages in thread
From: Florian Weimer (Code Review) @ 2019-11-15 16:02 UTC (permalink / raw)
To: Florian Weimer, Christian Häggström, libc-alpha
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471
......................................................................
Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
This introduces a “pending NODELETE” state in the link map, which is
flipped to the persistent NODELETE state late in dlopen, via
activate_nodelete. During initial relocation, symbol binding
records pending NODELETE state only. dlclose ignores pending NODELETE
state. Taken together, this results that a partially completed dlopen
is rolled back completely because new NODELETE mappings are unloaded.
Tested on x86_64-linux-gnu and i386-linux-gnu.
Change-Id: Ib2a3d86af6f92d75baca65431d74783ee0dbc292
---
M elf/Makefile
M elf/dl-close.c
M elf/dl-lookup.c
M elf/dl-open.c
M elf/get-dynamic-info.h
A elf/tst-dlopenfail.c
A elf/tst-dlopenfaillinkmod.c
A elf/tst-dlopenfailmod1.c
A elf/tst-dlopenfailmod2.c
M include/link.h
10 files changed, 328 insertions(+), 38 deletions(-)
diff --git a/elf/Makefile b/elf/Makefile
index 571f12f..9296162 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -193,7 +193,7 @@
tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
tst-unwind-ctor tst-unwind-main tst-audit13 \
tst-sonamemove-link tst-sonamemove-dlopen tst-dlopen-tlsmodid \
- tst-dlopen-self tst-auditmany tst-initfinilazyfail
+ tst-dlopen-self tst-auditmany tst-initfinilazyfail tst-dlopenfail
# reldep9
tests-internal += loadtest unload unload2 circleload1 \
neededtest neededtest2 neededtest3 neededtest4 \
@@ -285,7 +285,8 @@
tst-auditmanymod1 tst-auditmanymod2 tst-auditmanymod3 \
tst-auditmanymod4 tst-auditmanymod5 tst-auditmanymod6 \
tst-auditmanymod7 tst-auditmanymod8 tst-auditmanymod9 \
- tst-initlazyfailmod tst-finilazyfailmod
+ tst-initlazyfailmod tst-finilazyfailmod \
+ tst-dlopenfailmod1 tst-dlopenfaillinkmod tst-dlopenfailmod2
# Most modules build with _ISOMAC defined, but those filtered out
# depend on internal headers.
modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\
@@ -1591,3 +1592,13 @@
-Wl,-z,lazy -Wl,--unresolved-symbols=ignore-all
LDFLAGS-tst-finilazyfailmod.so = \
-Wl,-z,lazy -Wl,--unresolved-symbols=ignore-all
+
+$(objpfx)tst-dlopenfail: $(libdl)
+$(objpfx)tst-dlopenfail.out: \
+ $(objpfx)tst-dlopenfailmod1.so $(objpfx)tst-dlopenfailmod2.so
+# Order matters here. tst-dlopenfaillinkmod.so's soname ensures
+# a run-time loader failure.
+$(objpfx)tst-dlopenfailmod1.so: \
+ $(shared-thread-library) $(objpfx)tst-dlopenfaillinkmod.so
+LDFLAGS-tst-dlopenfaillinkmod.so = -Wl,-soname,tst-dlopenfail-missingmod.so
+$(objpfx)tst-dlopenfailmod2.so: $(shared-thread-library)
diff --git a/elf/dl-close.c b/elf/dl-close.c
index 33486b9..e35a62d 100644
--- a/elf/dl-close.c
+++ b/elf/dl-close.c
@@ -168,14 +168,6 @@
char done[nloaded];
struct link_map *maps[nloaded];
- /* Clear DF_1_NODELETE to force object deletion. We don't need to touch
- l_tls_dtor_count because forced object deletion only happens when an
- error occurs during object load. Destructor registration for TLS
- non-POD objects should not have happened till then for this
- object. */
- if (force)
- map->l_flags_1 &= ~DF_1_NODELETE;
-
/* Run over the list and assign indexes to the link maps and enter
them into the MAPS array. */
int idx = 0;
@@ -205,7 +197,7 @@
/* Check whether this object is still used. */
if (l->l_type == lt_loaded
&& l->l_direct_opencount == 0
- && (l->l_flags_1 & DF_1_NODELETE) == 0
+ && l->l_nodelete != link_map_nodelete_active
/* See CONCURRENCY NOTES in cxa_thread_atexit_impl.c to know why
acquire is sufficient and correct. */
&& atomic_load_acquire (&l->l_tls_dtor_count) == 0
@@ -288,7 +280,7 @@
if (!used[i])
{
assert (imap->l_type == lt_loaded
- && (imap->l_flags_1 & DF_1_NODELETE) == 0);
+ && imap->l_nodelete != link_map_nodelete_active);
/* Call its termination function. Do not do it for
half-cooked objects. Temporarily disable exception
@@ -838,7 +830,7 @@
before we took the lock. There is no way to detect this (see below)
so we proceed assuming this isn't the case. First see whether we
can remove the object at all. */
- if (__glibc_unlikely (map->l_flags_1 & DF_1_NODELETE))
+ if (__glibc_unlikely (map->l_nodelete == link_map_nodelete_active))
{
/* Nope. Do nothing. */
__rtld_lock_unlock_recursive (GL(dl_load_lock));
diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
index aaaf437..a2e85a5 100644
--- a/elf/dl-lookup.c
+++ b/elf/dl-lookup.c
@@ -192,9 +192,10 @@
Return the matching symbol in RESULT. */
static void
do_lookup_unique (const char *undef_name, uint_fast32_t new_hash,
- const struct link_map *map, struct sym_val *result,
+ struct link_map *map, struct sym_val *result,
int type_class, const ElfW(Sym) *sym, const char *strtab,
- const ElfW(Sym) *ref, const struct link_map *undef_map)
+ const ElfW(Sym) *ref, const struct link_map *undef_map,
+ int flags)
{
/* We have to determine whether we already found a symbol with this
name before. If not then we have to add it to the search table.
@@ -222,7 +223,7 @@
copy from the copy addressed through the
relocation. */
result->s = sym;
- result->m = (struct link_map *) map;
+ result->m = map;
}
else
{
@@ -311,9 +312,19 @@
new_hash, strtab + sym->st_name, sym, map);
if (map->l_type == lt_loaded)
- /* Make sure we don't unload this object by
- setting the appropriate flag. */
- ((struct link_map *) map)->l_flags_1 |= DF_1_NODELETE;
+ {
+ /* Make sure we don't unload this object by
+ setting the appropriate flag. */
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_BINDINGS)
+ && map->l_nodelete == link_map_nodelete_inactive)
+ _dl_debug_printf ("\
+marking %s [%lu] as NODELETE due to unique symbol\n",
+ map->l_name, map->l_ns);
+ if (flags & DL_LOOKUP_FOR_RELOCATE)
+ map->l_nodelete = link_map_nodelete_pending;
+ else
+ map->l_nodelete = link_map_nodelete_active;
+ }
}
++tab->n_elements;
@@ -525,8 +536,9 @@
return 1;
case STB_GNU_UNIQUE:;
- do_lookup_unique (undef_name, new_hash, map, result, type_class,
- sym, strtab, ref, undef_map);
+ do_lookup_unique (undef_name, new_hash, (struct link_map *) map,
+ result, type_class, sym, strtab, ref,
+ undef_map, flags);
return 1;
default:
@@ -568,9 +580,13 @@
if (undef_map == map)
return 0;
- /* Avoid references to objects which cannot be unloaded anyway. */
+ /* Avoid references to objects which cannot be unloaded anyway. We
+ do not need to record dependencies if this object goes away
+ during dlopen failure, either. IFUNC resolvers with relocation
+ dependencies may pick an dependency which can be dlclose'd, but
+ such IFUNC resolvers are undefined anyway. */
assert (map->l_type == lt_loaded);
- if ((map->l_flags_1 & DF_1_NODELETE) != 0)
+ if (map->l_nodelete != link_map_nodelete_inactive)
return 0;
struct link_map_reldeps *l_reldeps
@@ -678,16 +694,33 @@
/* Redo the NODELETE check, as when dl_load_lock wasn't held
yet this could have changed. */
- if ((map->l_flags_1 & DF_1_NODELETE) != 0)
+ if (map->l_nodelete != link_map_nodelete_inactive)
goto out;
/* If the object with the undefined reference cannot be removed ever
just make sure the same is true for the object which contains the
definition. */
if (undef_map->l_type != lt_loaded
- || (undef_map->l_flags_1 & DF_1_NODELETE) != 0)
+ || (undef_map->l_nodelete != link_map_nodelete_inactive))
{
- map->l_flags_1 |= DF_1_NODELETE;
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_BINDINGS)
+ && map->l_nodelete == link_map_nodelete_inactive)
+ {
+ if (undef_map->l_name[0] == '\0')
+ _dl_debug_printf ("\
+marking %s [%lu] as NODELETE due to reference to main program\n",
+ map->l_name, map->l_ns);
+ else
+ _dl_debug_printf ("\
+marking %s [%lu] as NODELETE due to reference to %s [%lu]\n",
+ map->l_name, map->l_ns,
+ undef_map->l_name, undef_map->l_ns);
+ }
+
+ if (flags & DL_LOOKUP_FOR_RELOCATE)
+ map->l_nodelete = link_map_nodelete_pending;
+ else
+ map->l_nodelete = link_map_nodelete_active;
goto out;
}
@@ -712,7 +745,18 @@
no fatal problem. We simply make sure the referenced object
cannot be unloaded. This is semantically the correct
behavior. */
- map->l_flags_1 |= DF_1_NODELETE;
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_BINDINGS)
+ && map->l_nodelete == link_map_nodelete_inactive)
+ _dl_debug_printf ("\
+marking %s [%lu] as NODELETE due to memory allocation failure\n",
+ map->l_name, map->l_ns);
+ if (flags & DL_LOOKUP_FOR_RELOCATE)
+ /* In case of non-lazy binding, we could actually
+ report the memory allocation error, but for now, we
+ use the conservative approximation as well. */
+ map->l_nodelete = link_map_nodelete_pending;
+ else
+ map->l_nodelete = link_map_nodelete_active;
goto out;
}
else
diff --git a/elf/dl-open.c b/elf/dl-open.c
index 89954bd..db870ed 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -410,6 +410,40 @@
}
}
+/* Mark the objects as NODELETE if required. This is delayed until
+ after dlopen failure is not possible, so that _dl_close can clean
+ up objects if necessary. */
+static void
+activate_nodelete (struct link_map *new, int mode)
+{
+ if (mode & RTLD_NODELETE || new->l_nodelete == link_map_nodelete_pending)
+ {
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_FILES))
+ _dl_debug_printf ("activating NODELETE for %s [%lu]\n",
+ new->l_name, new->l_ns);
+ new->l_nodelete = link_map_nodelete_active;
+ }
+
+ for (unsigned int i = 0; i < new->l_searchlist.r_nlist; ++i)
+ {
+ struct link_map *imap = new->l_searchlist.r_list[i];
+ if (imap->l_nodelete == link_map_nodelete_pending)
+ {
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_FILES))
+ _dl_debug_printf ("activating NODELETE for %s [%lu]\n",
+ imap->l_name, imap->l_ns);
+
+ /* Only new objects should have set
+ link_map_nodelete_pending. Existing objects should not
+ have gained any new dependencies and therefore cannot
+ reach NODELETE status. */
+ assert (!imap->l_init_called || imap->l_type != lt_loaded);
+
+ imap->l_nodelete = link_map_nodelete_active;
+ }
+ }
+}
+
/* struct dl_init_args and call_dl_init are used to call _dl_init with
exception handling disabled. */
struct dl_init_args
@@ -479,12 +513,6 @@
return;
}
- /* Mark the object as not deletable if the RTLD_NODELETE flags was passed.
- Do this early so that we don't skip marking the object if it was
- already loaded. */
- if (__glibc_unlikely (mode & RTLD_NODELETE))
- new->l_flags_1 |= DF_1_NODELETE;
-
if (__glibc_unlikely (mode & __RTLD_SPROF))
/* This happens only if we load a DSO for 'sprof'. */
return;
@@ -500,19 +528,37 @@
_dl_debug_printf ("opening file=%s [%lu]; direct_opencount=%u\n\n",
new->l_name, new->l_ns, new->l_direct_opencount);
- /* If the user requested the object to be in the global namespace
- but it is not so far, add it now. */
+ /* If the user requested the object to be in the global
+ namespace but it is not so far, prepare to add it now. This
+ can raise an exception to do a malloc failure. */
if ((mode & RTLD_GLOBAL) && new->l_global == 0)
+ add_to_global_resize (new);
+
+ /* Mark the object as not deletable if the RTLD_NODELETE flags
+ was passed. */
+ if (__glibc_unlikely (mode & RTLD_NODELETE))
{
- add_to_global_resize (new);
- add_to_global_update (new);
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_FILES)
+ && new->l_nodelete == link_map_nodelete_inactive)
+ _dl_debug_printf ("marking %s [%lu] as NODELETE\n",
+ new->l_name, new->l_ns);
+ new->l_nodelete = link_map_nodelete_active;
}
+ /* Finalize the addition to the global scope. */
+ if ((mode & RTLD_GLOBAL) && new->l_global == 0)
+ add_to_global_update (new);
+
assert (_dl_debug_initialize (0, args->nsid)->r_state == RT_CONSISTENT);
return;
}
+ /* Schedule NODELETE marking for the directly loaded object if
+ requested. */
+ if (__glibc_unlikely (mode & RTLD_NODELETE))
+ new->l_nodelete = link_map_nodelete_pending;
+
/* Load that object's dependencies. */
_dl_map_object_deps (new, NULL, 0, 0,
mode & (__RTLD_DLOPEN | RTLD_DEEPBIND | __RTLD_AUDIT));
@@ -587,6 +633,14 @@
int relocation_in_progress = 0;
+ /* Perform relocation. This can trigger lazy binding in IFUNC
+ resolvers. For NODELETE mappings, these dependencies are not
+ recorded because the flag has not been applied to the newly
+ loaded objects. This means that upon dlopen failure, these
+ NODELETE objects can be unloaded despite existing references to
+ them. However, such relocation dependencies in IFUNC resolvers
+ are undefined anyway, so this is not a problem. */
+
for (unsigned int i = nmaps; i-- > 0; )
{
l = maps[i];
@@ -616,7 +670,7 @@
_dl_start_profile ();
/* Prevent unloading the object. */
- GL(dl_profile_map)->l_flags_1 |= DF_1_NODELETE;
+ GL(dl_profile_map)->l_nodelete = link_map_nodelete_active;
}
}
else
@@ -647,6 +701,8 @@
All memory allocations for new objects must have happened
before. */
+ activate_nodelete (new, mode);
+
/* Second stage after resize_scopes: Actually perform the scope
update. After this, dlsym and lazy binding can bind to new
objects. */
@@ -805,6 +861,10 @@
GL(dl_tls_dtv_gaps) = true;
_dl_close_worker (args.map, true);
+
+ /* All link_map_nodelete_pending objects should have been
+ deleted at this point, which is why it is not necessary
+ to reset the flag here. */
}
assert (_dl_debug_initialize (0, args.nsid)->r_state == RT_CONSISTENT);
diff --git a/elf/get-dynamic-info.h b/elf/get-dynamic-info.h
index af9dd57..075683d 100644
--- a/elf/get-dynamic-info.h
+++ b/elf/get-dynamic-info.h
@@ -163,6 +163,8 @@
if (info[VERSYMIDX (DT_FLAGS_1)] != NULL)
{
l->l_flags_1 = info[VERSYMIDX (DT_FLAGS_1)]->d_un.d_val;
+ if (l->l_flags_1 & DF_1_NODELETE)
+ l->l_nodelete = link_map_nodelete_pending;
/* Only DT_1_SUPPORTED_MASK bits are supported, and we would like
to assert this, but we can't. Users have been setting
diff --git a/elf/tst-dlopenfail.c b/elf/tst-dlopenfail.c
new file mode 100644
index 0000000..ce3140c
--- /dev/null
+++ b/elf/tst-dlopenfail.c
@@ -0,0 +1,79 @@
+/* Test dlopen rollback after failures involving NODELETE objects (bug 20839).
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <dlfcn.h>
+#include <errno.h>
+#include <gnu/lib-names.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/xdlfcn.h>
+
+static int
+do_test (void)
+{
+ /* This test uses libpthread as the canonical NODELETE module. If
+ libpthread is no longer NODELETE because it has been merged into
+ libc, the test needs to be updated. */
+ TEST_VERIFY (dlsym (NULL, "pthread_create") == NULL);
+
+ /* This is expected to fail because of the missing dependency. */
+ puts ("info: attempting to load tst-dlopenfailmod1.so");
+ TEST_VERIFY (dlopen ("tst-dlopenfailmod1.so", RTLD_LAZY) == NULL);
+ const char *message = dlerror ();
+ TEST_COMPARE_STRING (message,
+ "tst-dlopenfail-missingmod.so:"
+ " cannot open shared object file:"
+ " No such file or directory");
+
+ /* Do not probe for the presence of libpthread at this point because
+ that might trigger relocation if bug 20839 is present, obscuring
+ a subsequent crash. */
+
+ /* This is expected to succeed. */
+ puts ("info: loading tst-dlopenfailmod2.so");
+ void *handle = xdlopen ("tst-dlopenfailmod2.so", RTLD_NOW);
+ xdlclose (handle);
+
+ /* libpthread should remain loaded. */
+ TEST_VERIFY (dlopen (LIBPTHREAD_SO, RTLD_LAZY | RTLD_NOLOAD) != NULL);
+ TEST_VERIFY (dlsym (NULL, "pthread_create") == NULL);
+
+ /* We can make libpthread global, and then the symbol should become
+ available. */
+ TEST_VERIFY (dlopen (LIBPTHREAD_SO, RTLD_LAZY | RTLD_GLOBAL) != NULL);
+ TEST_VERIFY (dlsym (NULL, "pthread_create") != NULL);
+
+ /* sem_open is sufficiently complex to depend on relocations. */
+ void *(*sem_open_ptr) (const char *, int flag, ...)
+ = dlsym (NULL, "sem_open");
+ if (sem_open_ptr == NULL)
+ /* Hurd does not implement sem_open. */
+ puts ("warning: sem_open not found, further testing not possible");
+ else
+ {
+ errno = 0;
+ TEST_VERIFY (sem_open_ptr ("/", 0) == NULL);
+ TEST_COMPARE (errno, EINVAL);
+ }
+
+ return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/elf/tst-dlopenfaillinkmod.c b/elf/tst-dlopenfaillinkmod.c
new file mode 100644
index 0000000..3b14b02
--- /dev/null
+++ b/elf/tst-dlopenfaillinkmod.c
@@ -0,0 +1,17 @@
+/* Empty module with a soname which is not available at run time.
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
diff --git a/elf/tst-dlopenfailmod1.c b/elf/tst-dlopenfailmod1.c
new file mode 100644
index 0000000..6ef4882
--- /dev/null
+++ b/elf/tst-dlopenfailmod1.c
@@ -0,0 +1,36 @@
+/* Module which depends on two modules: one NODELETE, one missing.
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+/* Note: Due to the missing second module, this object cannot be
+ loaded at run time. */
+
+#include <pthread.h>
+#include <stdio.h>
+#include <unistd.h>
+
+/* Force linking against libpthread. */
+void *pthread_create_reference = pthread_create;
+
+/* The constructor will never be executed because the module cannot be
+ loaded. */
+static void __attribute__ ((constructor))
+init (void)
+{
+ puts ("tst-dlopenfailmod1 constructor executed");
+ _exit (1);
+}
diff --git a/elf/tst-dlopenfailmod2.c b/elf/tst-dlopenfailmod2.c
new file mode 100644
index 0000000..7d60038
--- /dev/null
+++ b/elf/tst-dlopenfailmod2.c
@@ -0,0 +1,29 @@
+/* Module which depends on on a NODELETE module, and can be loaded.
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <pthread.h>
+#include <stdio.h>
+
+/* Force linking against libpthread. */
+void *pthread_create_reference = pthread_create;
+
+static void __attribute__ ((constructor))
+init (void)
+{
+ puts ("info: tst-dlopenfailmod2.so constructor invoked");
+}
diff --git a/include/link.h b/include/link.h
index be52b97..2e771e4 100644
--- a/include/link.h
+++ b/include/link.h
@@ -79,6 +79,21 @@
int malloced;
};
+/* Type used by the l_nodelete member. */
+enum link_map_nodelete
+{
+ /* This link map can be deallocated. */
+ link_map_nodelete_inactive = 0, /* Zero-initialized in _dl_new_object. */
+
+ /* This link map cannot be deallocated. */
+ link_map_nodelete_active,
+
+ /* This link map cannot be deallocated after dlopen has succeded.
+ dlopen turns this into link_map_nodelete_active. dlclose treats
+ this intermediate state as link_map_nodelete_active. */
+ link_map_nodelete_pending,
+};
+
/* Structure describing a loaded shared object. The `l_next' and `l_prev'
members form a chain of all the shared objects loaded at startup.
@@ -203,6 +218,11 @@
freed, ie. not allocated with
the dummy malloc in ld.so. */
+ /* Actually of type enum link_map_nodelete. Separate byte due to
+ a read in add_dependency in elf/dl-lookup.c outside the loader
+ lock. Only valid for l_type == lt_loaded. */
+ unsigned char l_nodelete;
+
#include <link_map.h>
/* Collected information about own RPATH directories. */
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ib2a3d86af6f92d75baca65431d74783ee0dbc292
Gerrit-Change-Number: 471
Gerrit-PatchSet: 3
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Christian Häggström <gnugerrit@kalvdans.no-ip.org>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-MessageType: newpatchset
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [review v3] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
2019-10-31 19:20 [review] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839] Florian Weimer (Code Review)
` (3 preceding siblings ...)
2019-11-15 16:02 ` [review v3] " Florian Weimer (Code Review)
@ 2019-11-21 12:57 ` Carlos O'Donell (Code Review)
2019-11-27 20:30 ` [pushed] " Sourceware to Gerrit sync (Code Review)
5 siblings, 0 replies; 12+ messages in thread
From: Carlos O'Donell (Code Review) @ 2019-11-21 12:57 UTC (permalink / raw)
To: Florian Weimer, libc-alpha; +Cc: Christian Häggström
Carlos O'Donell has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471
......................................................................
Patch Set 3: Code-Review+2
(43 comments)
This change is complex to review because we touch so many aspects of NODELETE handling, but in the end many of the changes are somewhat mechanical. The clearly important aspects are when pending nodelete is turned into active nodelete. These changes look logically correct to me. The place where I ad one worry was the possibly failing malloc in add_dependency, but it is not a showstopper to getting better semantics for NODELETE handling and all of the debug printfs you add are immensely helpful in determining what is going on with the internal loader state and why an object won't be removed (this is very useful to C++ developers suffering from STB_GNU_UNIQUE issues and trying to figure out why a particular plugin won't be unloaded, so thanks).
I think this should go in immediately. I appreciate this going in now rather than much closer to the release window. We'll be able to get some Fedora Rawhide testing with this.
OK for master.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
| --- elf/Makefile
| +++ elf/Makefile
| @@ -188,18 +188,18 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
| tst-nodelete) \
| tst-initorder tst-initorder2 tst-relsort1 tst-null-argv \
| tst-tlsalign tst-tlsalign-extern tst-nodelete-opened \
| tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
| tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
| tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
| tst-unwind-ctor tst-unwind-main tst-audit13 \
| tst-sonamemove-link tst-sonamemove-dlopen tst-dlopen-tlsmodid \
| - tst-dlopen-self tst-auditmany tst-initfinilazyfail
| + tst-dlopen-self tst-auditmany tst-initfinilazyfail tst-dlopenfail
PS3, Line 196:
OK. New test case.
| # reldep9
| tests-internal += loadtest unload unload2 circleload1 \
| neededtest neededtest2 neededtest3 neededtest4 \
| tst-tls3 tst-tls6 tst-tls7 tst-tls8 tst-dlmopen2 \
| tst-ptrguard1 tst-stackguard1 tst-libc_dlvsym \
| tst-create_format1
| tests-container += tst-pldd tst-dlopen-tlsmodid-container \
| tst-dlopen-self-container
| test-srcs = tst-pathopt
...
| @@ -281,17 +281,18 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
| tst-main1mod tst-libc_dlvsym-dso tst-absolute-sym-lib \
| tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib \
| tst-audit13mod1 tst-sonamemove-linkmod1 \
| tst-sonamemove-runmod1 tst-sonamemove-runmod2 \
| tst-auditmanymod1 tst-auditmanymod2 tst-auditmanymod3 \
| tst-auditmanymod4 tst-auditmanymod5 tst-auditmanymod6 \
| tst-auditmanymod7 tst-auditmanymod8 tst-auditmanymod9 \
| - tst-initlazyfailmod tst-finilazyfailmod
| + tst-initlazyfailmod tst-finilazyfailmod \
| + tst-dlopenfailmod1 tst-dlopenfaillinkmod tst-dlopenfailmod2
PS3, Line 289:
OK. Two 3 new dsos.
| # Most modules build with _ISOMAC defined, but those filtered out
| # depend on internal headers.
| modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\
| $(modules-names))
|
| ifeq (yes,$(have-mtls-dialect-gnu2))
| tests += tst-gnu2-tls1
| modules-names += tst-gnu2-tls1mod
| $(objpfx)tst-gnu2-tls1: $(objpfx)tst-gnu2-tls1mod.so
...
| @@ -1594,0 +1595,10 @@ LDFLAGS-tst-finilazyfailmod.so = \
| +
| +$(objpfx)tst-dlopenfail: $(libdl)
| +$(objpfx)tst-dlopenfail.out: \
| + $(objpfx)tst-dlopenfailmod1.so $(objpfx)tst-dlopenfailmod2.so
| +# Order matters here. tst-dlopenfaillinkmod.so's soname ensures
| +# a run-time loader failure.
| +$(objpfx)tst-dlopenfailmod1.so: \
| + $(shared-thread-library) $(objpfx)tst-dlopenfaillinkmod.so
| +LDFLAGS-tst-dlopenfaillinkmod.so = -Wl,-soname,tst-dlopenfail-missingmod.so
| +$(objpfx)tst-dlopenfailmod2.so: $(shared-thread-library)
PS3, Line 1604:
OK. Setup the test case flags.
| --- elf/dl-close.c
| +++ elf/dl-close.c
| @@ -168,19 +168,11 @@ _dl_close_worker (struct link_map *map, bool force)
| char done[nloaded];
| struct link_map *maps[nloaded];
|
| - /* Clear DF_1_NODELETE to force object deletion. We don't need to touch
| - l_tls_dtor_count because forced object deletion only happens when an
| - error occurs during object load. Destructor registration for TLS
| - non-POD objects should not have happened till then for this
| - object. */
| - if (force)
| - map->l_flags_1 &= ~DF_1_NODELETE;
PS3, Line 177:
OK, remove clearing of DF_1_NODLETE in _dl_close_worker, this is all
going to be handled in a more structured way via l_nodelete.
| -
| /* Run over the list and assign indexes to the link maps and enter
| them into the MAPS array. */
| int idx = 0;
| for (struct link_map *l = ns->_ns_loaded; l != NULL; l = l->l_next)
| {
| l->l_idx = idx;
| maps[idx] = l;
| ++idx;
...
| @@ -200,18 +192,18 @@ _dl_close_worker (struct link_map *map, bool force)
|
| if (done[done_index])
| /* Already handled. */
| continue;
|
| /* Check whether this object is still used. */
| if (l->l_type == lt_loaded
| && l->l_direct_opencount == 0
| - && (l->l_flags_1 & DF_1_NODELETE) == 0
| + && l->l_nodelete != link_map_nodelete_active
PS3, Line 200:
OK. First check where instead of using l_flags_1 we use l_nodelete. We
check to see if nodelete semantics are active (rather than pending).
| /* See CONCURRENCY NOTES in cxa_thread_atexit_impl.c to know why
| acquire is sufficient and correct. */
| && atomic_load_acquire (&l->l_tls_dtor_count) == 0
| && !used[done_index])
| continue;
|
| /* We need this object and we handle it now. */
| done[done_index] = 1;
| used[done_index] = 1;
...
| @@ -283,18 +275,18 @@ #endif
| struct link_map *imap = maps[i];
|
| /* All elements must be in the same namespace. */
| assert (imap->l_ns == nsid);
|
| if (!used[i])
| {
| assert (imap->l_type == lt_loaded
| - && (imap->l_flags_1 & DF_1_NODELETE) == 0);
| + && imap->l_nodelete != link_map_nodelete_active);
PS3, Line 283:
OK. Likwise, check for nodelete active (rather than pending).
|
| /* Call its termination function. Do not do it for
| half-cooked objects. Temporarily disable exception
| handling, so that errors are fatal. */
| if (imap->l_init_called)
| {
| /* When debugging print a message first. */
| if (__builtin_expect (GLRO(dl_debug_mask) & DL_DEBUG_IMPCALLS,
| 0))
...
| @@ -833,18 +825,18 @@ _dl_close (void *_map)
| concurrent dlopens. */
| __rtld_lock_lock_recursive (GL(dl_load_lock));
|
| /* At this point we are guaranteed nobody else is touching the list of
| loaded maps, but a concurrent dlclose might have freed our map
| before we took the lock. There is no way to detect this (see below)
| so we proceed assuming this isn't the case. First see whether we
| can remove the object at all. */
| - if (__glibc_unlikely (map->l_flags_1 & DF_1_NODELETE))
| + if (__glibc_unlikely (map->l_nodelete == link_map_nodelete_active))
PS3, Line 833:
OK. Likewise check for nodelete active (rather than pending).
| {
| /* Nope. Do nothing. */
| __rtld_lock_unlock_recursive (GL(dl_load_lock));
| return;
| }
|
| /* At present this is an unreliable check except in the case where the
| caller has recursively called dlclose and we are sure the link map
| has not been freed. In a non-recursive dlclose the map itself
| --- elf/dl-lookup.c
| +++ elf/dl-lookup.c
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ib2a3d86af6f92d75baca65431d74783ee0dbc292
Gerrit-Change-Number: 471
Gerrit-PatchSet: 3
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Christian Häggström <gnugerrit@kalvdans.no-ip.org>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 12:57:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 12+ messages in thread
* [pushed] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
2019-10-31 19:20 [review] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839] Florian Weimer (Code Review)
` (4 preceding siblings ...)
2019-11-21 12:57 ` Carlos O'Donell (Code Review)
@ 2019-11-27 20:30 ` Sourceware to Gerrit sync (Code Review)
2019-12-02 10:06 ` Szabolcs Nagy
5 siblings, 1 reply; 12+ messages in thread
From: Sourceware to Gerrit sync (Code Review) @ 2019-11-27 20:30 UTC (permalink / raw)
To: Florian Weimer, libc-alpha
Cc: Carlos O'Donell, Christian Häggström
Sourceware to Gerrit sync has submitted this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471
......................................................................
Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
This introduces a “pending NODELETE” state in the link map, which is
flipped to the persistent NODELETE state late in dlopen, via
activate_nodelete. During initial relocation, symbol binding
records pending NODELETE state only. dlclose ignores pending NODELETE
state. Taken together, this results that a partially completed dlopen
is rolled back completely because new NODELETE mappings are unloaded.
Tested on x86_64-linux-gnu and i386-linux-gnu.
Change-Id: Ib2a3d86af6f92d75baca65431d74783ee0dbc292
---
M elf/Makefile
M elf/dl-close.c
M elf/dl-lookup.c
M elf/dl-open.c
M elf/get-dynamic-info.h
A elf/tst-dlopenfail.c
A elf/tst-dlopenfaillinkmod.c
A elf/tst-dlopenfailmod1.c
A elf/tst-dlopenfailmod2.c
M include/link.h
10 files changed, 328 insertions(+), 38 deletions(-)
Approvals:
Carlos O'Donell: Looks good to me, approved
diff --git a/elf/Makefile b/elf/Makefile
index 30c3896..305bed2 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -199,7 +199,7 @@
tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
tst-unwind-ctor tst-unwind-main tst-audit13 \
tst-sonamemove-link tst-sonamemove-dlopen tst-dlopen-tlsmodid \
- tst-dlopen-self tst-auditmany tst-initfinilazyfail
+ tst-dlopen-self tst-auditmany tst-initfinilazyfail tst-dlopenfail
# reldep9
tests-internal += loadtest unload unload2 circleload1 \
neededtest neededtest2 neededtest3 neededtest4 \
@@ -291,7 +291,8 @@
tst-auditmanymod1 tst-auditmanymod2 tst-auditmanymod3 \
tst-auditmanymod4 tst-auditmanymod5 tst-auditmanymod6 \
tst-auditmanymod7 tst-auditmanymod8 tst-auditmanymod9 \
- tst-initlazyfailmod tst-finilazyfailmod
+ tst-initlazyfailmod tst-finilazyfailmod \
+ tst-dlopenfailmod1 tst-dlopenfaillinkmod tst-dlopenfailmod2
# Most modules build with _ISOMAC defined, but those filtered out
# depend on internal headers.
modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\
@@ -1597,3 +1598,13 @@
-Wl,-z,lazy -Wl,--unresolved-symbols=ignore-all
LDFLAGS-tst-finilazyfailmod.so = \
-Wl,-z,lazy -Wl,--unresolved-symbols=ignore-all
+
+$(objpfx)tst-dlopenfail: $(libdl)
+$(objpfx)tst-dlopenfail.out: \
+ $(objpfx)tst-dlopenfailmod1.so $(objpfx)tst-dlopenfailmod2.so
+# Order matters here. tst-dlopenfaillinkmod.so's soname ensures
+# a run-time loader failure.
+$(objpfx)tst-dlopenfailmod1.so: \
+ $(shared-thread-library) $(objpfx)tst-dlopenfaillinkmod.so
+LDFLAGS-tst-dlopenfaillinkmod.so = -Wl,-soname,tst-dlopenfail-missingmod.so
+$(objpfx)tst-dlopenfailmod2.so: $(shared-thread-library)
diff --git a/elf/dl-close.c b/elf/dl-close.c
index 33486b9..e35a62d 100644
--- a/elf/dl-close.c
+++ b/elf/dl-close.c
@@ -168,14 +168,6 @@
char done[nloaded];
struct link_map *maps[nloaded];
- /* Clear DF_1_NODELETE to force object deletion. We don't need to touch
- l_tls_dtor_count because forced object deletion only happens when an
- error occurs during object load. Destructor registration for TLS
- non-POD objects should not have happened till then for this
- object. */
- if (force)
- map->l_flags_1 &= ~DF_1_NODELETE;
-
/* Run over the list and assign indexes to the link maps and enter
them into the MAPS array. */
int idx = 0;
@@ -205,7 +197,7 @@
/* Check whether this object is still used. */
if (l->l_type == lt_loaded
&& l->l_direct_opencount == 0
- && (l->l_flags_1 & DF_1_NODELETE) == 0
+ && l->l_nodelete != link_map_nodelete_active
/* See CONCURRENCY NOTES in cxa_thread_atexit_impl.c to know why
acquire is sufficient and correct. */
&& atomic_load_acquire (&l->l_tls_dtor_count) == 0
@@ -288,7 +280,7 @@
if (!used[i])
{
assert (imap->l_type == lt_loaded
- && (imap->l_flags_1 & DF_1_NODELETE) == 0);
+ && imap->l_nodelete != link_map_nodelete_active);
/* Call its termination function. Do not do it for
half-cooked objects. Temporarily disable exception
@@ -838,7 +830,7 @@
before we took the lock. There is no way to detect this (see below)
so we proceed assuming this isn't the case. First see whether we
can remove the object at all. */
- if (__glibc_unlikely (map->l_flags_1 & DF_1_NODELETE))
+ if (__glibc_unlikely (map->l_nodelete == link_map_nodelete_active))
{
/* Nope. Do nothing. */
__rtld_lock_unlock_recursive (GL(dl_load_lock));
diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
index aaaf437..a2e85a5 100644
--- a/elf/dl-lookup.c
+++ b/elf/dl-lookup.c
@@ -192,9 +192,10 @@
Return the matching symbol in RESULT. */
static void
do_lookup_unique (const char *undef_name, uint_fast32_t new_hash,
- const struct link_map *map, struct sym_val *result,
+ struct link_map *map, struct sym_val *result,
int type_class, const ElfW(Sym) *sym, const char *strtab,
- const ElfW(Sym) *ref, const struct link_map *undef_map)
+ const ElfW(Sym) *ref, const struct link_map *undef_map,
+ int flags)
{
/* We have to determine whether we already found a symbol with this
name before. If not then we have to add it to the search table.
@@ -222,7 +223,7 @@
copy from the copy addressed through the
relocation. */
result->s = sym;
- result->m = (struct link_map *) map;
+ result->m = map;
}
else
{
@@ -311,9 +312,19 @@
new_hash, strtab + sym->st_name, sym, map);
if (map->l_type == lt_loaded)
- /* Make sure we don't unload this object by
- setting the appropriate flag. */
- ((struct link_map *) map)->l_flags_1 |= DF_1_NODELETE;
+ {
+ /* Make sure we don't unload this object by
+ setting the appropriate flag. */
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_BINDINGS)
+ && map->l_nodelete == link_map_nodelete_inactive)
+ _dl_debug_printf ("\
+marking %s [%lu] as NODELETE due to unique symbol\n",
+ map->l_name, map->l_ns);
+ if (flags & DL_LOOKUP_FOR_RELOCATE)
+ map->l_nodelete = link_map_nodelete_pending;
+ else
+ map->l_nodelete = link_map_nodelete_active;
+ }
}
++tab->n_elements;
@@ -525,8 +536,9 @@
return 1;
case STB_GNU_UNIQUE:;
- do_lookup_unique (undef_name, new_hash, map, result, type_class,
- sym, strtab, ref, undef_map);
+ do_lookup_unique (undef_name, new_hash, (struct link_map *) map,
+ result, type_class, sym, strtab, ref,
+ undef_map, flags);
return 1;
default:
@@ -568,9 +580,13 @@
if (undef_map == map)
return 0;
- /* Avoid references to objects which cannot be unloaded anyway. */
+ /* Avoid references to objects which cannot be unloaded anyway. We
+ do not need to record dependencies if this object goes away
+ during dlopen failure, either. IFUNC resolvers with relocation
+ dependencies may pick an dependency which can be dlclose'd, but
+ such IFUNC resolvers are undefined anyway. */
assert (map->l_type == lt_loaded);
- if ((map->l_flags_1 & DF_1_NODELETE) != 0)
+ if (map->l_nodelete != link_map_nodelete_inactive)
return 0;
struct link_map_reldeps *l_reldeps
@@ -678,16 +694,33 @@
/* Redo the NODELETE check, as when dl_load_lock wasn't held
yet this could have changed. */
- if ((map->l_flags_1 & DF_1_NODELETE) != 0)
+ if (map->l_nodelete != link_map_nodelete_inactive)
goto out;
/* If the object with the undefined reference cannot be removed ever
just make sure the same is true for the object which contains the
definition. */
if (undef_map->l_type != lt_loaded
- || (undef_map->l_flags_1 & DF_1_NODELETE) != 0)
+ || (undef_map->l_nodelete != link_map_nodelete_inactive))
{
- map->l_flags_1 |= DF_1_NODELETE;
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_BINDINGS)
+ && map->l_nodelete == link_map_nodelete_inactive)
+ {
+ if (undef_map->l_name[0] == '\0')
+ _dl_debug_printf ("\
+marking %s [%lu] as NODELETE due to reference to main program\n",
+ map->l_name, map->l_ns);
+ else
+ _dl_debug_printf ("\
+marking %s [%lu] as NODELETE due to reference to %s [%lu]\n",
+ map->l_name, map->l_ns,
+ undef_map->l_name, undef_map->l_ns);
+ }
+
+ if (flags & DL_LOOKUP_FOR_RELOCATE)
+ map->l_nodelete = link_map_nodelete_pending;
+ else
+ map->l_nodelete = link_map_nodelete_active;
goto out;
}
@@ -712,7 +745,18 @@
no fatal problem. We simply make sure the referenced object
cannot be unloaded. This is semantically the correct
behavior. */
- map->l_flags_1 |= DF_1_NODELETE;
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_BINDINGS)
+ && map->l_nodelete == link_map_nodelete_inactive)
+ _dl_debug_printf ("\
+marking %s [%lu] as NODELETE due to memory allocation failure\n",
+ map->l_name, map->l_ns);
+ if (flags & DL_LOOKUP_FOR_RELOCATE)
+ /* In case of non-lazy binding, we could actually
+ report the memory allocation error, but for now, we
+ use the conservative approximation as well. */
+ map->l_nodelete = link_map_nodelete_pending;
+ else
+ map->l_nodelete = link_map_nodelete_active;
goto out;
}
else
diff --git a/elf/dl-open.c b/elf/dl-open.c
index 03aaff7..7415c09 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -424,6 +424,40 @@
}
}
+/* Mark the objects as NODELETE if required. This is delayed until
+ after dlopen failure is not possible, so that _dl_close can clean
+ up objects if necessary. */
+static void
+activate_nodelete (struct link_map *new, int mode)
+{
+ if (mode & RTLD_NODELETE || new->l_nodelete == link_map_nodelete_pending)
+ {
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_FILES))
+ _dl_debug_printf ("activating NODELETE for %s [%lu]\n",
+ new->l_name, new->l_ns);
+ new->l_nodelete = link_map_nodelete_active;
+ }
+
+ for (unsigned int i = 0; i < new->l_searchlist.r_nlist; ++i)
+ {
+ struct link_map *imap = new->l_searchlist.r_list[i];
+ if (imap->l_nodelete == link_map_nodelete_pending)
+ {
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_FILES))
+ _dl_debug_printf ("activating NODELETE for %s [%lu]\n",
+ imap->l_name, imap->l_ns);
+
+ /* Only new objects should have set
+ link_map_nodelete_pending. Existing objects should not
+ have gained any new dependencies and therefore cannot
+ reach NODELETE status. */
+ assert (!imap->l_init_called || imap->l_type != lt_loaded);
+
+ imap->l_nodelete = link_map_nodelete_active;
+ }
+ }
+}
+
/* struct dl_init_args and call_dl_init are used to call _dl_init with
exception handling disabled. */
struct dl_init_args
@@ -493,12 +527,6 @@
return;
}
- /* Mark the object as not deletable if the RTLD_NODELETE flags was passed.
- Do this early so that we don't skip marking the object if it was
- already loaded. */
- if (__glibc_unlikely (mode & RTLD_NODELETE))
- new->l_flags_1 |= DF_1_NODELETE;
-
if (__glibc_unlikely (mode & __RTLD_SPROF))
/* This happens only if we load a DSO for 'sprof'. */
return;
@@ -514,19 +542,37 @@
_dl_debug_printf ("opening file=%s [%lu]; direct_opencount=%u\n\n",
new->l_name, new->l_ns, new->l_direct_opencount);
- /* If the user requested the object to be in the global namespace
- but it is not so far, add it now. */
+ /* If the user requested the object to be in the global
+ namespace but it is not so far, prepare to add it now. This
+ can raise an exception to do a malloc failure. */
if ((mode & RTLD_GLOBAL) && new->l_global == 0)
+ add_to_global_resize (new);
+
+ /* Mark the object as not deletable if the RTLD_NODELETE flags
+ was passed. */
+ if (__glibc_unlikely (mode & RTLD_NODELETE))
{
- add_to_global_resize (new);
- add_to_global_update (new);
+ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_FILES)
+ && new->l_nodelete == link_map_nodelete_inactive)
+ _dl_debug_printf ("marking %s [%lu] as NODELETE\n",
+ new->l_name, new->l_ns);
+ new->l_nodelete = link_map_nodelete_active;
}
+ /* Finalize the addition to the global scope. */
+ if ((mode & RTLD_GLOBAL) && new->l_global == 0)
+ add_to_global_update (new);
+
assert (_dl_debug_initialize (0, args->nsid)->r_state == RT_CONSISTENT);
return;
}
+ /* Schedule NODELETE marking for the directly loaded object if
+ requested. */
+ if (__glibc_unlikely (mode & RTLD_NODELETE))
+ new->l_nodelete = link_map_nodelete_pending;
+
/* Load that object's dependencies. */
_dl_map_object_deps (new, NULL, 0, 0,
mode & (__RTLD_DLOPEN | RTLD_DEEPBIND | __RTLD_AUDIT));
@@ -601,6 +647,14 @@
int relocation_in_progress = 0;
+ /* Perform relocation. This can trigger lazy binding in IFUNC
+ resolvers. For NODELETE mappings, these dependencies are not
+ recorded because the flag has not been applied to the newly
+ loaded objects. This means that upon dlopen failure, these
+ NODELETE objects can be unloaded despite existing references to
+ them. However, such relocation dependencies in IFUNC resolvers
+ are undefined anyway, so this is not a problem. */
+
for (unsigned int i = nmaps; i-- > 0; )
{
l = maps[i];
@@ -630,7 +684,7 @@
_dl_start_profile ();
/* Prevent unloading the object. */
- GL(dl_profile_map)->l_flags_1 |= DF_1_NODELETE;
+ GL(dl_profile_map)->l_nodelete = link_map_nodelete_active;
}
}
else
@@ -661,6 +715,8 @@
All memory allocations for new objects must have happened
before. */
+ activate_nodelete (new, mode);
+
/* Second stage after resize_scopes: Actually perform the scope
update. After this, dlsym and lazy binding can bind to new
objects. */
@@ -820,6 +876,10 @@
GL(dl_tls_dtv_gaps) = true;
_dl_close_worker (args.map, true);
+
+ /* All link_map_nodelete_pending objects should have been
+ deleted at this point, which is why it is not necessary
+ to reset the flag here. */
}
assert (_dl_debug_initialize (0, args.nsid)->r_state == RT_CONSISTENT);
diff --git a/elf/get-dynamic-info.h b/elf/get-dynamic-info.h
index af9dd57..075683d 100644
--- a/elf/get-dynamic-info.h
+++ b/elf/get-dynamic-info.h
@@ -163,6 +163,8 @@
if (info[VERSYMIDX (DT_FLAGS_1)] != NULL)
{
l->l_flags_1 = info[VERSYMIDX (DT_FLAGS_1)]->d_un.d_val;
+ if (l->l_flags_1 & DF_1_NODELETE)
+ l->l_nodelete = link_map_nodelete_pending;
/* Only DT_1_SUPPORTED_MASK bits are supported, and we would like
to assert this, but we can't. Users have been setting
diff --git a/elf/tst-dlopenfail.c b/elf/tst-dlopenfail.c
new file mode 100644
index 0000000..ce3140c
--- /dev/null
+++ b/elf/tst-dlopenfail.c
@@ -0,0 +1,79 @@
+/* Test dlopen rollback after failures involving NODELETE objects (bug 20839).
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <dlfcn.h>
+#include <errno.h>
+#include <gnu/lib-names.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/xdlfcn.h>
+
+static int
+do_test (void)
+{
+ /* This test uses libpthread as the canonical NODELETE module. If
+ libpthread is no longer NODELETE because it has been merged into
+ libc, the test needs to be updated. */
+ TEST_VERIFY (dlsym (NULL, "pthread_create") == NULL);
+
+ /* This is expected to fail because of the missing dependency. */
+ puts ("info: attempting to load tst-dlopenfailmod1.so");
+ TEST_VERIFY (dlopen ("tst-dlopenfailmod1.so", RTLD_LAZY) == NULL);
+ const char *message = dlerror ();
+ TEST_COMPARE_STRING (message,
+ "tst-dlopenfail-missingmod.so:"
+ " cannot open shared object file:"
+ " No such file or directory");
+
+ /* Do not probe for the presence of libpthread at this point because
+ that might trigger relocation if bug 20839 is present, obscuring
+ a subsequent crash. */
+
+ /* This is expected to succeed. */
+ puts ("info: loading tst-dlopenfailmod2.so");
+ void *handle = xdlopen ("tst-dlopenfailmod2.so", RTLD_NOW);
+ xdlclose (handle);
+
+ /* libpthread should remain loaded. */
+ TEST_VERIFY (dlopen (LIBPTHREAD_SO, RTLD_LAZY | RTLD_NOLOAD) != NULL);
+ TEST_VERIFY (dlsym (NULL, "pthread_create") == NULL);
+
+ /* We can make libpthread global, and then the symbol should become
+ available. */
+ TEST_VERIFY (dlopen (LIBPTHREAD_SO, RTLD_LAZY | RTLD_GLOBAL) != NULL);
+ TEST_VERIFY (dlsym (NULL, "pthread_create") != NULL);
+
+ /* sem_open is sufficiently complex to depend on relocations. */
+ void *(*sem_open_ptr) (const char *, int flag, ...)
+ = dlsym (NULL, "sem_open");
+ if (sem_open_ptr == NULL)
+ /* Hurd does not implement sem_open. */
+ puts ("warning: sem_open not found, further testing not possible");
+ else
+ {
+ errno = 0;
+ TEST_VERIFY (sem_open_ptr ("/", 0) == NULL);
+ TEST_COMPARE (errno, EINVAL);
+ }
+
+ return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/elf/tst-dlopenfaillinkmod.c b/elf/tst-dlopenfaillinkmod.c
new file mode 100644
index 0000000..3b14b02
--- /dev/null
+++ b/elf/tst-dlopenfaillinkmod.c
@@ -0,0 +1,17 @@
+/* Empty module with a soname which is not available at run time.
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
diff --git a/elf/tst-dlopenfailmod1.c b/elf/tst-dlopenfailmod1.c
new file mode 100644
index 0000000..6ef4882
--- /dev/null
+++ b/elf/tst-dlopenfailmod1.c
@@ -0,0 +1,36 @@
+/* Module which depends on two modules: one NODELETE, one missing.
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+/* Note: Due to the missing second module, this object cannot be
+ loaded at run time. */
+
+#include <pthread.h>
+#include <stdio.h>
+#include <unistd.h>
+
+/* Force linking against libpthread. */
+void *pthread_create_reference = pthread_create;
+
+/* The constructor will never be executed because the module cannot be
+ loaded. */
+static void __attribute__ ((constructor))
+init (void)
+{
+ puts ("tst-dlopenfailmod1 constructor executed");
+ _exit (1);
+}
diff --git a/elf/tst-dlopenfailmod2.c b/elf/tst-dlopenfailmod2.c
new file mode 100644
index 0000000..7d60038
--- /dev/null
+++ b/elf/tst-dlopenfailmod2.c
@@ -0,0 +1,29 @@
+/* Module which depends on on a NODELETE module, and can be loaded.
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <pthread.h>
+#include <stdio.h>
+
+/* Force linking against libpthread. */
+void *pthread_create_reference = pthread_create;
+
+static void __attribute__ ((constructor))
+init (void)
+{
+ puts ("info: tst-dlopenfailmod2.so constructor invoked");
+}
diff --git a/include/link.h b/include/link.h
index be52b97..2e771e4 100644
--- a/include/link.h
+++ b/include/link.h
@@ -79,6 +79,21 @@
int malloced;
};
+/* Type used by the l_nodelete member. */
+enum link_map_nodelete
+{
+ /* This link map can be deallocated. */
+ link_map_nodelete_inactive = 0, /* Zero-initialized in _dl_new_object. */
+
+ /* This link map cannot be deallocated. */
+ link_map_nodelete_active,
+
+ /* This link map cannot be deallocated after dlopen has succeded.
+ dlopen turns this into link_map_nodelete_active. dlclose treats
+ this intermediate state as link_map_nodelete_active. */
+ link_map_nodelete_pending,
+};
+
/* Structure describing a loaded shared object. The `l_next' and `l_prev'
members form a chain of all the shared objects loaded at startup.
@@ -203,6 +218,11 @@
freed, ie. not allocated with
the dummy malloc in ld.so. */
+ /* Actually of type enum link_map_nodelete. Separate byte due to
+ a read in add_dependency in elf/dl-lookup.c outside the loader
+ lock. Only valid for l_type == lt_loaded. */
+ unsigned char l_nodelete;
+
#include <link_map.h>
/* Collected information about own RPATH directories. */
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ib2a3d86af6f92d75baca65431d74783ee0dbc292
Gerrit-Change-Number: 471
Gerrit-PatchSet: 6
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Christian Häggström <gnugerrit@kalvdans.no-ip.org>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-MessageType: merged
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [pushed] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
2019-11-27 20:30 ` [pushed] " Sourceware to Gerrit sync (Code Review)
@ 2019-12-02 10:06 ` Szabolcs Nagy
2019-12-02 12:17 ` Florian Weimer
0 siblings, 1 reply; 12+ messages in thread
From: Szabolcs Nagy @ 2019-12-02 10:06 UTC (permalink / raw)
To: noreply@gnutoolchain-gerrit.osci.io, fweimer@redhat.com,
libc-alpha@sourceware.org, carlos@redhat.com,
gnugerrit@kalvdans.no-ip.org,
Sourceware to Gerrit sync (Code Review)
Cc: nd
On 27/11/2019 20:30, Sourceware to Gerrit sync (Code Review) wrote:
> Sourceware to Gerrit sync has submitted this change.
>
> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471
> ......................................................................
>
> Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
>
> This introduces a “pending NODELETE” state in the link map, which is
> flipped to the persistent NODELETE state late in dlopen, via
> activate_nodelete. During initial relocation, symbol binding
> records pending NODELETE state only. dlclose ignores pending NODELETE
> state. Taken together, this results that a partially completed dlopen
> is rolled back completely because new NODELETE mappings are unloaded.
>
> Tested on x86_64-linux-gnu and i386-linux-gnu.
>
> Change-Id: Ib2a3d86af6f92d75baca65431d74783ee0dbc292
> ---
> M elf/Makefile
> M elf/dl-close.c
> M elf/dl-lookup.c
> M elf/dl-open.c
> M elf/get-dynamic-info.h
> A elf/tst-dlopenfail.c
> A elf/tst-dlopenfaillinkmod.c
> A elf/tst-dlopenfailmod1.c
> A elf/tst-dlopenfailmod2.c
> M include/link.h
> 10 files changed, 328 insertions(+), 38 deletions(-)
>
> Approvals:
> Carlos O'Donell: Looks good to me, approved
i see
FAIL: elf/tst-dlopenfail
and
$ cat elf/tst-dlopenfail.out
info: attempting to load tst-dlopenfailmod1.so
tst-dlopenfailmod1 constructor executed
on the aarch64 buildbot
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [pushed] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
2019-12-02 10:06 ` Szabolcs Nagy
@ 2019-12-02 12:17 ` Florian Weimer
2019-12-02 13:52 ` Szabolcs Nagy
0 siblings, 1 reply; 12+ messages in thread
From: Florian Weimer @ 2019-12-02 12:17 UTC (permalink / raw)
To: Szabolcs Nagy
Cc: libc-alpha@sourceware.org, carlos@redhat.com,
gnugerrit@kalvdans.no-ip.org, nd
* Szabolcs Nagy:
> On 27/11/2019 20:30, Sourceware to Gerrit sync (Code Review) wrote:
>> Sourceware to Gerrit sync has submitted this change.
>>
>> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471
>> ......................................................................
>>
>> Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
>>
>> This introduces a “pending NODELETE” state in the link map, which is
>> flipped to the persistent NODELETE state late in dlopen, via
>> activate_nodelete. During initial relocation, symbol binding
>> records pending NODELETE state only. dlclose ignores pending NODELETE
>> state. Taken together, this results that a partially completed dlopen
>> is rolled back completely because new NODELETE mappings are unloaded.
>>
>> Tested on x86_64-linux-gnu and i386-linux-gnu.
>>
>> Change-Id: Ib2a3d86af6f92d75baca65431d74783ee0dbc292
>> ---
>> M elf/Makefile
>> M elf/dl-close.c
>> M elf/dl-lookup.c
>> M elf/dl-open.c
>> M elf/get-dynamic-info.h
>> A elf/tst-dlopenfail.c
>> A elf/tst-dlopenfaillinkmod.c
>> A elf/tst-dlopenfailmod1.c
>> A elf/tst-dlopenfailmod2.c
>> M include/link.h
>> 10 files changed, 328 insertions(+), 38 deletions(-)
>>
>> Approvals:
>> Carlos O'Donell: Looks good to me, approved
>
> i see
>
> FAIL: elf/tst-dlopenfail
>
> and
>
> $ cat elf/tst-dlopenfail.out
> info: attempting to load tst-dlopenfailmod1.so
> tst-dlopenfailmod1 constructor executed
>
> on the aarch64 buildbot
It's likely that your toolchain defaults to --no-as-needed. Does this
patch fix things for you?
diff --git a/elf/Makefile b/elf/Makefile
index f825354d3f..0debea7759 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -1618,8 +1618,11 @@ LDFLAGS-tst-finilazyfailmod.so = \
$(objpfx)tst-dlopenfail: $(libdl)
$(objpfx)tst-dlopenfail.out: \
$(objpfx)tst-dlopenfailmod1.so $(objpfx)tst-dlopenfailmod2.so
-# Order matters here. tst-dlopenfaillinkmod.so's soname ensures
-# a run-time loader failure.
+# Order matters here. tst-dlopenfaillinkmod.so's soname ensures a
+# run-time loader failure. --as-needed breaks this test because
+# nothing actually references tst-dlopenfailmod2.so (with its soname
+# tst-dlopenfail-missingmod.so).
+LDFLAGS-tst-dlopenfailmod1.so = -Wl,--no-as-needed
$(objpfx)tst-dlopenfailmod1.so: \
$(shared-thread-library) $(objpfx)tst-dlopenfaillinkmod.so
LDFLAGS-tst-dlopenfaillinkmod.so = -Wl,-soname,tst-dlopenfail-missingmod.so
Thanks,
Florian
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [pushed] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
2019-12-02 12:17 ` Florian Weimer
@ 2019-12-02 13:52 ` Szabolcs Nagy
0 siblings, 0 replies; 12+ messages in thread
From: Szabolcs Nagy @ 2019-12-02 13:52 UTC (permalink / raw)
To: Florian Weimer
Cc: nd, libc-alpha@sourceware.org, carlos@redhat.com,
gnugerrit@kalvdans.no-ip.org
On 02/12/2019 12:17, Florian Weimer wrote:
> * Szabolcs Nagy:
>> i see
>>
>> FAIL: elf/tst-dlopenfail
>>
>> and
>>
>> $ cat elf/tst-dlopenfail.out
>> info: attempting to load tst-dlopenfailmod1.so
>> tst-dlopenfailmod1 constructor executed
>>
>> on the aarch64 buildbot
>
> It's likely that your toolchain defaults to --no-as-needed. Does this
> patch fix things for you?
yes, this patch fixes the issue.
> diff --git a/elf/Makefile b/elf/Makefile
> index f825354d3f..0debea7759 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -1618,8 +1618,11 @@ LDFLAGS-tst-finilazyfailmod.so = \
> $(objpfx)tst-dlopenfail: $(libdl)
> $(objpfx)tst-dlopenfail.out: \
> $(objpfx)tst-dlopenfailmod1.so $(objpfx)tst-dlopenfailmod2.so
> -# Order matters here. tst-dlopenfaillinkmod.so's soname ensures
> -# a run-time loader failure.
> +# Order matters here. tst-dlopenfaillinkmod.so's soname ensures a
> +# run-time loader failure. --as-needed breaks this test because
> +# nothing actually references tst-dlopenfailmod2.so (with its soname
> +# tst-dlopenfail-missingmod.so).
> +LDFLAGS-tst-dlopenfailmod1.so = -Wl,--no-as-needed
> $(objpfx)tst-dlopenfailmod1.so: \
> $(shared-thread-library) $(objpfx)tst-dlopenfaillinkmod.so
> LDFLAGS-tst-dlopenfaillinkmod.so = -Wl,-soname,tst-dlopenfail-missingmod.so
>
> Thanks,
> Florian
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [review v3] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
2019-11-15 16:02 ` [review v3] " Florian Weimer (Code Review)
@ 2019-12-02 16:07 ` Florian Weimer
2019-12-02 16:53 ` Carlos O'Donell
0 siblings, 1 reply; 12+ messages in thread
From: Florian Weimer @ 2019-12-02 16:07 UTC (permalink / raw)
To: libc-alpha
* Florian Weimer:
> +/* Mark the objects as NODELETE if required. This is delayed until
> + after dlopen failure is not possible, so that _dl_close can clean
> + up objects if necessary. */
> +static void
> +activate_nodelete (struct link_map *new, int mode)
> +{
> + if (mode & RTLD_NODELETE || new->l_nodelete == link_map_nodelete_pending)
> + {
> + if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_FILES))
> + _dl_debug_printf ("activating NODELETE for %s [%lu]\n",
> + new->l_name, new->l_ns);
> + new->l_nodelete = link_map_nodelete_active;
> + }
> +
> + for (unsigned int i = 0; i < new->l_searchlist.r_nlist; ++i)
> + {
> + struct link_map *imap = new->l_searchlist.r_list[i];
> + if (imap->l_nodelete == link_map_nodelete_pending)
> + {
> + if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_FILES))
> + _dl_debug_printf ("activating NODELETE for %s [%lu]\n",
> + imap->l_name, imap->l_ns);
> +
> + /* Only new objects should have set
> + link_map_nodelete_pending. Existing objects should not
> + have gained any new dependencies and therefore cannot
> + reach NODELETE status. */
> + assert (!imap->l_init_called || imap->l_type != lt_loaded);
This assert is incorrect because the NODELETE markers actually go in the
other direction (from new loaded libraries to their dependencies).
I still need to write tests for this. But I can submit a patch for the
removal of the assert immediately, if so desired.
Thanks,
Florian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [review v3] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
2019-12-02 16:07 ` Florian Weimer
@ 2019-12-02 16:53 ` Carlos O'Donell
0 siblings, 0 replies; 12+ messages in thread
From: Carlos O'Donell @ 2019-12-02 16:53 UTC (permalink / raw)
To: Florian Weimer, libc-alpha
On 12/2/19 11:07 AM, Florian Weimer wrote:
> * Florian Weimer:
>
>> +/* Mark the objects as NODELETE if required. This is delayed until
>> + after dlopen failure is not possible, so that _dl_close can clean
>> + up objects if necessary. */
>> +static void
>> +activate_nodelete (struct link_map *new, int mode)
>> +{
>> + if (mode & RTLD_NODELETE || new->l_nodelete == link_map_nodelete_pending)
>> + {
>> + if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_FILES))
>> + _dl_debug_printf ("activating NODELETE for %s [%lu]\n",
>> + new->l_name, new->l_ns);
>> + new->l_nodelete = link_map_nodelete_active;
>> + }
>> +
>> + for (unsigned int i = 0; i < new->l_searchlist.r_nlist; ++i)
>> + {
>> + struct link_map *imap = new->l_searchlist.r_list[i];
>> + if (imap->l_nodelete == link_map_nodelete_pending)
>> + {
>> + if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_FILES))
>> + _dl_debug_printf ("activating NODELETE for %s [%lu]\n",
>> + imap->l_name, imap->l_ns);
>> +
>> + /* Only new objects should have set
>> + link_map_nodelete_pending. Existing objects should not
>> + have gained any new dependencies and therefore cannot
>> + reach NODELETE status. */
>> + assert (!imap->l_init_called || imap->l_type != lt_loaded);
>
> This assert is incorrect because the NODELETE markers actually go in the
> other direction (from new loaded libraries to their dependencies).
>
> I still need to write tests for this. But I can submit a patch for the
> removal of the assert immediately, if so desired.
I'm happy with seeing two patches, one which fixes the regression for
Fedora Rawhide and OpenSUSE Tumbleweed, and another which expands the
testsuite coverage.
If I understood correctly it's because:
- A dlopen of libstdc++ ...
- Can be used by something else dlopen'd later with a STB_GNU_UNIQUE symbol...
- Causing libstdc++ to be marked NODLETE.
So libstdc++ is l_type == lt_loaded, and l_init_called == 1, which means bot
sides of the || are false.
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-12-02 16:53 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-31 19:20 [review] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839] Florian Weimer (Code Review)
2019-10-31 20:18 ` Christian Häggström (Code Review)
2019-11-13 12:56 ` [review v2] " Florian Weimer (Code Review)
2019-11-13 14:28 ` Christian Häggström (Code Review)
2019-11-15 16:02 ` [review v3] " Florian Weimer (Code Review)
2019-12-02 16:07 ` Florian Weimer
2019-12-02 16:53 ` Carlos O'Donell
2019-11-21 12:57 ` Carlos O'Donell (Code Review)
2019-11-27 20:30 ` [pushed] " Sourceware to Gerrit sync (Code Review)
2019-12-02 10:06 ` Szabolcs Nagy
2019-12-02 12:17 ` Florian Weimer
2019-12-02 13:52 ` Szabolcs Nagy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).