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][V2][BZ #17833][PING] _dl_close_worker() does not release inconsistent objects.



On 02/12/2015 12:04 AM, Pavel Kopyl wrote:

On 01/21/2015 03:52 AM, Carlos O'Donell wrote:
Hello,
>
>I faced with the following problem. [See test case:
>https://sourceware.org/bugzilla/show_bug.cgi?id=17833]
>
>I've a shared library that contains both undefined and unique
>symbols. Then I try to call the following sequence of dlopen:
>
>1. dlopen("./libfoo.so", RTLD_NOW)
>2. dlopen("./libfoo.so", RTLD_LAZY | RTLD_GLOBAL)
>
>First dlopen call terminates with error because of undefined symbols,
>but STB_GNU_UNIQUE ones set DF_1_NODELETE flag and hence block
>library in the memory. The library goes into inconsistent state as
>several structures remain uninitialized. For instance, relocations
>for GOT table were not performed. By the time of second dlopen call
>this library looks like as it would be fully initialized but this is
>not true: any call through incorrect GOT table leads to segmentation
>fault. On some systems this inconsistency triggers assertions in the
>dynamic linker.
>
>Suggested patch forces object deletion in case of dlopen() fail:
>
>1. Clears DF_1_NODELETE flag if dlopen() fails, allowing library to be
>    deleted from memory.
>2. For each unique symbol that is defined in this object clears
>    appropriate entry in _ns_unique_sym_table.
>From a high level perspective your patch looks plausibly correct.

I expect no other threads can have possibly seen the result of the
dlopen() because it's not yet complete, and so it's OK to clear
the STB_GNU_UNIQUE symbols list because none are yet used. Did you
verify this?

The solution you've chosen looks good, failing at dlopen time and
cleaning up the resulting changes is good.

This change should go into glibc 2.22 (February when branch opens).
I don't want to make this kind of internals change in dlopen right
now without having a full release to make sure we didn't break anything.

As another reviewer points out, we need a test case for this.

Please repost a v2 with a regression test.

Thanks for review.
Here is patch v2 with regression test

Ping.
commit 386f49f30c90af764b720cc6ef01fc559a63de35
Author: Pavel Kopyl <p.kopyl@samsung.com>
Date:   Wed Feb 11 18:24:07 2015 +0300

    2015-02-11  Pavel Kopyl  <p.kopyl@samsung.com>
    	    Mikhail Ilin  <m.ilin@samsung.com>
    
    	[BZ #17833]
    	* elf/Makefile: Add build test commands.
    	* elf/dl-close.c (_dl_close_worker): Implement force object deletion.
    	* elf/dl-open.c (_dl_open): Forced _dl_close_worker call.
    	(_dl_close): Default _dl_close_worker call.
    	* elf/tst-unique5.cc: Test executable.  New file.
    	* elf/tst-unique5lib.cc: Test library.  New file.
    	* include/dlfcn.h (_dl_close_worker): Add new parameter.

diff --git a/elf/Makefile b/elf/Makefile
index f2d1781..2a186ba 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -144,7 +144,7 @@ tests += loadtest restest1 preloadtest loadfail multiload origtest resolvfail \
 	 unload3 unload4 unload5 unload6 unload7 unload8 tst-global1 order2 \
 	 tst-audit1 tst-audit2 tst-audit8 tst-audit9 \
 	 tst-stackguard1 tst-addr1 tst-thrlock \
-	 tst-unique1 tst-unique2 tst-unique3 tst-unique4 \
+	 tst-unique1 tst-unique2 tst-unique3 tst-unique4 tst-unique5 \
 	 tst-initorder tst-initorder2 tst-relsort1 tst-null-argv \
 	 tst-ptrguard1
 #	 reldep9
@@ -206,7 +206,7 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
 		tst-unique2mod1 tst-unique2mod2 \
 		tst-auditmod9a tst-auditmod9b \
 		tst-unique3lib tst-unique3lib2 \
-		tst-unique4lib \
+		tst-unique4lib tst-unique5lib \
 		tst-initordera1 tst-initorderb1 \
 		tst-initordera2 tst-initorderb2 \
 		tst-initordera3 tst-initordera4 \
@@ -576,6 +576,7 @@ ifuncmod5.so-no-z-defs = yes
 ifuncmod6.so-no-z-defs = yes
 tst-auditmod9a.so-no-z-defs = yes
 tst-auditmod9b.so-no-z-defs = yes
+tst-unique5lib.so-no-z-defs = yes
 
 ifeq ($(build-shared),yes)
 # Build all the modules even when not actually running test programs.
@@ -1132,6 +1133,9 @@ $(objpfx)tst-unique3.out: $(objpfx)tst-unique3lib2.so
 
 $(objpfx)tst-unique4: $(objpfx)tst-unique4lib.so
 
+$(objpfx)tst-unique5: $(libdl)
+$(objpfx)tst-unique5.out: $(objpfx)tst-unique5lib.so
+
 $(objpfx)tst-initorder-cmp.out: tst-initorder.exp $(objpfx)tst-initorder.out
 	cmp $^ > $@; \
 	$(evaluate-test)
diff --git a/elf/dl-close.c b/elf/dl-close.c
index cf8f9e0..d640254 100644
--- a/elf/dl-close.c
+++ b/elf/dl-close.c
@@ -108,7 +108,7 @@ remove_slotinfo (size_t idx, struct dtv_slotinfo_list *listp, size_t disp,
 
 
 void
-_dl_close_worker (struct link_map *map)
+_dl_close_worker (struct link_map *map, bool force)
 {
   /* One less direct use.  */
   --map->l_direct_opencount;
@@ -152,6 +152,10 @@ _dl_close_worker (struct link_map *map)
       l->l_idx = idx;
       maps[idx] = l;
       ++idx;
+
+      /* clear DF_1_NODELETE to force object deletion.  */
+      if (force)
+	l->l_flags_1 &= ~DF_1_NODELETE;
     }
   assert (idx == nloaded);
 
@@ -635,6 +639,31 @@ _dl_close_worker (struct link_map *map)
 		}
 	    }
 
+	  /* Reset unique symbols if forced.  */
+	  if (force)
+	    {
+	      struct unique_sym_table *tab = &ns->_ns_unique_sym_table;
+	      __rtld_lock_lock_recursive (tab->lock);
+	      struct unique_sym *entries = tab->entries;
+	      if (entries != NULL)
+		{
+		  size_t idx, size = tab->size;
+		  for (idx = 0; idx < size; ++idx)
+		    {
+		      /* Clear unique symbol entries
+		       that belong to this object.  */
+		      if (entries[idx].name != NULL
+			  && entries[idx].map == imap)
+			{
+			  entries[idx].name = NULL;
+			  entries[idx].hashval = 0;
+			  tab->n_elements--;
+			}
+		    }
+		}
+		__rtld_lock_unlock_recursive (tab->lock);
+	    }
+
 	  /* We can unmap all the maps at once.  We determined the
 	     start address and length when we loaded the object and
 	     the `munmap' call does the rest.  */
@@ -772,7 +801,7 @@ _dl_close (void *_map)
   /* Acquire the lock.  */
   __rtld_lock_lock_recursive (GL(dl_load_lock));
 
-  _dl_close_worker (map);
+  _dl_close_worker (map, false);
 
   __rtld_lock_unlock_recursive (GL(dl_load_lock));
 }
diff --git a/elf/dl-open.c b/elf/dl-open.c
index 47b4cb5..d71c454 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -674,7 +674,7 @@ no more namespaces available for dlmopen()"));
 	  if ((mode & __RTLD_AUDIT) == 0)
 	    GL(dl_tls_dtv_gaps) = true;
 
-	  _dl_close_worker (args.map);
+	  _dl_close_worker (args.map, true);
 	}
 
       assert (_dl_debug_initialize (0, args.nsid)->r_state == RT_CONSISTENT);
diff --git a/elf/tst-unique5.cc b/elf/tst-unique5.cc
new file mode 100644
index 0000000..95a25d6
--- /dev/null
+++ b/elf/tst-unique5.cc
@@ -0,0 +1,11 @@
+#include "../dlfcn/dlfcn.h"
+#include <stdlib.h>
+
+int
+main (void)
+{
+  void *h;
+  dlopen ("$ORIGIN/tst-unique5lib.so", RTLD_NOW);
+  h = dlopen ("$ORIGIN/tst-unique5lib.so", RTLD_LAZY | RTLD_NOLOAD);
+  return h != NULL;
+}
diff --git a/elf/tst-unique5lib.cc b/elf/tst-unique5lib.cc
new file mode 100644
index 0000000..7d17c3b
--- /dev/null
+++ b/elf/tst-unique5lib.cc
@@ -0,0 +1,13 @@
+
+extern int not_exist ();
+
+inline int make_unique ()
+{
+  static int unique;
+  return ++unique;
+}
+
+int foo ()
+{
+  return make_unique () + not_exist ();
+}
diff --git a/include/dlfcn.h b/include/dlfcn.h
index a67b2e3..0ce0af5 100644
--- a/include/dlfcn.h
+++ b/include/dlfcn.h
@@ -54,7 +54,8 @@ struct link_map;
 extern void _dl_close (void *map) attribute_hidden;
 /* Same as above, but without locking and safety checks for user
    provided map arguments.  */
-extern void _dl_close_worker (struct link_map *map) attribute_hidden;
+extern void _dl_close_worker (struct link_map *map, bool force)
+    attribute_hidden;
 
 /* Look up NAME in shared object HANDLE (which may be RTLD_DEFAULT or
    RTLD_NEXT).  WHO is the calling function, for RTLD_NEXT.  Returns

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