BZ #17702: Fix recursive dlopen.

Message ID 548B976F.2050506@redhat.com
State Committed
Headers

Commit Message

Carlos O'Donell Dec. 13, 2014, 1:33 a.m. UTC
  On 12/12/2014 05:27 PM, Andreas Schwab wrote:
> "Carlos O'Donell" <carlos@redhat.com> writes:
> 
>> On 12/12/2014 06:10 AM, Andreas Schwab wrote:
>>> "Carlos O'Donell" <carlos@redhat.com> writes:
>>>
>>>> +LDLIBS-tst-rec-dlopen = -ldl
>>>> +$(objpfx)tst-rec-dlopen: $(libdl)
>>>
>>> Why do you need to set LDLIBS?
>>
>> How else do I get it to to link libdl.so against the test case?
> 
> It's listed as a dependency.

I see. It does indeed work with just the dependency.
I guess I don't know what LDLIBS is for? Is it only
for building DSOs?

Here is a v2 with several fixes, including checking to make sure
that the return value of the called function is indeed that of
moddummy1 and provides some assurance that nothing failed.

Cheers,
Carlos.

v2.
- Remove LDLIBS
- tst-rec-dlopen should depend on moddummy1 and moddummy2 DSOs.
- Use return of DSO to determine if DSO func was called and that
  dummy1 was the outermost called function.

---

Cheers,
Carlos.
  

Comments

Andreas Schwab Dec. 13, 2014, 12:09 p.m. UTC | #1
"Carlos O'Donell" <carlos@redhat.com> writes:

> I guess I don't know what LDLIBS is for? Is it only
> for building DSOs?

It's for external libraries.

Andreas.
  
Carlos O'Donell Dec. 13, 2014, 1:23 p.m. UTC | #2
On 12/13/2014 07:09 AM, Andreas Schwab wrote:
> "Carlos O'Donell" <carlos@redhat.com> writes:
> 
>> I guess I don't know what LDLIBS is for? Is it only
>> for building DSOs?
> 
> It's for external libraries.

Thanks. Any thoughts on v2 of the patch?

Cheers,
Carlos.
  
Carlos O'Donell Jan. 21, 2015, 2:12 p.m. UTC | #3
On 12/12/2014 08:33 PM, Carlos O'Donell wrote:
> On 12/12/2014 05:27 PM, Andreas Schwab wrote:
>> "Carlos O'Donell" <carlos@redhat.com> writes:
>>
>>> On 12/12/2014 06:10 AM, Andreas Schwab wrote:
>>>> "Carlos O'Donell" <carlos@redhat.com> writes:
>>>>
>>>>> +LDLIBS-tst-rec-dlopen = -ldl
>>>>> +$(objpfx)tst-rec-dlopen: $(libdl)
>>>>
>>>> Why do you need to set LDLIBS?
>>>
>>> How else do I get it to to link libdl.so against the test case?
>>
>> It's listed as a dependency.
> 
> I see. It does indeed work with just the dependency.
> I guess I don't know what LDLIBS is for? Is it only
> for building DSOs?
> 
> Here is a v2 with several fixes, including checking to make sure
> that the return value of the called function is indeed that of
> moddummy1 and provides some assurance that nothing failed.
> 
> Cheers,
> Carlos.
> 
> v2.
> - Remove LDLIBS
> - tst-rec-dlopen should depend on moddummy1 and moddummy2 DSOs.
> - Use return of DSO to determine if DSO func was called and that
>   dummy1 was the outermost called function.

I have committed v2 to allow dlopen to be called recursively.

We're using this in RHEL to allow 3rd party malloc interposes
to use dlopen.

Cheers,
Carlos.
  

Patch

diff --git a/dlfcn/Makefile b/dlfcn/Makefile
index 1fad0a5..537fe85 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
+
+$(objpfx)tst-rec-dlopen: $(libdl)
+$(objpfx)tst-rec-dlopen.out: $(objpfx)moddummy1.so $(objpfx)moddummy2.so
diff --git a/dlfcn/moddummy1.c b/dlfcn/moddummy1.c
new file mode 100644
index 0000000..6e549fa
--- /dev/null
+++ b/dlfcn/moddummy1.c
@@ -0,0 +1,10 @@ 
+/* Provide a dummy DSO for tst-rec-dlopen to use.  */
+#include <stdio.h>
+#include <stdlib.h>
+
+int
+dummy1 (void)
+{
+  printf ("Called dummy1()\n");
+  return 1;
+}
diff --git a/dlfcn/moddummy2.c b/dlfcn/moddummy2.c
new file mode 100644
index 0000000..cb4edc8
--- /dev/null
+++ b/dlfcn/moddummy2.c
@@ -0,0 +1,13 @@ 
+/* Provide a dummy DSO for tst-rec-dlopen to use.  */
+#include <stdio.h>
+#include <stdlib.h>
+
+int
+dummy2 (void)
+{
+  printf ("Called dummy2()\n");
+  /* If the outer dlopen is not dummy1 (becuase of some error)
+     then tst-rec-dlopen will see a value of -1 as the returned
+     result and fail.  */
+  return -1;
+}
diff --git a/dlfcn/tst-rec-dlopen.c b/dlfcn/tst-rec-dlopen.c
new file mode 100644
index 0000000..35b08d4
--- /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"
+
+/* Result of the called function.  */
+int func_result;
+
+/* 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;
+  int (*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_result = (*func) ();
+
+  /* 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.  */
+  printf ("Returned result is %d\n", func_result);
+  if (func_result <= 0)
+    {
+      printf ("FAIL: Function call_func() not called.\n");
+      exit (1);
+    }
+
+  printf ("PASS: Function call_func() called more than once.\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