This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] Fix concurrency problem between dl_open and dl_iterate_phdr


Hi,

while modifying the list of loaded namespaces is protected with the
dl_load_write_lock there doesn't seem to be any locking when
manipulating the contents of its elements.  So it might happen that
one thread is in dl_iterate_phdr (e.g. called from __pthread_unwind)
while another calls dlopen.  In the case I looked into the phdr field
of a link_map entry was written in dl_map_object_from_fd while another
thread created a local stack copy of that value in dl_iterate_phdr.
The program crashed when the stack copy was used to access the program
header in _Unwind_IteratePhdrCallback.

The attached patch fixes the problem by appending the newly created
elements to the list *after* its content has been written.

I haven't separated the allocation and adding the element to the list
in rtld.c.  I assume that there can't be a thread doing unwinding in
parallel.  dl_main also seems to do list manipulations without holding
the dl_load_write_lock lock.

No regressions on x86_64 and s390x.

It has been verified with a JVM stress test that the problem does not
occur anymore when the patch is applied.

Bye,

-Andreas-


2010-10-20  Andreas Krebbel  <Andreas.Krebbel@de.ibm.com>

	* elf/dl-object.c (_dl_add_to_namespace_list): New function.
	(_dl_new_object): Don't append the new object to the list here.
	* elf/rtld.c (dl_main): Invoke _dl_add_to_namespace_list.
	* glibc/sysdeps/generic/ldsodefs.h (_dl_add_to_namespace_list):
	New prototype.
	* elf/dl-load.c (lose): Don't remove the element from the list.
	(_dl_map_object_from_fd): Invoke _dl_add_to_namespace_list.
	(_dl_map_object): Likewise.


Index: glibc/elf/dl-object.c
===================================================================
--- glibc.orig/elf/dl-object.c
+++ glibc/elf/dl-object.c
@@ -25,9 +25,37 @@
 
 #include <assert.h>
 
+/* Add the new link_map NEW to the end of the namespace list.  */
 
-/* Allocate a `struct link_map' for a new object being loaded,
-   and enter it into the _dl_loaded list.  */
+void
+internal_function
+_dl_add_to_namespace_list (struct link_map *new, Lmid_t nsid)
+{
+  struct link_map *l;
+
+  /* We modify the list of loaded objects.  */
+  __rtld_lock_lock_recursive (GL(dl_load_write_lock));
+
+  if (GL(dl_ns)[nsid]._ns_loaded != NULL)
+    {
+      l = GL(dl_ns)[nsid]._ns_loaded;
+      while (l->l_next != NULL)
+	l = l->l_next;
+      new->l_prev = l;
+      /* new->l_next = NULL;	Would be necessary but we use calloc.  */
+      l->l_next = new;
+    }
+  else
+    GL(dl_ns)[nsid]._ns_loaded = new;
+  ++GL(dl_ns)[nsid]._ns_nloaded;
+  new->l_serial = GL(dl_load_adds);
+  ++GL(dl_load_adds);
+
+  __rtld_lock_unlock_recursive (GL(dl_load_write_lock));
+}
+
+
+/* Allocate a `struct link_map' for a new object being loaded.  */
 
 struct link_map *
 internal_function
@@ -35,7 +63,7 @@ _dl_new_object (char *realname, const ch
 		struct link_map *loader, int mode, Lmid_t nsid)
 {
   struct link_map *l;
-  int idx;
+  int idx = 0;
   size_t libname_len = strlen (libname) + 1;
   struct link_map *new;
   struct libname_list *newname;
@@ -93,31 +121,9 @@ _dl_new_object (char *realname, const ch
   new->l_scope = new->l_scope_mem;
   new->l_scope_max = sizeof (new->l_scope_mem) / sizeof (new->l_scope_mem[0]);
 
-  /* We modify the list of loaded objects.  */
-  __rtld_lock_lock_recursive (GL(dl_load_write_lock));
-
-  /* Counter for the scopes we have to handle.  */
-  idx = 0;
-
   if (GL(dl_ns)[nsid]._ns_loaded != NULL)
-    {
-      l = GL(dl_ns)[nsid]._ns_loaded;
-      while (l->l_next != NULL)
-	l = l->l_next;
-      new->l_prev = l;
-      /* new->l_next = NULL;	Would be necessary but we use calloc.  */
-      l->l_next = new;
-
-      /* Add the global scope.  */
-      new->l_scope[idx++] = &GL(dl_ns)[nsid]._ns_loaded->l_searchlist;
-    }
-  else
-    GL(dl_ns)[nsid]._ns_loaded = new;
-  ++GL(dl_ns)[nsid]._ns_nloaded;
-  new->l_serial = GL(dl_load_adds);
-  ++GL(dl_load_adds);
-
-  __rtld_lock_unlock_recursive (GL(dl_load_write_lock));
+    /* Add the global scope.  */
+    new->l_scope[idx++] = &GL(dl_ns)[nsid]._ns_loaded->l_searchlist;
 
   /* If we have no loader the new object acts as it.  */
   if (loader == NULL)
Index: glibc/elf/rtld.c
===================================================================
--- glibc.orig/elf/rtld.c
+++ glibc/elf/rtld.c
@@ -1109,6 +1109,7 @@ of this helper program; chances are you
 	 This will be what dlopen on "" returns.  */
       main_map = _dl_new_object ((char *) "", "", lt_executable, NULL,
 				 __RTLD_OPENEXEC, LM_ID_BASE);
+      _dl_add_to_namespace_list (main_map, LM_ID_BASE);
       assert (main_map != NULL);
       assert (main_map == GL(dl_ns)[LM_ID_BASE]._ns_loaded);
       main_map->l_phdr = phdr;
@@ -1320,6 +1321,7 @@ of this helper program; chances are you
 	 mapped and relocated it normally.  */
       struct link_map *l = _dl_new_object ((char *) "", "", lt_library, NULL,
 					   0, LM_ID_BASE);
+      _dl_add_to_namespace_list (l, LM_ID_BASE);
       if (__builtin_expect (l != NULL, 1))
 	{
 	  static ElfW(Dyn) dyn_temp[DL_RO_DYN_TEMP_CNT] attribute_relro;
Index: glibc/sysdeps/generic/ldsodefs.h
===================================================================
--- glibc.orig/sysdeps/generic/ldsodefs.h
+++ glibc/sysdeps/generic/ldsodefs.h
@@ -893,8 +893,11 @@ extern lookup_t _dl_lookup_symbol_x (con
 extern ElfW(Addr) _dl_symbol_value (struct link_map *map, const char *name)
      internal_function;
 
-/* Allocate a `struct link_map' for a new object being loaded,
-   and enter it into the _dl_main_map list.  */
+/* Add the new link_map NEW to the end of the namespace list.  */
+extern void _dl_add_to_namespace_list (struct link_map *new, Lmid_t nsid)
+     internal_function attribute_hidden;
+
+/* Allocate a `struct link_map' for a new object being loaded.  */
 extern struct link_map *_dl_new_object (char *realname, const char *libname,
 					int type, struct link_map *loader,
 					int mode, Lmid_t nsid)
Index: glibc/elf/dl-load.c
===================================================================
--- glibc.orig/elf/dl-load.c
+++ glibc/elf/dl-load.c
@@ -799,21 +799,7 @@ lose (int code, int fd, const char *name
   if (fd != -1)
     (void) __close (fd);
   if (l != NULL)
-    {
-      /* We modify the list of loaded objects.  */
-      __rtld_lock_lock_recursive (GL(dl_load_write_lock));
-      /* Remove the stillborn object from the list and free it.  */
-      assert (l->l_next == NULL);
-      if (l->l_prev == NULL)
-	/* No other module loaded. This happens only in the static library,
-	   or in rtld under --verify.  */
-	GL(dl_ns)[l->l_ns]._ns_loaded = NULL;
-      else
-	l->l_prev->l_next = NULL;
-      --GL(dl_ns)[l->l_ns]._ns_nloaded;
-      free (l);
-      __rtld_lock_unlock_recursive (GL(dl_load_write_lock));
-    }
+    free (l);
   free (realname);
 
   if (r != NULL)
@@ -898,6 +884,8 @@ _dl_map_object_from_fd (const char *name
 	 never be unloaded.  */
       __close (fd);
 
+      _dl_add_to_namespace_list (l, nsid);
+
       return l;
     }
 #endif
@@ -1492,6 +1480,8 @@ cannot enable executable stack as shared
     add_name_to_object (l, ((const char *) D_PTR (l, l_info[DT_STRTAB])
 			    + l->l_info[DT_SONAME]->d_un.d_val));
 
+  _dl_add_to_namespace_list (l, nsid);
+
 #ifdef SHARED
   /* Auditing checkpoint: we have a new object.  */
   if (__builtin_expect (GLRO(dl_naudit) > 0, 0)
@@ -2232,6 +2222,8 @@ _dl_map_object (struct link_map *loader,
 	  l->l_nbuckets = 1;
 	  l->l_relocated = 1;
 
+	  _dl_add_to_namespace_list (l, nsid);
+
 	  return l;
 	}
       else if (found_other_class)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]