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

[Carlos O'Donell <carlos at redhat dot com>]

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.

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

-- 
Cheers,
Carlos.