diff mbox

[v2] Bug 11941: Improper assert map->l_init_called in dlclose

Message ID e205557c-5b54-4e91-9f96-3107d155e711@redhat.com
State New
Headers show

Commit Message

Carlos O'Donell Dec. 22, 2016, 6:26 p.m. UTC
On 12/22/2016 04:36 AM, Florian Weimer wrote:
> On 12/22/2016 06:36 AM, Carlos O'Donell wrote:
> 
>> +  /* We must take the lock to examine the contents of map whose
>> +     l_flags_1 or l_direct_opencount may be modified by concurrent
>> +     dlopen calls.  */
>> +  __rtld_lock_lock_recursive (GL(dl_load_lock));
>> +
>>    /* First see whether we can remove the object at all.  */
>>    if (__glibc_unlikely (map->l_flags_1 & DF_1_NODELETE))
>>      {
>> -      assert (map->l_init_called);
>>        /* Nope.  Do nothing.  */
>> +      __rtld_lock_unlock_recursive (GL(dl_load_lock));
>>        return;
>>      }
>>
>>    if (__builtin_expect (map->l_direct_opencount, 1) == 0)
>>      _dl_signal_error (0, map->l_name, NULL, N_("shared object not open"));
> 
> Missing unlock before non-local exit.

Fixed.

> The plugin should have some reference to a symbol defined by the
> other DSO, so that if ld applies --as-needed by default for some
> reason, the test still has the expected behavior.


Done.  Added a function in the DSO which the plugin can call.
Verified there is a jump slot for this now, we never call it, but it
satisfies the requirement (alternatively could have been a table of
function descriptors).

>> diff --git a/elf/tst-nodelete-dlclose-dso.c b/elf/tst-nodelete-dlclose-dso.c
> 
>> +void (*plugin_func)(void);
> 
> Missing space before paramter list.  Those variables could be static.

Fixed.
 
>> +#define LIB_PLUGIN "tst-nodelete-dlclose-plugin.so"
>> +
>> +void
>> +primary(void)
> 
> Likewise.

Fixed.

>> +{
>> +  char *error;
>> +
>> +  plugin_lib = dlopen (LIB_PLUGIN, RTLD_NOW | RTLD_LOCAL | RTLD_NODELETE);
>> +  if (!plugin_lib)
> 
> Comparison against NULL is required by the style guide.

Fixed.

>> +__attribute__((destructor))
>> +void
>> +primary_dtor(void)
> 
> Missing space before parameter list.  Function could be static.

Both fixed.
 
>> diff --git a/elf/tst-nodelete-dlclose-plugin.c b/elf/tst-nodelete-dlclose-plugin.c
> 
>> +plugin(void)
> 
> Likewise.

Fixed.
> 
>> +{
>> +  printf("INFO: Calling plugin function.\n");
> 
> Likewise.

Fixed.

>> +}
>> +
>> +__attribute__((destructor))
> 
> Likewise.

Fixed.

>> +static void
>> +plugin_dtor(void)
> 
> Likewise.

Fixed.

>> +{
>> +  printf("INFO: Calling plugin destructor.\n");
> 
> Likewise.

Fixed.

>> diff --git a/elf/tst-nodelete-dlclose.c b/elf/tst-nodelete-dlclose.c
> 
>> +extern void primary(void);
> 
> Likewise.

Fixed.

>> +
>> +static int
>> +do_test(void)
> 
> Likewise.

Fixed.

>> +{
>> +  printf ("INFO: Starting applicaiton.\n");
> 
> Typo: application

Fixed.

Thanks for the thorough review.

v2
- Fix GNU style problems.
- Fix INFO message typo.
- Fix missing unlock in non-local goto from _dl_signal_error.
- Filed bug 20990 for double-dlclose detection issues and reference
  bug in new expanded comment.
- Added reference from plugin to DSO to avoid future --as-needed problems.

No regressions on x86_64.

2016-12-21  Carlos O'Donell  <carlos@redhat.com>

	[BZ #11941]
	* elf/dl-close.c (_dl_close): Take dl_load_lock to examine map.
	Remove assert (map->l_init_called); if DF_1_NODELETE is set.
	* elf/Makefile [ifeq (yes,$(build-shared))] (tests): Add
	tst-nodelete-dlclose.
	(modules-names): Add tst-nodelete-dlclose-dso and
	tst-nodelete-dlclose-plugin.
	($(objpfx)tst-nodelete-dlclose-dso.so): Define.
	($(objpfx)tst-nodelete-dlclose-plugin.so): Define.
	($(objpfx)tst-nodelete-dlclose): Define.
	($(objpfx)tst-nodelete-dlclose.out): Define.

---

Comments

Carlos O'Donell Dec. 22, 2016, 8:47 p.m. UTC | #1
On 12/22/2016 01:26 PM, Carlos O'Donell wrote:
> v2
> - Fix GNU style problems.
> - Fix INFO message typo.
> - Fix missing unlock in non-local goto from _dl_signal_error.
> - Filed bug 20990 for double-dlclose detection issues and reference
>   bug in new expanded comment.
> - Added reference from plugin to DSO to avoid future --as-needed problems.

v3
- Send the right patch with 'plugin_func' plugin symbol.

> +  plugin_func = (void (*)(void)) dlsym (plugin_lib, "plugin_func");

> diff --git a/elf/tst-nodelete-dlclose-plugin.c b/elf/tst-nodelete-dlclose-plugin.c
> new file mode 100644
> index 0000000..d0261ea
> --- /dev/null
> +++ b/elf/tst-nodelete-dlclose-plugin.c

> +void
> +plugin (void)

s/plugin/plugin_func/g.
Florian Weimer Dec. 23, 2016, 10:37 a.m. UTC | #2
On 12/22/2016 09:47 PM, Carlos O'Donell wrote:

> s/plugin/plugin_func/g.

GNU does not use “()” to mark function names:

+   destructed we will be calling dlclose() on an object that has already
+   been destructed.  It is allowed to call dlclose() in this way and
+     hasn't called primary(), in that case do nothing.  */

There is still a missing space before the parameter list in a function 
pointer type:

+  plugin_func = (void (*)(void)) dlsym (plugin_lib, "plugin_func");

Okay with these changes.

Thanks,
Florian
Carlos O'Donell Dec. 23, 2016, 6:44 p.m. UTC | #3
On 12/23/2016 05:37 AM, Florian Weimer wrote:
> On 12/22/2016 09:47 PM, Carlos O'Donell wrote:
> 
>> s/plugin/plugin_func/g.
> 
> GNU does not use “()” to mark function names:
> 
> +   destructed we will be calling dlclose() on an object that has already
> +   been destructed.  It is allowed to call dlclose() in this way and
> +     hasn't called primary(), in that case do nothing.  */
> 
> There is still a missing space before the parameter list in a function pointer type:
> 
> +  plugin_func = (void (*)(void)) dlsym (plugin_lib, "plugin_func");
> 
> Okay with these changes.

v5
- Removed () markup for function names.
- Fixed GNU style error.

Committed.

commit 57707b7fcc38855869321f8c7827bfe21d729f37
Author: Carlos O'Donell <carlos@redhat.com>
Date:   Fri Dec 23 13:30:22 2016 -0500

    Bug 11941: ld.so: Improper assert map->l_init_called in dlclose
    
    There is at least one use case where during exit a library destructor
    might call dlclose() on a valid handle and have it fail with an
    assertion. We must allow this case, it is a valid handle, and dlclose()
    should not fail with an assert. In the future we might be able to return
    an error that the dlclose() could not be completed because the opened
    library has already been unloaded and destructors have run as part of
    exit processing.
    
    For more details see:
    https://www.sourceware.org/ml/libc-alpha/2016-12/msg00859.html
diff mbox

Patch

diff --git a/elf/Makefile b/elf/Makefile
index 330397e..cd26e16 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -152,7 +152,7 @@  tests += loadtest restest1 preloadtest loadfail multiload origtest resolvfail \
 	 tst-initorder tst-initorder2 tst-relsort1 tst-null-argv \
 	 tst-ptrguard1 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-latepthread tst-tls-manydynamic tst-nodelete-dlclose
 #	 reldep9
 ifeq ($(build-hardcoded-path-in-tests),yes)
 tests += tst-dlopen-aout
@@ -231,7 +231,8 @@  modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
 		tst-tlsalign-lib tst-nodelete-opened-lib tst-nodelete2mod \
 		tst-audit11mod1 tst-audit11mod2 tst-auditmod11 \
 		tst-audit12mod1 tst-audit12mod2 tst-audit12mod3 tst-auditmod12 \
-		tst-latepthreadmod $(tst-tls-many-dynamic-modules)
+		tst-latepthreadmod $(tst-tls-many-dynamic-modules) \
+		tst-nodelete-dlclose-dso tst-nodelete-dlclose-plugin
 ifeq (yes,$(have-mtls-dialect-gnu2))
 tests += tst-gnu2-tls1
 modules-names += tst-gnu2-tls1mod
@@ -1345,3 +1346,12 @@  ifeq (no,$(nss-crypt))
 $(objpfx)tst-linkall-static: \
   $(common-objpfx)crypt/libcrypt.a
 endif
+
+# The application depends on the DSO, and the DSO loads the plugin.
+# The plugin also depends on the DSO. This creates the circular
+# dependency via dlopen that we're testing to make sure works.
+$(objpfx)tst-nodelete-dlclose-dso.so: $(libdl)
+$(objpfx)tst-nodelete-dlclose-plugin.so: $(objpfx)tst-nodelete-dlclose-dso.so
+$(objpfx)tst-nodelete-dlclose: $(objpfx)tst-nodelete-dlclose-dso.so
+$(objpfx)tst-nodelete-dlclose.out: $(objpfx)tst-nodelete-dlclose-dso.so \
+				   $(objpfx)tst-nodelete-dlclose-plugin.so
diff --git a/elf/dl-close.c b/elf/dl-close.c
index 6489703..4ffbcca 100644
--- a/elf/dl-close.c
+++ b/elf/dl-close.c
@@ -805,19 +805,37 @@  _dl_close (void *_map)
 {
   struct link_map *map = _map;
 
-  /* First see whether we can remove the object at all.  */
+  /* We must take the lock to examine the contents of map and avoid
+     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))
     {
-      assert (map->l_init_called);
       /* 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
+     might have been freed and this access is potentially a data race
+     with whatever other use this memory might have now, or worse we
+     might silently corrupt memory if it looks enough like a link map.
+     POSIX has language in dlclose that appears to guarantee that this
+     should be a detectable case and given that dlclose should be threadsafe
+     we need this to be a reliable detection.
+     This is bug 20990. */
   if (__builtin_expect (map->l_direct_opencount, 1) == 0)
-    _dl_signal_error (0, map->l_name, NULL, N_("shared object not open"));
-
-  /* Acquire the lock.  */
-  __rtld_lock_lock_recursive (GL(dl_load_lock));
+    {
+      __rtld_lock_unlock_recursive (GL(dl_load_lock));
+      _dl_signal_error (0, map->l_name, NULL, N_("shared object not open"));
+    }
 
   _dl_close_worker (map, false);
 
diff --git a/elf/tst-nodelete-dlclose-dso.c b/elf/tst-nodelete-dlclose-dso.c
new file mode 100644
index 0000000..0240bea
--- /dev/null
+++ b/elf/tst-nodelete-dlclose-dso.c
@@ -0,0 +1,90 @@ 
+/* Bug 11941: Improper assert map->l_init_called in dlclose.
+   Copyright (C) 2016 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
+   <http://www.gnu.org/licenses/>.  */
+
+/* This is the primary DSO that is loaded by the appliation.  This DSO
+   then loads a plugin with RTLD_NODELETE.  This plugin depends on this
+   DSO.  This dependency chain means that at application shutdown the
+   plugin will be destructed first.  Thus by the time this DSO is
+   destructed we will be calling dlclose() on an object that has already
+   been destructed.  It is allowed to call dlclose() in this way and
+   should not assert.  */
+#include <stdio.h>
+#include <stdlib.h>
+#include <dlfcn.h>
+
+/* Plugin to load.  */
+static void *plugin_lib = NULL;
+/* Plugin function.  */
+static void (*plugin_func) (void);
+#define LIB_PLUGIN "tst-nodelete-dlclose-plugin.so"
+
+/* This function is never called but the plugin references it.
+   We do this to avoid any future --as-needed from removing the
+   plugin's DT_NEEDED on this DSO (required for the test).  */
+void
+primary_reference (void)
+{
+  printf ("INFO: Called primary_reference function.\n");
+}
+
+void
+primary (void)
+{
+  char *error;
+
+  plugin_lib = dlopen (LIB_PLUGIN, RTLD_NOW | RTLD_LOCAL | RTLD_NODELETE);
+  if (plugin_lib == NULL)
+    {
+      printf ("ERROR: Unable to load plugin library.\n");
+      exit (EXIT_FAILURE);
+    }
+  dlerror ();
+
+  plugin_func = (void (*)(void)) dlsym (plugin_lib, "plugin_func");
+  error = dlerror ();
+  if (error != NULL)
+    {
+      printf ("ERROR: Unable to find symbol with error \"%s\".",
+	      error);
+      exit (EXIT_FAILURE);
+    }
+
+  return;
+}
+
+__attribute__ ((destructor))
+static void
+primary_dtor (void)
+{
+  int ret;
+
+  printf ("INFO: Calling primary destructor.\n");
+
+  /* The destructor runs in the test driver also, which
+     hasn't called primary(), in that case do nothing.  */
+  if (plugin_lib == NULL)
+    return;
+
+  ret = dlclose (plugin_lib);
+  if (ret != 0)
+    {
+      printf ("ERROR: Calling dlclose failed with \"%s\"\n",
+	      dlerror ());
+      exit (EXIT_FAILURE);
+    }
+}
diff --git a/elf/tst-nodelete-dlclose-plugin.c b/elf/tst-nodelete-dlclose-plugin.c
new file mode 100644
index 0000000..d0261ea
--- /dev/null
+++ b/elf/tst-nodelete-dlclose-plugin.c
@@ -0,0 +1,40 @@ 
+/* Bug 11941: Improper assert map->l_init_called in dlclose.
+   Copyright (C) 2016 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
+   <http://www.gnu.org/licenses/>.  */
+
+/* This DSO simulates a plugin with a dependency on the
+   primary DSO loaded by the appliation.  */
+#include <stdio.h>
+
+extern void primary_reference (void);
+
+void
+plugin (void)
+{
+  printf ("INFO: Calling plugin function.\n");
+  /* Need a reference to the DSO to ensure that a potential --as-needed
+     doesn't remove the DT_NEEDED entry which we rely upon to ensure
+     destruction ordering.  */
+  primary_reference ();
+}
+
+__attribute__ ((destructor))
+static void
+plugin_dtor (void)
+{
+  printf ("INFO: Calling plugin destructor.\n");
+}
diff --git a/elf/tst-nodelete-dlclose.c b/elf/tst-nodelete-dlclose.c
new file mode 100644
index 0000000..a12f929
--- /dev/null
+++ b/elf/tst-nodelete-dlclose.c
@@ -0,0 +1,35 @@ 
+/* Bug 11941: Improper assert map->l_init_called in dlclose.
+   Copyright (C) 2016 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
+   <http://www.gnu.org/licenses/>.  */
+
+/* This simulates an application using the primary DSO which loads the
+   plugin DSO.  */
+#include <stdio.h>
+#include <stdlib.h>
+
+extern void primary (void);
+
+static int
+do_test (void)
+{
+  printf ("INFO: Starting application.\n");
+  primary ();
+  printf ("INFO: Exiting application.\n");
+  return 0;
+}
+
+#include <support/test-driver.c>