[PATCH] BZ #17702: Fix recursive dlopen.

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

The ability to recursively call dlopen is useful for malloc 
implementations that wish to load other dynamic modules that
implement reentrant/AS-safe functions to use in their own
implementation.

Given that a user malloc implementation may be called by an
ongoing dlopen to allocate memory the user malloc
implementation interrupts dlopen and if it calls dlopen again
that's a reentrant call.

At present there are two problems with this:

(a) The first dlopen to return unmaps the ld.so.cache from
memory resulting in any outer dlopens potentially referencing
unmapped data (if they were in the middle of looking up an
entry in the cache when they themselves called malloc).

(b) There is code that asserts that _r_debug is RT_CONSISTENT
when dlopen is entered. This may not be true because we may
be inside another dlopen.

The solution is to:

(1) Change _dl_load_cache_lookup to return a copy of the
mapping data, and make the copy using alloca/strcpy first
to prevent malloc from interrupting the copy and potentially
unmapping the cache. If the alloca fails the strcpy immediatly
SIGSEGV's the process, and so a corrupted or overly long cache
path shouldn't compromise the application.

(2) Remove the assertion that _r_debug should be RT_CONSISTENT.

(3) Work with the gdb team and write up a description of
how _r_debug behaves, and specifically how it behaves during
recursive dlopen.

This patch implements (1) and (2). Fixing (3) is going to be
longer term and the next step towards making sure this can
be debugged during a recursive dlopen.

The patch adds a regression test for this new requirement.
The test uses malloc hooks to test recursive dlopen doesn't
fault or abort.

Comments?

OK to checkin?

2014-12-11  Carlos O'Donell  <carlos@redhat.com>

	[BZ #17702]
	* dlfcn/Makefile (tests): Add tst-rec-dlopen.
	(modules-names): Add moddummy1 and moddummy2.
	(LDLIBS-tst-rec-dlopen): Define.
	($(objpfx)tst-rec-dlopen): Define.
	* dlfcn/moddummy1.c: New file.
	* dlfcn/moddummy2.c: New file.
	* dlfcn/tst-rec-dlopen.c: New file.
	* elf/dl-cache.c (_dl_load_cache_lookup):
	Return char*. Copy result with alloca/strcpy/strdup.
	* elf/dl-load.c (_dl_map_object): _dl_load_cached_lookup
	returns char*. Free cached. If not saving realname
	free cached.
	* elf/dl-open.c (dl_open_worker): Do not assert that
	_r_debug->r_state is RT_CONSISTENT.
	* sysdeps/generic/ldsodefs.h: _dl_load_cache_lookup
	returns char*.

diff --git a/dlfcn/Makefile b/dlfcn/Makefile
index 1fad0a5..e4bae7f 100644
--- a/dlfcn/Makefile
+++ b/dlfcn/Makefile
@@ -36,13 +36,13 @@ endif
 ifeq (yes,$(build-shared))
 tests = glrefmain failtest tst-dladdr default errmsg1 tstcxaatexit \
 	bug-dlopen1 bug-dlsym1 tst-dlinfo bug-atexit1 bug-atexit2 \
-	bug-atexit3 tstatexit bug-dl-leaf
+	bug-atexit3 tstatexit bug-dl-leaf tst-rec-dlopen
 endif
 modules-names = glreflib1 glreflib2 glreflib3 failtestmod defaultmod1 \
 		defaultmod2 errmsg1mod modatexit modcxaatexit \
 		bug-dlsym1-lib1 bug-dlsym1-lib2 bug-atexit1-lib \
 		bug-atexit2-lib bug-atexit3-lib bug-dl-leaf-lib \
-		bug-dl-leaf-lib-cb
+		bug-dl-leaf-lib-cb moddummy1 moddummy2
 
 failtestmod.so-no-z-defs = yes
 glreflib2.so-no-z-defs = yes
@@ -139,3 +139,6 @@ $(objpfx)bug-dl-leaf: $(objpfx)bug-dl-leaf-lib.so
 $(objpfx)bug-dl-leaf.out: $(objpfx)bug-dl-leaf-lib-cb.so
 $(objpfx)bug-dl-leaf-lib.so: $(libdl)
 $(objpfx)bug-dl-leaf-lib-cb.so: $(objpfx)bug-dl-leaf-lib.so
+
+LDLIBS-tst-rec-dlopen = -ldl
+$(objpfx)tst-rec-dlopen: $(libdl)
diff --git a/dlfcn/moddummy1.c b/dlfcn/moddummy1.c
new file mode 100644
index 0000000..0e66f32
--- /dev/null
+++ b/dlfcn/moddummy1.c
@@ -0,0 +1,13 @@
+/* Provide a dummy DSO for tst-recursive-dlopen to use.  */
+#include <stdio.h>
+#include <stdlib.h>
+
+int called_dummy1;
+
+void
+dummy1 (void)
+{
+  printf ("Called dummy1()\n");
+  called_dummy1++;
+}
+
diff --git a/dlfcn/moddummy2.c b/dlfcn/moddummy2.c
new file mode 100644
index 0000000..bf18a2f
--- /dev/null
+++ b/dlfcn/moddummy2.c
@@ -0,0 +1,13 @@
+/* Provide a dummy DSO for tst-recursive-dlopen to use.  */
+#include <stdio.h>
+#include <stdlib.h>
+
+int called_dummy2;
+
+void
+dummy2 (void)
+{
+  printf ("Called dummy2()\n");
+  called_dummy2++;
+}
+
diff --git a/dlfcn/tst-rec-dlopen.c b/dlfcn/tst-rec-dlopen.c
new file mode 100644
index 0000000..b461fdc
--- /dev/null
+++ b/dlfcn/tst-rec-dlopen.c
@@ -0,0 +1,144 @@
+/* Test recursive dlopen using malloc hooks.
+   Copyright (C) 1998-2014 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Ulrich Drepper <drepper@cygnus.com>, 1998.
+
+   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/>.  */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <malloc.h>
+#include <dlfcn.h>
+
+#define DSO "moddummy1.so"
+#define FUNC "dummy1"
+
+#define DSO1 "moddummy2.so"
+#define FUNC1 "dummy2"
+
+/* Prevent the compiler from moving the assignment to called_func
+   before (*func)() since the compiler doesn't know we might abort
+   or catch a SIGSEGV signal and it may move the store.  */
+volatile int called_func;
+
+/* Prototype for my hook.  */
+void *custom_malloc_hook (size_t, const void *);
+
+/* Pointer to old malloc hooks.  */
+void *(*old_malloc_hook) (size_t, const void *);
+
+/* Call function func_name in DSO dso_name via dlopen.  */
+void
+call_func (const char *dso_name, const char *func_name)
+{
+  int ret;
+  void *dso;
+  void (*func) (void);
+  char *err;
+
+  /* Open the DSO.  */
+  dso = dlopen (dso_name, RTLD_NOW|RTLD_GLOBAL);
+  if (dso == NULL)
+    {
+      err = dlerror ();
+      fprintf (stderr, "%s\n", err);
+      exit (1);
+    }
+  /* Clear any errors.  */
+  dlerror ();
+
+  /* Lookup func.  */
+  *(void **) (&func) = dlsym (dso, func_name);
+  if (func == NULL)
+    {
+      err = dlerror ();
+      if (err != NULL)
+        {
+	  fprintf (stderr, "%s\n", err);
+	  exit (1);
+        }
+    }
+  /* Call func.  */
+  (*func) ();
+  called_func = 1;
+
+  /* Close the library and look for errors too.  */
+  ret = dlclose (dso);
+  if (ret != 0)
+    {
+      err = dlerror ();
+      fprintf (stderr, "%s\n", err);
+      exit (1);
+    }
+
+}
+
+/* Empty hook that does nothing.  */
+void *
+custom_malloc_hook (size_t size, const void *caller)
+{
+  void *result;
+  /* Restore old hooks.  */
+  __malloc_hook = old_malloc_hook;
+  /* First call a function in another library via dlopen.  */
+  call_func (DSO1, FUNC1);
+  /* Called recursively.  */
+  result = malloc (size);
+  /* Restore new hooks.  */
+  __malloc_hook = custom_malloc_hook;
+  return result;
+}
+
+static int
+do_test (void)
+{
+  /* Save old hook.  */
+  old_malloc_hook = __malloc_hook;
+  /* Install new hook.  */
+  __malloc_hook = custom_malloc_hook;
+
+  /* Bug 17702 fixes two things:
+       * A recursive dlopen unmapping the ld.so.cache.
+       * An assertion that _r_debug is RT_CONSISTENT at entry to dlopen.
+     We can only test the latter. Testing the former requires modifying
+     ld.so.conf to cache the dummy libraries, then running ldconfig,
+     then run the test. If you do all of that (and glibc's test
+     infrastructure doesn't support that yet) then the test will
+     SEGFAULT without the fix. If you don't do that, then the test
+     will abort because of the assert described in detail below.  */
+  call_func (DSO, FUNC);
+
+  /* Restore old hook.  */
+  __malloc_hook = old_malloc_hook;
+
+  /* The function dummy2() is called by the malloc hook. Check to
+     see that it was called. This ensures the second recursive
+     dlopen happened and we called the function in that library.
+     Before the fix you either get a SIGSEGV when accessing mmap'd
+     ld.so.cache data or an assertion failure about _r_debug not
+     beint RT_CONSISTENT.  We don't test for the SIGSEGV since it
+     would require finding moddummy1 or moddummy2 in the cache and
+     we don't have any infrastructure to test that, but the _r_debug
+     assertion triggers.  */
+  if (called_func > 0)
+    printf ("PASS: Function call_func() called more than once.\n");
+  else
+    printf ("FAIL: Function call_func() not called.\n");
+
+  return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/elf/dl-cache.c b/elf/dl-cache.c
index 91fef95..396b541 100644
--- a/elf/dl-cache.c
+++ b/elf/dl-cache.c
@@ -174,9 +174,12 @@ _dl_cache_libcmp (const char *p1, const char *p2)
 
 /* Look up NAME in ld.so.cache and return the file name stored there, or null
    if none is found.  The cache is loaded if it was not already.  If loading
-   the cache previously failed there will be no more attempts to load it.  */
-
-const char *
+   the cache previously failed there will be no more attempts to load it.
+   The caller is responsible for freeing the returned string.  The ld.so.cache
+   may be unmapped at any time by a completing recursive dlopen and
+   this function must take care that it does not return references to
+   any data in the mapping.  */
+char *
 internal_function
 _dl_load_cache_lookup (const char *name)
 {
@@ -289,7 +292,17 @@ _dl_load_cache_lookup (const char *name)
       && best != NULL)
     _dl_debug_printf ("  trying file=%s\n", best);
 
-  return best;
+  if (best == NULL)
+    return NULL;
+
+  /* The double copy is *required* since malloc may be interposed
+     and call dlopen itself whose completion would unmap the data
+     we are accessing. Therefore we must make the copy of the
+     mapping data without using malloc.  */
+  char *temp;
+  temp = alloca (strlen (best) + 1);
+  strcpy (temp, best);
+  return strdup (temp);
 }
 
 #ifndef MAP_COPY
diff --git a/elf/dl-load.c b/elf/dl-load.c
index 7a03ccf..02285f3 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -2051,7 +2051,7 @@ _dl_map_object (struct link_map *loader, const char *name,
 	{
 	  /* Check the list of libraries in the file /etc/ld.so.cache,
 	     for compatibility with Linux's ldconfig program.  */
-	  const char *cached = _dl_load_cache_lookup (name);
+	  char *cached = _dl_load_cache_lookup (name);
 
 	  if (cached != NULL)
 	    {
@@ -2075,6 +2075,7 @@ _dl_map_object (struct link_map *loader, const char *name,
 		      if (memcmp (cached, dirp, system_dirs_len[cnt]) == 0)
 			{
 			  /* The prefix matches.  Don't use the entry.  */
+			  free (cached);
 			  cached = NULL;
 			  break;
 			}
@@ -2092,14 +2093,9 @@ _dl_map_object (struct link_map *loader, const char *name,
 				    LA_SER_CONFIG, mode, &found_other_class,
 				    false);
 		  if (__glibc_likely (fd != -1))
-		    {
-		      realname = __strdup (cached);
-		      if (realname == NULL)
-			{
-			  __close (fd);
-			  fd = -1;
-			}
-		    }
+		    realname = cached;
+		  else
+		    free (cached);
 		}
 	    }
 	}
diff --git a/elf/dl-open.c b/elf/dl-open.c
index dec3d32..ae699a9 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -217,7 +217,9 @@ dl_open_worker (void *a)
 	args->nsid = call_map->l_ns;
     }
 
-  assert (_dl_debug_initialize (0, args->nsid)->r_state == RT_CONSISTENT);
+  /* One might be tempted to assert that we are RT_CONSISTENT at this point, but that
+     may not be true if this is a recursive call to dlopen.  */
+  _dl_debug_initialize (0, args->nsid);
 
   /* Load the named object.  */
   struct link_map *new;
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index d07eaac..77f26d4 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -905,8 +905,8 @@ extern const struct r_strlenpair *_dl_important_hwcaps (const char *platform,
      internal_function;
 
 /* Look up NAME in ld.so.cache and return the file name stored there,
-   or null if none is found.  */
-extern const char *_dl_load_cache_lookup (const char *name)
+   or null if none is found.  Caller must free returned string.  */
+extern char *_dl_load_cache_lookup (const char *name)
      internal_function;
 
 /* If the system does not support MAP_COPY we cannot leave the file open
---

Cheers,
Carlos.