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]

[PATCHv5][PING][BZ #17833] _dl_close_worker() does not release inconsistent objects.


On 05/29/2015 08:38 PM, Pavel Kopyl wrote:
On 05/29/2015 12:31 AM, Pavel Kopyl wrote:
On 05/28/2015 01:06 AM, H.J. Lu wrote:
On Wed, May 27, 2015 at 2:38 PM, Pavel Kopyl <p.kopyl@samsung.com>
wrote:
AFAIU DF_1_NODELETE change is necessary. Otherwise unique
symbols force
library to remain loaded in half-initialized state (with missing
dependencies). This usecase is demonstrated by the testcase in
patch.

But unique symbols != DF_1_NODELETE:

DF_1_NODELETE is set in do_lookup_unique at runtime (Paul, am I
right?):

        enter_unique_sym (entries, size,
                          new_hash, strtab + sym->st_name, sym, map);

        if (map->l_type == lt_loaded)
          /* Make sure we don't unload this object by
             setting the appropriate flag.  */
          ((struct link_map *) map)->l_flags_1 |= DF_1_NODELETE;

-Y
Should we set DF_1_NODELETE when dlopen fails?  What happens
when dlopen a  DF_1_NODELETE fails?  Do we keep it in memory
even when dopen fails?

As I know there are following ways how to set DF_1_NODELETE flag for a
library.

1. Unique symbols.
2, Load with RTLD_NODELETE flag.
3. Link with "-z nodelete" option.

As for RTLD_NODELETE, there is no problem: dlopen exits in case of
error
before DF_1_NODELETE is set.
If dlopen fails both in first and third cases this leads to locking
libraries in half-initialized state. I think this is a bug.
Suggested patch fixes both usecases.
Assume there is a problem with DF_1_NODELETE, which can be
set explicitly as well as implicitly.  Your testcase only covers
implicitly DF_1_NODELETE, not explicitly  DF_1_NODELETE.
I think we should verify the status of explicitly DF_1_NODELETE.

If explicitly  DF_1_NODELETE works fine, then we only have a
problem with implicitly DF_1_NODELETE.

If explicitly  DF_1_NODELETE doesn't work, we need to fix it
first and implicitly DF_1_NODELETE may just work.

Ok, I prepared the following testcase.

tst-nodelete.cc:
#include <stdio.h>
#include <stdlib.h>
#include <dlfcn.h>

int
main (void)
{
  int ret = 0;

  /* This is a test for correct handling of dlopen failures for
library that
     is loaded with RTLD_NODELETE flag. The first dlopen should fail
because
     of undefined symbols in shared library. The second dlopen then
verifies
     that library was properly unloaded.  */
  if (dlopen ("./tst-nodeletelib.so", RTLD_NOW | RTLD_NODELETE) != NULL
      || dlopen ("./tst-nodeletelib.so", RTLD_LAZY | RTLD_NOLOAD) !=
NULL)
    {
      printf("RTLD_NOLOAD test failed\n");
      ret = 1;
    }

  /* This is a test for correct handling of dlopen failures for
library that
     is linked with '-z nodelete' option and hence has DF_1_NODELETE
flag.
     The first dlopen should fail because of undefined symbols in shared
     library. The second dlopen then verifies that library was properly
     unloaded.  */
  if (dlopen ("./tst-znodeletelib.so", RTLD_NOW) != NULL
      || dlopen ("./tst-znodeletelib.so", RTLD_LAZY | RTLD_NOLOAD) !=
NULL)
    {
      printf("-z \'nodelete\' test failed\n");
      ret = 1;
    }

   /* This is a test for correct handling of dlopen failures for library
     with unique symbols. The first dlopen should fail because of
undefined
     symbols in shared library. The second dlopen then verifies that
library
     was properly unloaded.  */
  if (dlopen ("./tst-unique5lib.so", RTLD_NOW) != NULL
      || dlopen ("./tst-unique5lib.so", RTLD_LAZY | RTLD_NOLOAD) != NULL)
    {
      printf("Unique symbols test failed\n");
      ret = 1;
    }

  if (!ret)
    printf ("SUCCESS\n");

  return ret;
}


tst-unique5lib.cc:
extern int not_exist (void);

inline int make_unique (void)
{
/* Static variables in inline functions and classes
   generate STB_GNU_UNIQUE symbols.  */
  static int unique;
  return ++unique;
}

int foo (void)
{
  return make_unique () + not_exist ();
}


tst-nodeletelib.cc:
extern int not_exist (void);

int foo (void)
{
  return  not_exist ();
}


g++ -o tst-nodelete tst-nodelete.cc -ldl
g++ -shared -fPIC -o tst-nodeletelib.so tst-nodeletelib.cc
g++ -shared -fPIC -Wl,-z,nodelete -o tst-znodeletelib.so
tst-nodeletelib.cc
./tst-nodelete
-z 'nodelete' test failed
Unique symbols test failed


Thus, besides unique symbols we have a problem with explicitly
DF_1_NODELETE when library is built with "-Wl,-z,nodelete".
I checked that patch fixes this usecase too. Now I'm going to update
testcase in make check.

This is new patch with updated testcase.

-Pavel

Ping.
commit eebadd0f414fe6aa5163a184ddae35c57a5fa454
Author: Pavel Kopyl <p.kopyl@samsung.com>
Date:   Fri May 29 20:15:33 2015 +0300

    2015-05-29  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-nodelete.cc: Test executable.  New file.
    	* elf/tst-nodeletelib.cc: Test library.  New file.
    	* elf/tst-znodeletelib.cc: Likewise.
    	* include/dlfcn.h (_dl_close_worker): Add new parameter.

diff --git a/elf/Makefile b/elf/Makefile
index dedf3c7..a46eaff 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -131,7 +131,7 @@ tests += $(tests-static)
 ifeq (yes,$(build-shared))
 tests += loadtest restest1 preloadtest loadfail multiload origtest resolvfail \
 	 constload1 order noload filter unload \
-	 reldep reldep2 reldep3 reldep4 nodelete nodelete2 \
+	 reldep reldep2 reldep3 reldep4 nodelete nodelete2 tst-nodelete \
 	 nodlopen nodlopen2 neededtest neededtest2 \
 	 neededtest3 neededtest4 unload2 lateglobal initfirst global \
 	 restest2 next dblload dblunload reldep5 reldep6 reldep7 reldep8 \
@@ -205,7 +205,9 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
 		tst-unique1mod1 tst-unique1mod2 \
 		tst-unique2mod1 tst-unique2mod2 \
 		tst-auditmod9a tst-auditmod9b \
-		$(if $(CXX),tst-unique3lib tst-unique3lib2 tst-unique4lib) \
+		$(if $(CXX),tst-unique3lib tst-unique3lib2 tst-unique4lib \
+		  tst-nodelete-uniquemod) \
+		tst-nodelete-rtldmod tst-nodelete-zmod \
 		tst-initordera1 tst-initorderb1 \
 		tst-initordera2 tst-initorderb2 \
 		tst-initordera3 tst-initordera4 \
@@ -592,6 +594,9 @@ 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-nodelete-uniquemod.so-no-z-defs = yes
+tst-nodelete-rtldmod.so-no-z-defs = yes
+tst-nodelete-zmod.so-no-z-defs = yes
 
 ifeq ($(build-shared),yes)
 # Build all the modules even when not actually running test programs.
@@ -1154,6 +1159,13 @@ $(objpfx)tst-unique3.out: $(objpfx)tst-unique3lib2.so
 
 $(objpfx)tst-unique4: $(objpfx)tst-unique4lib.so
 
+$(objpfx)tst-nodelete: $(libdl)
+$(objpfx)tst-nodelete.out: $(objpfx)tst-nodelete-uniquemod.so \
+			   $(objpfx)tst-nodelete-rtldmod.so \
+			   $(objpfx)tst-nodelete-zmod.so
+
+LDFLAGS-tst-nodelete = -rdynamic
+LDFLAGS-tst-nodelete-zmod.so = -Wl,--enable-new-dtags,-z,nodelete
 $(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 412f71d..0595675 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.  */
@@ -782,7 +811,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 2d0e082..027c1e0 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -670,7 +670,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-nodelete-rtldmod.cc b/elf/tst-nodelete-rtldmod.cc
new file mode 100644
index 0000000..740e1d8
--- /dev/null
+++ b/elf/tst-nodelete-rtldmod.cc
@@ -0,0 +1,6 @@
+extern int not_exist (void);
+
+int foo (void)
+{
+  return not_exist ();
+}
diff --git a/elf/tst-nodelete-uniquemod.cc b/elf/tst-nodelete-uniquemod.cc
new file mode 100644
index 0000000..632b303
--- /dev/null
+++ b/elf/tst-nodelete-uniquemod.cc
@@ -0,0 +1,14 @@
+extern int not_exist (void);
+
+inline int make_unique (void)
+{
+  /* Static variables in inline functions and classes
+     generate STB_GNU_UNIQUE symbols.  */
+  static int unique;
+  return ++unique;
+}
+
+int foo (void)
+{
+  return make_unique () + not_exist ();
+}
diff --git a/elf/tst-nodelete-zmod.cc b/elf/tst-nodelete-zmod.cc
new file mode 100644
index 0000000..740e1d8
--- /dev/null
+++ b/elf/tst-nodelete-zmod.cc
@@ -0,0 +1,6 @@
+extern int not_exist (void);
+
+int foo (void)
+{
+  return not_exist ();
+}
diff --git a/elf/tst-nodelete.cc b/elf/tst-nodelete.cc
new file mode 100644
index 0000000..176cb68
--- /dev/null
+++ b/elf/tst-nodelete.cc
@@ -0,0 +1,51 @@
+#include "../dlfcn/dlfcn.h"
+#include <stdio.h>
+#include <stdlib.h>
+
+static int
+do_test (void)
+{
+  int result = 0;
+
+  /* This is a test for correct handling of dlopen failures for library that
+     is loaded with RTLD_NODELETE flag.  The first dlopen should fail because
+     of undefined symbols in shared library.  The second dlopen then verifies
+     that library was properly unloaded.  */
+  if (dlopen ("tst-nodelete-rtldmod.so", RTLD_NOW | RTLD_NODELETE) != NULL
+      || dlopen ("tst-nodelete-rtldmod.so", RTLD_LAZY | RTLD_NOLOAD) != NULL)
+    {
+      printf ("RTLD_NODELETE test failed\n");
+      result = 1;
+    }
+
+  /* This is a test for correct handling of dlopen failures for library that
+     is linked with '-z nodelete' option and hence has DF_1_NODELETE flag.
+     The first dlopen should fail because of undefined symbols in shared
+     library.  The second dlopen then verifies that library was properly
+     unloaded.  */
+  if (dlopen ("tst-nodelete-zmod.so", RTLD_NOW) != NULL
+      || dlopen ("tst-nodelete-zmod.so", RTLD_LAZY | RTLD_NOLOAD) != NULL)
+    {
+      printf ("-z nodelete test failed\n");
+      result = 1;
+    }
+
+   /* This is a test for correct handling of dlopen failures for library
+     with unique symbols.  The first dlopen should fail because of undefined
+     symbols in shared library.  The second dlopen then verifies that library
+     was properly unloaded.  */
+  if (dlopen ("tst-nodelete-uniquemod.so", RTLD_NOW) != NULL
+      || dlopen ("tst-nodelete-uniquemod.so", RTLD_LAZY | RTLD_NOLOAD) != NULL)
+    {
+      printf ("Unique symbols test failed\n");
+      result = 1;
+    }
+
+  if (result == 0)
+    printf ("SUCCESS\n");
+
+  return result;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/elf/tst-znodelete-zlib.cc b/elf/tst-znodelete-zlib.cc
new file mode 100644
index 0000000..1e8f368
--- /dev/null
+++ b/elf/tst-znodelete-zlib.cc
@@ -0,0 +1,6 @@
+extern int not_exist (void);
+
+int foo (void)
+{
+  return  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]