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

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

[Pavel Kopyl]

[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]

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.


Thanks

Pavel



[-- Attachment #2: delete_inconsistent_objs.patch --]
[-- Type: text/x-patch, Size: 3237 bytes --]

commit 10e53df26cdd232bfe38b449add8cb238ee108b8
Author: Pavel Kopyl <p.kopyl@samsung.com>
Date:   Mon Jan 19 18:05:02 2015 +0300

    2015-01-15  Pavel Kopyl  <p.kopyl@samsung.com>
    	    Mikhail Ilin  m.ilin@samsung.com
    
    	[BZ #17833]
    	* elf/dl-open.c (_dl_open): Forced _dl_close_worker call.
    	(_dl_close): default _dl_close_worker call.
    	* include/dlfcn.h (_dl_close_worker): Add new parameter.

diff --git a/elf/dl-close.c b/elf/dl-close.c
index cf8f9e0..3728d6d 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, int 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, 0);
 
   __rtld_lock_unlock_recursive (GL(dl_load_lock));
 }
diff --git a/elf/dl-open.c b/elf/dl-open.c
index c358fff..2e245e6 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -672,7 +672,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, 1);
 	}
 
       assert (_dl_debug_initialize (0, args.nsid)->r_state == RT_CONSISTENT);
diff --git a/include/dlfcn.h b/include/dlfcn.h
index a67b2e3..cc1e2c0 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, int 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

Re: [PATCH][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[Yury Gribov]

On 01/19/2015 09:39 PM, Pavel Kopyl wrote:
> Suggested patch forces object deletion in case of dlopen() fail:

Cc Mike. Could you add a testcase?

-Y

Re: [PATCH][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[Carlos O'Donell]

On 01/19/2015 01:39 PM, Pavel Kopyl 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.

Good work on the patch.

Cheers,
Carlos.

[PATCH][V2][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[Pavel Kopyl]

[-- Attachment #1: Type: text/plain, Size: 2125 bytes --]


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.


Regards,
Pavel


[-- Attachment #2: delete_inconsistent_objs.patch --]
[-- Type: text/x-patch, Size: 5754 bytes --]

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

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

[Pavel Kopyl]

[-- Attachment #1: Type: text/plain, Size: 2209 bytes --]


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.

[-- Attachment #2: delete_inconsistent_objs.patch --]
[-- Type: text/x-patch, Size: 5754 bytes --]

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

Re: [PATCH][V2][BZ #17833][PING] _dl_close_worker() does not release inconsistent objects.

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 654 bytes --]

On 27 Feb 2015 16:32, Pavel Kopyl wrote:
> --- /dev/null
> +++ b/elf/tst-unique5lib.cc
> @@ -0,0 +1,13 @@
> +

i know existing tests are bad examples, but lets try and start fixing that.  
namely, there should be a header here giving a quick overview of what it is 
exactly you're testing for, and a BZ reference.

> +extern int not_exist ();
> +
> +inline int make_unique ()
> +{
> +  static int unique;
> +  return ++unique;
> +}
> +
> +int foo ()
> +{
> +  return make_unique () + not_exist ();
> +}

i don't know if this is just copy & pasting, but prototypes that do not intend 
to take args should always be (void).
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH][V3][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[Pavel Kopyl]

[-- Attachment #1: Type: text/plain, Size: 764 bytes --]


On 03/01/2015 10:17 PM, Mike Frysinger wrote:
> On 27 Feb 2015 16:32, Pavel Kopyl wrote:
>> --- /dev/null
>> +++ b/elf/tst-unique5lib.cc
>> @@ -0,0 +1,13 @@
>> +
> i know existing tests are bad examples, but lets try and start fixing that.
> namely, there should be a header here giving a quick overview of what it is
> exactly you're testing for, and a BZ reference.
>
>> +extern int not_exist ();
>> +
>> +inline int make_unique ()
>> +{
>> +  static int unique;
>> +  return ++unique;
>> +}
>> +
>> +int foo ()
>> +{
>> +  return make_unique () + not_exist ();
>> +}
> i don't know if this is just copy & pasting, but prototypes that do not intend
> to take args should always be (void).
> -mike

Thanks for review, I fixed that in patch v3.

Regards,
Pavel



[-- Attachment #2: delete_inconsistent_objs_v3.patch --]
[-- Type: text/x-patch, Size: 6054 bytes --]

commit 0d8ff75e18fad28123dca0df29f23942299dac56
Author: Pavel Kopyl <p.kopyl@samsung.com>
Date:   Mon Mar 2 23:05:48 2015 +0300

    2015-03-02  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 711beed..f3aaf4d 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 \
@@ -580,6 +580,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.
@@ -1138,6 +1139,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..628897a
--- /dev/null
+++ b/elf/tst-unique5.cc
@@ -0,0 +1,15 @@
+#include "../dlfcn/dlfcn.h"
+#include <stdlib.h>
+
+/* unique5lib.so library contains both STB_GNU_UNIQUE and undefined symbols.
+   First dlopen call fails because of undefined symbols.  Test checks that
+   library does not remain locked in the memory in this case. [BZ #17833]  */
+
+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..c292a23
--- /dev/null
+++ b/elf/tst-unique5lib.cc
@@ -0,0 +1,14 @@
+/* tst-unique5lib.so library for tst-unique5.cc.  */
+
+extern int not_exist (void);
+
+inline int make_unique (void)
+{
+  static int unique;
+  return ++unique;
+}
+
+int foo (void)
+{
+  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

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

[Pavel Kopyl]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]



On 03/03/2015 12:13 PM, Pavel Kopyl wrote:
>
> On 03/01/2015 10:17 PM, Mike Frysinger wrote:
>> On 27 Feb 2015 16:32, Pavel Kopyl wrote:
>>> --- /dev/null
>>> +++ b/elf/tst-unique5lib.cc
>>> @@ -0,0 +1,13 @@
>>> +
>> i know existing tests are bad examples, but lets try and start fixing 
>> that.
>> namely, there should be a header here giving a quick overview of what 
>> it is
>> exactly you're testing for, and a BZ reference.
>>
>>> +extern int not_exist ();
>>> +
>>> +inline int make_unique ()
>>> +{
>>> +  static int unique;
>>> +  return ++unique;
>>> +}
>>> +
>>> +int foo ()
>>> +{
>>> +  return make_unique () + not_exist ();
>>> +}
>> i don't know if this is just copy & pasting, but prototypes that do 
>> not intend
>> to take args should always be (void).
>> -mike
>
> Thanks for review, I fixed that in patch v3.

Ping.


[-- Attachment #2: delete_inconsistent_objs_v3.patch --]
[-- Type: text/x-patch, Size: 6054 bytes --]

commit 0d8ff75e18fad28123dca0df29f23942299dac56
Author: Pavel Kopyl <p.kopyl@samsung.com>
Date:   Mon Mar 2 23:05:48 2015 +0300

    2015-03-02  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 711beed..f3aaf4d 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 \
@@ -580,6 +580,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.
@@ -1138,6 +1139,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..628897a
--- /dev/null
+++ b/elf/tst-unique5.cc
@@ -0,0 +1,15 @@
+#include "../dlfcn/dlfcn.h"
+#include <stdlib.h>
+
+/* unique5lib.so library contains both STB_GNU_UNIQUE and undefined symbols.
+   First dlopen call fails because of undefined symbols.  Test checks that
+   library does not remain locked in the memory in this case. [BZ #17833]  */
+
+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..c292a23
--- /dev/null
+++ b/elf/tst-unique5lib.cc
@@ -0,0 +1,14 @@
+/* tst-unique5lib.so library for tst-unique5.cc.  */
+
+extern int not_exist (void);
+
+inline int make_unique (void)
+{
+  static int unique;
+  return ++unique;
+}
+
+int foo (void)
+{
+  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

Re: [PATCH][V3][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 132 bytes --]

patch looks ok to me, but i'm not super familiar with the dlfcn code, so you 
probably need someone else to sign off on it
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCHv3][PING^2][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[Pavel Kopyl]

[-- Attachment #1: Type: text/plain, Size: 840 bytes --]



On 03/03/2015 12:13 PM, Pavel Kopyl wrote:
>
> On 03/01/2015 10:17 PM, Mike Frysinger wrote:
>> On 27 Feb 2015 16:32, Pavel Kopyl wrote:
>>> --- /dev/null
>>> +++ b/elf/tst-unique5lib.cc
>>> @@ -0,0 +1,13 @@
>>> +
>> i know existing tests are bad examples, but lets try and start fixing 
>> that.
>> namely, there should be a header here giving a quick overview of what 
>> it is
>> exactly you're testing for, and a BZ reference.
>>
>>> +extern int not_exist ();
>>> +
>>> +inline int make_unique ()
>>> +{
>>> +  static int unique;
>>> +  return ++unique;
>>> +}
>>> +
>>> +int foo ()
>>> +{
>>> +  return make_unique () + not_exist ();
>>> +}
>> i don't know if this is just copy & pasting, but prototypes that do 
>> not intend
>> to take args should always be (void).
>> -mike
>
> Thanks for review, I fixed that in patch v3.

Ping.

[-- Attachment #2: delete_inconsistent_objs_v3.patch --]
[-- Type: text/x-patch, Size: 6054 bytes --]

commit 0d8ff75e18fad28123dca0df29f23942299dac56
Author: Pavel Kopyl <p.kopyl@samsung.com>
Date:   Mon Mar 2 23:05:48 2015 +0300

    2015-03-02  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 711beed..f3aaf4d 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 \
@@ -580,6 +580,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.
@@ -1138,6 +1139,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..628897a
--- /dev/null
+++ b/elf/tst-unique5.cc
@@ -0,0 +1,15 @@
+#include "../dlfcn/dlfcn.h"
+#include <stdlib.h>
+
+/* unique5lib.so library contains both STB_GNU_UNIQUE and undefined symbols.
+   First dlopen call fails because of undefined symbols.  Test checks that
+   library does not remain locked in the memory in this case. [BZ #17833]  */
+
+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..c292a23
--- /dev/null
+++ b/elf/tst-unique5lib.cc
@@ -0,0 +1,14 @@
+/* tst-unique5lib.so library for tst-unique5.cc.  */
+
+extern int not_exist (void);
+
+inline int make_unique (void)
+{
+  static int unique;
+  return ++unique;
+}
+
+int foo (void)
+{
+  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

[PATCHv3][PING^3][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[Pavel Kopyl]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]



On 03/03/2015 12:13 PM, Pavel Kopyl wrote:
>
> On 03/01/2015 10:17 PM, Mike Frysinger wrote:
>> On 27 Feb 2015 16:32, Pavel Kopyl wrote:
>>> --- /dev/null
>>> +++ b/elf/tst-unique5lib.cc
>>> @@ -0,0 +1,13 @@
>>> +
>> i know existing tests are bad examples, but lets try and start fixing 
>> that.
>> namely, there should be a header here giving a quick overview of what 
>> it is
>> exactly you're testing for, and a BZ reference.
>>
>>> +extern int not_exist ();
>>> +
>>> +inline int make_unique ()
>>> +{
>>> +  static int unique;
>>> +  return ++unique;
>>> +}
>>> +
>>> +int foo ()
>>> +{
>>> +  return make_unique () + not_exist ();
>>> +}
>> i don't know if this is just copy & pasting, but prototypes that do 
>> not intend
>> to take args should always be (void).
>> -mike
>
> Thanks for review, I fixed that in patch v3.

Ping.


[-- Attachment #2: delete_inconsistent_objs_v3.patch --]
[-- Type: text/x-patch, Size: 6054 bytes --]

commit 0d8ff75e18fad28123dca0df29f23942299dac56
Author: Pavel Kopyl <p.kopyl@samsung.com>
Date:   Mon Mar 2 23:05:48 2015 +0300

    2015-03-02  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 711beed..f3aaf4d 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 \
@@ -580,6 +580,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.
@@ -1138,6 +1139,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..628897a
--- /dev/null
+++ b/elf/tst-unique5.cc
@@ -0,0 +1,15 @@
+#include "../dlfcn/dlfcn.h"
+#include <stdlib.h>
+
+/* unique5lib.so library contains both STB_GNU_UNIQUE and undefined symbols.
+   First dlopen call fails because of undefined symbols.  Test checks that
+   library does not remain locked in the memory in this case. [BZ #17833]  */
+
+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..c292a23
--- /dev/null
+++ b/elf/tst-unique5lib.cc
@@ -0,0 +1,14 @@
+/* tst-unique5lib.so library for tst-unique5.cc.  */
+
+extern int not_exist (void);
+
+inline int make_unique (void)
+{
+  static int unique;
+  return ++unique;
+}
+
+int foo (void)
+{
+  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

[PATCHv3][PING^4][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[Pavel Kopyl]

[-- Attachment #1: Type: text/plain, Size: 840 bytes --]



On 03/03/2015 12:13 PM, Pavel Kopyl wrote:
>
> On 03/01/2015 10:17 PM, Mike Frysinger wrote:
>> On 27 Feb 2015 16:32, Pavel Kopyl wrote:
>>> --- /dev/null
>>> +++ b/elf/tst-unique5lib.cc
>>> @@ -0,0 +1,13 @@
>>> +
>> i know existing tests are bad examples, but lets try and start fixing 
>> that.
>> namely, there should be a header here giving a quick overview of what 
>> it is
>> exactly you're testing for, and a BZ reference.
>>
>>> +extern int not_exist ();
>>> +
>>> +inline int make_unique ()
>>> +{
>>> +  static int unique;
>>> +  return ++unique;
>>> +}
>>> +
>>> +int foo ()
>>> +{
>>> +  return make_unique () + not_exist ();
>>> +}
>> i don't know if this is just copy & pasting, but prototypes that do 
>> not intend
>> to take args should always be (void).
>> -mike
>
> Thanks for review, I fixed that in patch v3.
Ping.


[-- Attachment #2: delete_inconsistent_objs_v3.patch --]
[-- Type: text/x-patch, Size: 6054 bytes --]

commit 0d8ff75e18fad28123dca0df29f23942299dac56
Author: Pavel Kopyl <p.kopyl@samsung.com>
Date:   Mon Mar 2 23:05:48 2015 +0300

    2015-03-02  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 711beed..f3aaf4d 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 \
@@ -580,6 +580,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.
@@ -1138,6 +1139,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..628897a
--- /dev/null
+++ b/elf/tst-unique5.cc
@@ -0,0 +1,15 @@
+#include "../dlfcn/dlfcn.h"
+#include <stdlib.h>
+
+/* unique5lib.so library contains both STB_GNU_UNIQUE and undefined symbols.
+   First dlopen call fails because of undefined symbols.  Test checks that
+   library does not remain locked in the memory in this case. [BZ #17833]  */
+
+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..c292a23
--- /dev/null
+++ b/elf/tst-unique5lib.cc
@@ -0,0 +1,14 @@
+/* tst-unique5lib.so library for tst-unique5.cc.  */
+
+extern int not_exist (void);
+
+inline int make_unique (void)
+{
+  static int unique;
+  return ++unique;
+}
+
+int foo (void)
+{
+  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

[PATCHv3][PING^5][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[Pavel Kopyl]

[-- Attachment #1: Type: text/plain, Size: 840 bytes --]



On 03/03/2015 12:13 PM, Pavel Kopyl wrote:
>
> On 03/01/2015 10:17 PM, Mike Frysinger wrote:
>> On 27 Feb 2015 16:32, Pavel Kopyl wrote:
>>> --- /dev/null
>>> +++ b/elf/tst-unique5lib.cc
>>> @@ -0,0 +1,13 @@
>>> +
>> i know existing tests are bad examples, but lets try and start fixing 
>> that.
>> namely, there should be a header here giving a quick overview of what 
>> it is
>> exactly you're testing for, and a BZ reference.
>>
>>> +extern int not_exist ();
>>> +
>>> +inline int make_unique ()
>>> +{
>>> +  static int unique;
>>> +  return ++unique;
>>> +}
>>> +
>>> +int foo ()
>>> +{
>>> +  return make_unique () + not_exist ();
>>> +}
>> i don't know if this is just copy & pasting, but prototypes that do 
>> not intend
>> to take args should always be (void).
>> -mike
>
> Thanks for review, I fixed that in patch v3.
Ping.


[-- Attachment #2: delete_inconsistent_objs_v3.patch --]
[-- Type: text/x-patch, Size: 6054 bytes --]

commit 0d8ff75e18fad28123dca0df29f23942299dac56
Author: Pavel Kopyl <p.kopyl@samsung.com>
Date:   Mon Mar 2 23:05:48 2015 +0300

    2015-03-02  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 711beed..f3aaf4d 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 \
@@ -580,6 +580,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.
@@ -1138,6 +1139,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..628897a
--- /dev/null
+++ b/elf/tst-unique5.cc
@@ -0,0 +1,15 @@
+#include "../dlfcn/dlfcn.h"
+#include <stdlib.h>
+
+/* unique5lib.so library contains both STB_GNU_UNIQUE and undefined symbols.
+   First dlopen call fails because of undefined symbols.  Test checks that
+   library does not remain locked in the memory in this case. [BZ #17833]  */
+
+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..c292a23
--- /dev/null
+++ b/elf/tst-unique5lib.cc
@@ -0,0 +1,14 @@
+/* tst-unique5lib.so library for tst-unique5.cc.  */
+
+extern int not_exist (void);
+
+inline int make_unique (void)
+{
+  static int unique;
+  return ++unique;
+}
+
+int foo (void)
+{
+  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

Re: [PATCHv3][PING^5][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[H.J. Lu]

On Fri, Apr 24, 2015 at 3:33 AM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>
>
> On 03/03/2015 12:13 PM, Pavel Kopyl wrote:
>>
>>
>> On 03/01/2015 10:17 PM, Mike Frysinger wrote:
>>>
>>> On 27 Feb 2015 16:32, Pavel Kopyl wrote:
>>>>
>>>> --- /dev/null
>>>> +++ b/elf/tst-unique5lib.cc
>>>> @@ -0,0 +1,13 @@
>>>> +
>>>
>>> i know existing tests are bad examples, but lets try and start fixing
>>> that.
>>> namely, there should be a header here giving a quick overview of what it
>>> is
>>> exactly you're testing for, and a BZ reference.
>>>
>>>> +extern int not_exist ();
>>>> +
>>>> +inline int make_unique ()
>>>> +{
>>>> +  static int unique;
>>>> +  return ++unique;
>>>> +}
>>>> +
>>>> +int foo ()
>>>> +{
>>>> +  return make_unique () + not_exist ();
>>>> +}
>>>
>>> i don't know if this is just copy & pasting, but prototypes that do not
>>> intend
>>> to take args should always be (void).
>>> -mike
>>
>>
>> Thanks for review, I fixed that in patch v3.
>
> Ping.
>

Some comments:

1. The bug report is against STB_GNU_UNIQUE.  But I don't see STB_GNU_UNIQUE in
testcase.  I can't tell if the original STB_GNU_UNIQUE bug is fixed.
2. Your testcase ignores dlopen error.  Why should it work at all?
3. Your testcase doesn't use test-skeleton.c.

-- 
H.J.

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

[Pavel Kopyl]

[-- Attachment #1: Type: text/plain, Size: 1911 bytes --]



On 04/24/2015 02:47 PM, H.J. Lu wrote:
> On Fri, Apr 24, 2015 at 3:33 AM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>>
>> On 03/03/2015 12:13 PM, Pavel Kopyl wrote:
>>>
>>> On 03/01/2015 10:17 PM, Mike Frysinger wrote:
>>>> On 27 Feb 2015 16:32, Pavel Kopyl wrote:
>>>>> --- /dev/null
>>>>> +++ b/elf/tst-unique5lib.cc
>>>>> @@ -0,0 +1,13 @@
>>>>> +
>>>> i know existing tests are bad examples, but lets try and start fixing
>>>> that.
>>>> namely, there should be a header here giving a quick overview of what it
>>>> is
>>>> exactly you're testing for, and a BZ reference.
>>>>
>>>>> +extern int not_exist ();
>>>>> +
>>>>> +inline int make_unique ()
>>>>> +{
>>>>> +  static int unique;
>>>>> +  return ++unique;
>>>>> +}
>>>>> +
>>>>> +int foo ()
>>>>> +{
>>>>> +  return make_unique () + not_exist ();
>>>>> +}
>>>> i don't know if this is just copy & pasting, but prototypes that do not
>>>> intend
>>>> to take args should always be (void).
>>>> -mike
>>>
>>> Thanks for review, I fixed that in patch v3.
>> Ping.
>>
> Some comments:
>
> 1. The bug report is against STB_GNU_UNIQUE.  But I don't see STB_GNU_UNIQUE in
> testcase.  I can't tell if the original STB_GNU_UNIQUE bug is fixed.
> 2. Your testcase ignores dlopen error.  Why should it work at all?
> 3. Your testcase doesn't use test-skeleton.c.
>

Thanks for review!

1. I added some comments in testcase to be more clear.

2. Actually the call below is expected to return NULL because of 
undefined symbol in the library.

dlopen ("$ORIGIN/tst-unique5lib.so", RTLD_NOW);

It's a goal of this testcase: I need some library with STB_GNU_UNIQUE 
symbols that returns error when trying to load it.
In new patch version I especially point out to this fact by checking 
return value.

3. It seems for me that template of the test-skeleton.c is not 
appropriate because this testcase has two related binary -  library and 
executable.


-Pavel





[-- Attachment #2: delete_inconsistent_objs_v4.patch --]
[-- Type: text/x-patch, Size: 6494 bytes --]

commit 9a5dc6c0988d9cd4329328de810097ff2b9368d3
Author: Pavel Kopyl <p.kopyl@samsung.com>
Date:   Wed May 13 14:31:15 2015 +0300

    2015-05-13  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 34450ea..d0dd0a2 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 $(if $(CXX),tst-unique3 tst-unique4) \
+	 tst-unique1 tst-unique2 $(if $(CXX),tst-unique3 tst-unique4 tst-unique5) \
 	 tst-initorder tst-initorder2 tst-relsort1 tst-null-argv \
 	 tst-ptrguard1 tst-tlsalign
 #	 reldep9
@@ -205,7 +205,7 @@ 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-unique5lib) \
 		tst-initordera1 tst-initorderb1 \
 		tst-initordera2 tst-initorderb2 \
 		tst-initordera3 tst-initordera4 \
@@ -592,6 +592,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.
@@ -1150,6 +1151,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 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-unique5.cc b/elf/tst-unique5.cc
new file mode 100644
index 0000000..a18fb42
--- /dev/null
+++ b/elf/tst-unique5.cc
@@ -0,0 +1,21 @@
+#include "../dlfcn/dlfcn.h"
+#include <stdlib.h>
+
+/* unique5lib.so library contains both STB_GNU_UNIQUE and undefined symbols.
+   First dlopen call fails because of undefined symbols.  Test checks that
+   library does not remain locked in the memory in this case. [BZ #17833]  */
+
+int
+main (void)
+{
+  /* This call should return error because of undefined symbol in library.  */
+  if (dlopen ("$ORIGIN/tst-unique5lib.so", RTLD_NOW) != NULL)
+    return 1;
+
+  /* Check that library is not present in the memory after unsuccessful
+     attempt to load it.  */
+  if (dlopen ("$ORIGIN/tst-unique5lib.so", RTLD_LAZY | RTLD_NOLOAD) != NULL)
+    return 1;
+
+  return 0;
+}
diff --git a/elf/tst-unique5lib.cc b/elf/tst-unique5lib.cc
new file mode 100644
index 0000000..b45ead8
--- /dev/null
+++ b/elf/tst-unique5lib.cc
@@ -0,0 +1,16 @@
+/* tst-unique5lib.so library for tst-unique5.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 ();
+}
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

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

[Pavel Kopyl]

[-- Attachment #1: Type: text/plain, Size: 2061 bytes --]



On 05/13/2015 02:40 PM, Pavel Kopyl wrote:
>
>
> On 04/24/2015 02:47 PM, H.J. Lu wrote:
>> On Fri, Apr 24, 2015 at 3:33 AM, Pavel Kopyl <p.kopyl@samsung.com> 
>> wrote:
>>>
>>> On 03/03/2015 12:13 PM, Pavel Kopyl wrote:
>>>>
>>>> On 03/01/2015 10:17 PM, Mike Frysinger wrote:
>>>>> On 27 Feb 2015 16:32, Pavel Kopyl wrote:
>>>>>> --- /dev/null
>>>>>> +++ b/elf/tst-unique5lib.cc
>>>>>> @@ -0,0 +1,13 @@
>>>>>> +
>>>>> i know existing tests are bad examples, but lets try and start fixing
>>>>> that.
>>>>> namely, there should be a header here giving a quick overview of 
>>>>> what it
>>>>> is
>>>>> exactly you're testing for, and a BZ reference.
>>>>>
>>>>>> +extern int not_exist ();
>>>>>> +
>>>>>> +inline int make_unique ()
>>>>>> +{
>>>>>> +  static int unique;
>>>>>> +  return ++unique;
>>>>>> +}
>>>>>> +
>>>>>> +int foo ()
>>>>>> +{
>>>>>> +  return make_unique () + not_exist ();
>>>>>> +}
>>>>> i don't know if this is just copy & pasting, but prototypes that 
>>>>> do not
>>>>> intend
>>>>> to take args should always be (void).
>>>>> -mike
>>>>
>>>> Thanks for review, I fixed that in patch v3.
>>> Ping.
>>>
>> Some comments:
>>
>> 1. The bug report is against STB_GNU_UNIQUE.  But I don't see 
>> STB_GNU_UNIQUE in
>> testcase.  I can't tell if the original STB_GNU_UNIQUE bug is fixed.
>> 2. Your testcase ignores dlopen error.  Why should it work at all?
>> 3. Your testcase doesn't use test-skeleton.c.
>>
>
> Thanks for review!
>
> 1. I added some comments in testcase to be more clear.
>
> 2. Actually the call below is expected to return NULL because of 
> undefined symbol in the library.
>
> dlopen ("$ORIGIN/tst-unique5lib.so", RTLD_NOW);
>
> It's a goal of this testcase: I need some library with STB_GNU_UNIQUE 
> symbols that returns error when trying to load it.
> In new patch version I especially point out to this fact by checking 
> return value.
>
> 3. It seems for me that template of the test-skeleton.c is not 
> appropriate because this testcase has two related binary - library and 
> executable.
>
>
> -Pavel
Ping.


[-- Attachment #2: delete_inconsistent_objs_v4.patch --]
[-- Type: text/x-patch, Size: 6494 bytes --]

commit 9a5dc6c0988d9cd4329328de810097ff2b9368d3
Author: Pavel Kopyl <p.kopyl@samsung.com>
Date:   Wed May 13 14:31:15 2015 +0300

    2015-05-13  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 34450ea..d0dd0a2 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 $(if $(CXX),tst-unique3 tst-unique4) \
+	 tst-unique1 tst-unique2 $(if $(CXX),tst-unique3 tst-unique4 tst-unique5) \
 	 tst-initorder tst-initorder2 tst-relsort1 tst-null-argv \
 	 tst-ptrguard1 tst-tlsalign
 #	 reldep9
@@ -205,7 +205,7 @@ 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-unique5lib) \
 		tst-initordera1 tst-initorderb1 \
 		tst-initordera2 tst-initorderb2 \
 		tst-initordera3 tst-initordera4 \
@@ -592,6 +592,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.
@@ -1150,6 +1151,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 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-unique5.cc b/elf/tst-unique5.cc
new file mode 100644
index 0000000..a18fb42
--- /dev/null
+++ b/elf/tst-unique5.cc
@@ -0,0 +1,21 @@
+#include "../dlfcn/dlfcn.h"
+#include <stdlib.h>
+
+/* unique5lib.so library contains both STB_GNU_UNIQUE and undefined symbols.
+   First dlopen call fails because of undefined symbols.  Test checks that
+   library does not remain locked in the memory in this case. [BZ #17833]  */
+
+int
+main (void)
+{
+  /* This call should return error because of undefined symbol in library.  */
+  if (dlopen ("$ORIGIN/tst-unique5lib.so", RTLD_NOW) != NULL)
+    return 1;
+
+  /* Check that library is not present in the memory after unsuccessful
+     attempt to load it.  */
+  if (dlopen ("$ORIGIN/tst-unique5lib.so", RTLD_LAZY | RTLD_NOLOAD) != NULL)
+    return 1;
+
+  return 0;
+}
diff --git a/elf/tst-unique5lib.cc b/elf/tst-unique5lib.cc
new file mode 100644
index 0000000..b45ead8
--- /dev/null
+++ b/elf/tst-unique5lib.cc
@@ -0,0 +1,16 @@
+/* tst-unique5lib.so library for tst-unique5.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 ();
+}
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

Re: [PATCHv4][PING][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[H.J. Lu]

On Wed, May 27, 2015 at 5:17 AM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>
>
> On 05/13/2015 02:40 PM, Pavel Kopyl wrote:
>>
>>
>>
>> On 04/24/2015 02:47 PM, H.J. Lu wrote:
>>>
>>> On Fri, Apr 24, 2015 at 3:33 AM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>>>>
>>>>
>>>> On 03/03/2015 12:13 PM, Pavel Kopyl wrote:
>>>>>
>>>>>
>>>>> On 03/01/2015 10:17 PM, Mike Frysinger wrote:
>>>>>>
>>>>>> On 27 Feb 2015 16:32, Pavel Kopyl wrote:
>>>>>>>
>>>>>>> --- /dev/null
>>>>>>> +++ b/elf/tst-unique5lib.cc
>>>>>>> @@ -0,0 +1,13 @@
>>>>>>> +
>>>>>>
>>>>>> i know existing tests are bad examples, but lets try and start fixing
>>>>>> that.
>>>>>> namely, there should be a header here giving a quick overview of what
>>>>>> it
>>>>>> is
>>>>>> exactly you're testing for, and a BZ reference.
>>>>>>
>>>>>>> +extern int not_exist ();
>>>>>>> +
>>>>>>> +inline int make_unique ()
>>>>>>> +{
>>>>>>> +  static int unique;
>>>>>>> +  return ++unique;
>>>>>>> +}
>>>>>>> +
>>>>>>> +int foo ()
>>>>>>> +{
>>>>>>> +  return make_unique () + not_exist ();
>>>>>>> +}
>>>>>>
>>>>>> i don't know if this is just copy & pasting, but prototypes that do
>>>>>> not
>>>>>> intend
>>>>>> to take args should always be (void).
>>>>>> -mike
>>>>>
>>>>>
>>>>> Thanks for review, I fixed that in patch v3.
>>>>
>>>> Ping.
>>>>
>>> Some comments:
>>>
>>> 1. The bug report is against STB_GNU_UNIQUE.  But I don't see
>>> STB_GNU_UNIQUE in
>>> testcase.  I can't tell if the original STB_GNU_UNIQUE bug is fixed.
>>> 2. Your testcase ignores dlopen error.  Why should it work at all?
>>> 3. Your testcase doesn't use test-skeleton.c.
>>>
>>
>> Thanks for review!
>>
>> 1. I added some comments in testcase to be more clear.
>>
>> 2. Actually the call below is expected to return NULL because of undefined
>> symbol in the library.
>>
>> dlopen ("$ORIGIN/tst-unique5lib.so", RTLD_NOW);
>>
>> It's a goal of this testcase: I need some library with STB_GNU_UNIQUE
>> symbols that returns error when trying to load it.
>> In new patch version I especially point out to this fact by checking
>> return value.
>>
>> 3. It seems for me that template of the test-skeleton.c is not appropriate
>> because this testcase has two related binary - library and executable.
>>
>>
>> -Pavel
>
> Ping.
>

Since you changed DF_1_NODELETE, how did you test it?  Does your test
fail without the DF_1_NODELETE change?

-- 
H.J.

Re: [PATCHv4][PING][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[Yury Gribov]

On 05/27/2015 04:04 PM, H.J. Lu wrote:
> On Wed, May 27, 2015 at 5:17 AM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>>
>>
>> On 05/13/2015 02:40 PM, Pavel Kopyl wrote:
>>>
>>>
>>>
>>> On 04/24/2015 02:47 PM, H.J. Lu wrote:
>>>>
>>>> On Fri, Apr 24, 2015 at 3:33 AM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>>>>>
>>>>>
>>>>> On 03/03/2015 12:13 PM, Pavel Kopyl wrote:
>>>>>>
>>>>>>
>>>>>> On 03/01/2015 10:17 PM, Mike Frysinger wrote:
>>>>>>>
>>>>>>> On 27 Feb 2015 16:32, Pavel Kopyl wrote:
>>>>>>>>
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/elf/tst-unique5lib.cc
>>>>>>>> @@ -0,0 +1,13 @@
>>>>>>>> +
>>>>>>>
>>>>>>> i know existing tests are bad examples, but lets try and start fixing
>>>>>>> that.
>>>>>>> namely, there should be a header here giving a quick overview of what
>>>>>>> it
>>>>>>> is
>>>>>>> exactly you're testing for, and a BZ reference.
>>>>>>>
>>>>>>>> +extern int not_exist ();
>>>>>>>> +
>>>>>>>> +inline int make_unique ()
>>>>>>>> +{
>>>>>>>> +  static int unique;
>>>>>>>> +  return ++unique;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +int foo ()
>>>>>>>> +{
>>>>>>>> +  return make_unique () + not_exist ();
>>>>>>>> +}
>>>>>>>
>>>>>>> i don't know if this is just copy & pasting, but prototypes that do
>>>>>>> not
>>>>>>> intend
>>>>>>> to take args should always be (void).
>>>>>>> -mike
>>>>>>
>>>>>>
>>>>>> Thanks for review, I fixed that in patch v3.
>>>>>
>>>>> Ping.
>>>>>
>>>> Some comments:
>>>>
>>>> 1. The bug report is against STB_GNU_UNIQUE.  But I don't see
>>>> STB_GNU_UNIQUE in
>>>> testcase.  I can't tell if the original STB_GNU_UNIQUE bug is fixed.
>>>> 2. Your testcase ignores dlopen error.  Why should it work at all?
>>>> 3. Your testcase doesn't use test-skeleton.c.
>>>>
>>>
>>> Thanks for review!
>>>
>>> 1. I added some comments in testcase to be more clear.
>>>
>>> 2. Actually the call below is expected to return NULL because of undefined
>>> symbol in the library.
>>>
>>> dlopen ("$ORIGIN/tst-unique5lib.so", RTLD_NOW);
>>>
>>> It's a goal of this testcase: I need some library with STB_GNU_UNIQUE
>>> symbols that returns error when trying to load it.
>>> In new patch version I especially point out to this fact by checking
>>> return value.
>>>
>>> 3. It seems for me that template of the test-skeleton.c is not appropriate
>>> because this testcase has two related binary - library and executable.
>>>
>>>
>>> -Pavel
>>
>> Ping.
>>
>
> Since you changed DF_1_NODELETE, how did you test it?  Does your test
> fail without the DF_1_NODELETE change?

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.

-Y

Re: [PATCHv4][PING][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[H.J. Lu]

On Wed, May 27, 2015 at 6:12 AM, Yury Gribov <y.gribov@samsung.com> wrote:
> On 05/27/2015 04:04 PM, H.J. Lu wrote:
>>
>> On Wed, May 27, 2015 at 5:17 AM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>>>
>>>
>>>
>>> On 05/13/2015 02:40 PM, Pavel Kopyl wrote:
>>>>
>>>>
>>>>
>>>>
>>>> On 04/24/2015 02:47 PM, H.J. Lu wrote:
>>>>>
>>>>>
>>>>> On Fri, Apr 24, 2015 at 3:33 AM, Pavel Kopyl <p.kopyl@samsung.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 03/03/2015 12:13 PM, Pavel Kopyl wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 03/01/2015 10:17 PM, Mike Frysinger wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 27 Feb 2015 16:32, Pavel Kopyl wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/elf/tst-unique5lib.cc
>>>>>>>>> @@ -0,0 +1,13 @@
>>>>>>>>> +
>>>>>>>>
>>>>>>>>
>>>>>>>> i know existing tests are bad examples, but lets try and start
>>>>>>>> fixing
>>>>>>>> that.
>>>>>>>> namely, there should be a header here giving a quick overview of
>>>>>>>> what
>>>>>>>> it
>>>>>>>> is
>>>>>>>> exactly you're testing for, and a BZ reference.
>>>>>>>>
>>>>>>>>> +extern int not_exist ();
>>>>>>>>> +
>>>>>>>>> +inline int make_unique ()
>>>>>>>>> +{
>>>>>>>>> +  static int unique;
>>>>>>>>> +  return ++unique;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +int foo ()
>>>>>>>>> +{
>>>>>>>>> +  return make_unique () + not_exist ();
>>>>>>>>> +}
>>>>>>>>
>>>>>>>>
>>>>>>>> i don't know if this is just copy & pasting, but prototypes that do
>>>>>>>> not
>>>>>>>> intend
>>>>>>>> to take args should always be (void).
>>>>>>>> -mike
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Thanks for review, I fixed that in patch v3.
>>>>>>
>>>>>>
>>>>>> Ping.
>>>>>>
>>>>> Some comments:
>>>>>
>>>>> 1. The bug report is against STB_GNU_UNIQUE.  But I don't see
>>>>> STB_GNU_UNIQUE in
>>>>> testcase.  I can't tell if the original STB_GNU_UNIQUE bug is fixed.
>>>>> 2. Your testcase ignores dlopen error.  Why should it work at all?
>>>>> 3. Your testcase doesn't use test-skeleton.c.
>>>>>
>>>>
>>>> Thanks for review!
>>>>
>>>> 1. I added some comments in testcase to be more clear.
>>>>
>>>> 2. Actually the call below is expected to return NULL because of
>>>> undefined
>>>> symbol in the library.
>>>>
>>>> dlopen ("$ORIGIN/tst-unique5lib.so", RTLD_NOW);
>>>>
>>>> It's a goal of this testcase: I need some library with STB_GNU_UNIQUE
>>>> symbols that returns error when trying to load it.
>>>> In new patch version I especially point out to this fact by checking
>>>> return value.
>>>>
>>>> 3. It seems for me that template of the test-skeleton.c is not
>>>> appropriate
>>>> because this testcase has two related binary - library and executable.
>>>>
>>>>
>>>> -Pavel
>>>
>>>
>>> Ping.
>>>
>>
>> Since you changed DF_1_NODELETE, how did you test it?  Does your test
>> fail without the DF_1_NODELETE change?
>
>
> 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:

[hjl@gnu-tools-1 elf]$ readelf -sW tst-unique1mod1.so | grep UNIQUE
     9: 0000000000201020     4 OBJECT  UNIQUE DEFAULT   22 var
    53: 0000000000201020     4 OBJECT  UNIQUE DEFAULT   22 var
[hjl@gnu-tools-1 elf]$ readelf -d tst-unique1mod1.so

Dynamic section at offset 0xe40 contains 21 entries:
  Tag        Type                         Name/Value
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
 0x000000000000000c (INIT)               0x560
 0x000000000000000d (FINI)               0x6a0
 0x0000000000000019 (INIT_ARRAY)         0x200e28
 0x000000000000001b (INIT_ARRAYSZ)       8 (bytes)
 0x000000000000001a (FINI_ARRAY)         0x200e30
 0x000000000000001c (FINI_ARRAYSZ)       8 (bytes)
 0x0000000000000004 (HASH)               0x6f8
 0x000000006ffffef5 (GNU_HASH)           0x210
 0x0000000000000005 (STRTAB)             0x3a0
 0x0000000000000006 (SYMTAB)             0x250
 0x000000000000000a (STRSZ)              169 (bytes)
 0x000000000000000b (SYMENT)             24 (bytes)
 0x0000000000000007 (RELA)               0x488
 0x0000000000000008 (RELASZ)             216 (bytes)
 0x0000000000000009 (RELAENT)            24 (bytes)
 0x000000006ffffffe (VERNEED)            0x468
 0x000000006fffffff (VERNEEDNUM)         1
 0x000000006ffffff0 (VERSYM)             0x44a
 0x000000006ffffff9 (RELACOUNT)          3
 0x0000000000000000 (NULL)               0x0
[hjl@gnu-tools-1 elf]$

Does your test have the DF_1_NODELETE bit set?


-- 
H.J.

Re: [PATCHv4][PING][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[Yury Gribov]

On 05/27/2015 04:23 PM, H.J. Lu wrote:
> On Wed, May 27, 2015 at 6:12 AM, Yury Gribov <y.gribov@samsung.com> wrote:
>> On 05/27/2015 04:04 PM, H.J. Lu wrote:
>>>
>>> On Wed, May 27, 2015 at 5:17 AM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>>>>
>>>>
>>>>
>>>> On 05/13/2015 02:40 PM, Pavel Kopyl wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 04/24/2015 02:47 PM, H.J. Lu wrote:
>>>>>>
>>>>>>
>>>>>> On Fri, Apr 24, 2015 at 3:33 AM, Pavel Kopyl <p.kopyl@samsung.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 03/03/2015 12:13 PM, Pavel Kopyl wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 03/01/2015 10:17 PM, Mike Frysinger wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 27 Feb 2015 16:32, Pavel Kopyl wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/elf/tst-unique5lib.cc
>>>>>>>>>> @@ -0,0 +1,13 @@
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> i know existing tests are bad examples, but lets try and start
>>>>>>>>> fixing
>>>>>>>>> that.
>>>>>>>>> namely, there should be a header here giving a quick overview of
>>>>>>>>> what
>>>>>>>>> it
>>>>>>>>> is
>>>>>>>>> exactly you're testing for, and a BZ reference.
>>>>>>>>>
>>>>>>>>>> +extern int not_exist ();
>>>>>>>>>> +
>>>>>>>>>> +inline int make_unique ()
>>>>>>>>>> +{
>>>>>>>>>> +  static int unique;
>>>>>>>>>> +  return ++unique;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +int foo ()
>>>>>>>>>> +{
>>>>>>>>>> +  return make_unique () + not_exist ();
>>>>>>>>>> +}
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> i don't know if this is just copy & pasting, but prototypes that do
>>>>>>>>> not
>>>>>>>>> intend
>>>>>>>>> to take args should always be (void).
>>>>>>>>> -mike
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks for review, I fixed that in patch v3.
>>>>>>>
>>>>>>>
>>>>>>> Ping.
>>>>>>>
>>>>>> Some comments:
>>>>>>
>>>>>> 1. The bug report is against STB_GNU_UNIQUE.  But I don't see
>>>>>> STB_GNU_UNIQUE in
>>>>>> testcase.  I can't tell if the original STB_GNU_UNIQUE bug is fixed.
>>>>>> 2. Your testcase ignores dlopen error.  Why should it work at all?
>>>>>> 3. Your testcase doesn't use test-skeleton.c.
>>>>>>
>>>>>
>>>>> Thanks for review!
>>>>>
>>>>> 1. I added some comments in testcase to be more clear.
>>>>>
>>>>> 2. Actually the call below is expected to return NULL because of
>>>>> undefined
>>>>> symbol in the library.
>>>>>
>>>>> dlopen ("$ORIGIN/tst-unique5lib.so", RTLD_NOW);
>>>>>
>>>>> It's a goal of this testcase: I need some library with STB_GNU_UNIQUE
>>>>> symbols that returns error when trying to load it.
>>>>> In new patch version I especially point out to this fact by checking
>>>>> return value.
>>>>>
>>>>> 3. It seems for me that template of the test-skeleton.c is not
>>>>> appropriate
>>>>> because this testcase has two related binary - library and executable.
>>>>>
>>>>>
>>>>> -Pavel
>>>>
>>>>
>>>> Ping.
>>>>
>>>
>>> Since you changed DF_1_NODELETE, how did you test it?  Does your test
>>> fail without the DF_1_NODELETE change?
>>
>>
>> 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

Re: [PATCHv4][PING][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[H.J. Lu]

On Wed, May 27, 2015 at 6:36 AM, Yury Gribov <y.gribov@samsung.com> wrote:
> On 05/27/2015 04:23 PM, H.J. Lu wrote:
>>
>> On Wed, May 27, 2015 at 6:12 AM, Yury Gribov <y.gribov@samsung.com> wrote:
>>>
>>> On 05/27/2015 04:04 PM, H.J. Lu wrote:
>>>>
>>>>
>>>> On Wed, May 27, 2015 at 5:17 AM, Pavel Kopyl <p.kopyl@samsung.com>
>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 05/13/2015 02:40 PM, Pavel Kopyl wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 04/24/2015 02:47 PM, H.J. Lu wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Fri, Apr 24, 2015 at 3:33 AM, Pavel Kopyl <p.kopyl@samsung.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 03/03/2015 12:13 PM, Pavel Kopyl wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 03/01/2015 10:17 PM, Mike Frysinger wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 27 Feb 2015 16:32, Pavel Kopyl wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> --- /dev/null
>>>>>>>>>>> +++ b/elf/tst-unique5lib.cc
>>>>>>>>>>> @@ -0,0 +1,13 @@
>>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> i know existing tests are bad examples, but lets try and start
>>>>>>>>>> fixing
>>>>>>>>>> that.
>>>>>>>>>> namely, there should be a header here giving a quick overview of
>>>>>>>>>> what
>>>>>>>>>> it
>>>>>>>>>> is
>>>>>>>>>> exactly you're testing for, and a BZ reference.
>>>>>>>>>>
>>>>>>>>>>> +extern int not_exist ();
>>>>>>>>>>> +
>>>>>>>>>>> +inline int make_unique ()
>>>>>>>>>>> +{
>>>>>>>>>>> +  static int unique;
>>>>>>>>>>> +  return ++unique;
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +int foo ()
>>>>>>>>>>> +{
>>>>>>>>>>> +  return make_unique () + not_exist ();
>>>>>>>>>>> +}
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> i don't know if this is just copy & pasting, but prototypes that
>>>>>>>>>> do
>>>>>>>>>> not
>>>>>>>>>> intend
>>>>>>>>>> to take args should always be (void).
>>>>>>>>>> -mike
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks for review, I fixed that in patch v3.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Ping.
>>>>>>>>
>>>>>>> Some comments:
>>>>>>>
>>>>>>> 1. The bug report is against STB_GNU_UNIQUE.  But I don't see
>>>>>>> STB_GNU_UNIQUE in
>>>>>>> testcase.  I can't tell if the original STB_GNU_UNIQUE bug is fixed.
>>>>>>> 2. Your testcase ignores dlopen error.  Why should it work at all?
>>>>>>> 3. Your testcase doesn't use test-skeleton.c.
>>>>>>>
>>>>>>
>>>>>> Thanks for review!
>>>>>>
>>>>>> 1. I added some comments in testcase to be more clear.
>>>>>>
>>>>>> 2. Actually the call below is expected to return NULL because of
>>>>>> undefined
>>>>>> symbol in the library.
>>>>>>
>>>>>> dlopen ("$ORIGIN/tst-unique5lib.so", RTLD_NOW);
>>>>>>
>>>>>> It's a goal of this testcase: I need some library with STB_GNU_UNIQUE
>>>>>> symbols that returns error when trying to load it.
>>>>>> In new patch version I especially point out to this fact by checking
>>>>>> return value.
>>>>>>
>>>>>> 3. It seems for me that template of the test-skeleton.c is not
>>>>>> appropriate
>>>>>> because this testcase has two related binary - library and executable.
>>>>>>
>>>>>>
>>>>>> -Pavel
>>>>>
>>>>>
>>>>>
>>>>> Ping.
>>>>>
>>>>
>>>> Since you changed DF_1_NODELETE, how did you test it?  Does your test
>>>> fail without the DF_1_NODELETE change?
>>>
>>>
>>>
>>> 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?

-- 
H.J.

Re: [PATCHv4][PING][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[Pavel Kopyl]

On 05/27/2015 04:45 PM, H.J. Lu wrote:
> On Wed, May 27, 2015 at 6:36 AM, Yury Gribov <y.gribov@samsung.com> wrote:
>> On 05/27/2015 04:23 PM, H.J. Lu wrote:
>>> On Wed, May 27, 2015 at 6:12 AM, Yury Gribov <y.gribov@samsung.com> wrote:
>>>> On 05/27/2015 04:04 PM, H.J. Lu wrote:
>>>>>
>>>>> On Wed, May 27, 2015 at 5:17 AM, Pavel Kopyl <p.kopyl@samsung.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 05/13/2015 02:40 PM, Pavel Kopyl wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 04/24/2015 02:47 PM, H.J. Lu wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On Fri, Apr 24, 2015 at 3:33 AM, Pavel Kopyl <p.kopyl@samsung.com>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 03/03/2015 12:13 PM, Pavel Kopyl wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 03/01/2015 10:17 PM, Mike Frysinger wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 27 Feb 2015 16:32, Pavel Kopyl wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> --- /dev/null
>>>>>>>>>>>> +++ b/elf/tst-unique5lib.cc
>>>>>>>>>>>> @@ -0,0 +1,13 @@
>>>>>>>>>>>> +
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> i know existing tests are bad examples, but lets try and start
>>>>>>>>>>> fixing
>>>>>>>>>>> that.
>>>>>>>>>>> namely, there should be a header here giving a quick overview of
>>>>>>>>>>> what
>>>>>>>>>>> it
>>>>>>>>>>> is
>>>>>>>>>>> exactly you're testing for, and a BZ reference.
>>>>>>>>>>>
>>>>>>>>>>>> +extern int not_exist ();
>>>>>>>>>>>> +
>>>>>>>>>>>> +inline int make_unique ()
>>>>>>>>>>>> +{
>>>>>>>>>>>> +  static int unique;
>>>>>>>>>>>> +  return ++unique;
>>>>>>>>>>>> +}
>>>>>>>>>>>> +
>>>>>>>>>>>> +int foo ()
>>>>>>>>>>>> +{
>>>>>>>>>>>> +  return make_unique () + not_exist ();
>>>>>>>>>>>> +}
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> i don't know if this is just copy & pasting, but prototypes that
>>>>>>>>>>> do
>>>>>>>>>>> not
>>>>>>>>>>> intend
>>>>>>>>>>> to take args should always be (void).
>>>>>>>>>>> -mike
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks for review, I fixed that in patch v3.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Ping.
>>>>>>>>>
>>>>>>>> Some comments:
>>>>>>>>
>>>>>>>> 1. The bug report is against STB_GNU_UNIQUE.  But I don't see
>>>>>>>> STB_GNU_UNIQUE in
>>>>>>>> testcase.  I can't tell if the original STB_GNU_UNIQUE bug is fixed.
>>>>>>>> 2. Your testcase ignores dlopen error.  Why should it work at all?
>>>>>>>> 3. Your testcase doesn't use test-skeleton.c.
>>>>>>>>
>>>>>>> Thanks for review!
>>>>>>>
>>>>>>> 1. I added some comments in testcase to be more clear.
>>>>>>>
>>>>>>> 2. Actually the call below is expected to return NULL because of
>>>>>>> undefined
>>>>>>> symbol in the library.
>>>>>>>
>>>>>>> dlopen ("$ORIGIN/tst-unique5lib.so", RTLD_NOW);
>>>>>>>
>>>>>>> It's a goal of this testcase: I need some library with STB_GNU_UNIQUE
>>>>>>> symbols that returns error when trying to load it.
>>>>>>> In new patch version I especially point out to this fact by checking
>>>>>>> return value.
>>>>>>>
>>>>>>> 3. It seems for me that template of the test-skeleton.c is not
>>>>>>> appropriate
>>>>>>> because this testcase has two related binary - library and executable.
>>>>>>>
>>>>>>>
>>>>>>> -Pavel
>>>>>>
>>>>>>
>>>>>> Ping.
>>>>>>
>>>>> Since you changed DF_1_NODELETE, how did you test it?  Does your test
>>>>> fail without the DF_1_NODELETE change?
>>>>
>>>>
>>>> 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.

-Pavel

Re: [PATCHv4][PING][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[H.J. Lu]

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.


-- 
H.J.

Re: [PATCHv4][PING][BZ #17833] _dl_close_worker() does not release inconsistent objects.

[Pavel Kopyl]

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.


-Pavel

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

[Pavel Kopyl]

[-- Attachment #1: Type: text/plain, Size: 4862 bytes --]



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.
>
>
> -Pavel

This is new patch with updated testcase.

-Pavel

[-- Attachment #2: delete_inconsistent_objs_v5.patch --]
[-- Type: text/x-patch, Size: 8717 bytes --]

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

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

[Yury Gribov]

[-- Attachment #1: Type: text/plain, Size: 5036 bytes --]

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.

[-- Attachment #2: delete_inconsistent_objs_v5.patch --]
[-- Type: text/x-patch, Size: 8717 bytes --]

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

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

[Yury Gribov]

[-- Attachment #1: Type: text/plain, Size: 5254 bytes --]

On 06/05/2015 09:44 AM, Yury Gribov wrote:
> 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.


[-- Attachment #2: delete_inconsistent_objs_v5.patch --]
[-- Type: text/x-patch, Size: 8717 bytes --]

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

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

[Pavel Kopyl]

[-- Attachment #1: Type: text/plain, Size: 5077 bytes --]



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.

[-- Attachment #2: delete_inconsistent_objs_v5.patch --]
[-- Type: text/x-patch, Size: 8717 bytes --]

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

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

[H.J. Lu]

On Tue, Jun 30, 2015 at 7:45 AM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>

This patch changes DF_1_NODELETE path.  Do we have a testcase for
sucessfully loading/unloading DF_1_NODELETE DSO with undefined
symbols?

It may have been asked before.  Can we reset unique symbols
in _dl_open before calling _dl_close_worker?

-- 
H.J.

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

[Pavel Kopyl]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]



On 06/30/2015 06:12 PM, H.J. Lu wrote:
> On Tue, Jun 30, 2015 at 7:45 AM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
> This patch changes DF_1_NODELETE path.  Do we have a testcase for
> sucessfully loading/unloading DF_1_NODELETE DSO with undefined
> symbols?
>
> It may have been asked before.  Can we reset unique symbols
> in _dl_open before calling _dl_close_worker?
>
Yes, I added testcases for three possible ways where we can get 
DF_1_NODELETE:
1. Unique symbols
2. Load with RTLD_NODELETE flag.
3. Link with '-z nodelete' option

 >Can we reset unique symbols in _dl_open before calling _dl_close_worker?
But I clear unique symbols exactly in 
_dl_close_worker.<https://slovari.yandex.ru/exactly/en-ru>



[-- Attachment #2: delete_inconsistent_objs_v5.patch --]
[-- Type: text/x-patch, Size: 8717 bytes --]

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

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

[H.J. Lu]

On Thu, Jul 2, 2015 at 3:53 PM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>
>
> On 06/30/2015 06:12 PM, H.J. Lu wrote:
>>
>> On Tue, Jun 30, 2015 at 7:45 AM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>> This patch changes DF_1_NODELETE path.  Do we have a testcase for
>> sucessfully loading/unloading DF_1_NODELETE DSO with undefined
>> symbols?
>>
>> It may have been asked before.  Can we reset unique symbols
>> in _dl_open before calling _dl_close_worker?
>>
> Yes, I added testcases for three possible ways where we can get
> DF_1_NODELETE:
> 1. Unique symbols
> 2. Load with RTLD_NODELETE flag.
> 3. Link with '-z nodelete' option
>
>>Can we reset unique symbols in _dl_open before calling _dl_close_worker?
> But I clear unique symbols exactly in
> _dl_close_worker.<https://slovari.yandex.ru/exactly/en-ru>
>
>

Looks good to me.

Thanks.

-- 
H.J.

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

[Pavel Kopyl]

[-- Attachment #1: Type: text/plain, Size: 1066 bytes --]



On 07/03/2015 02:46 AM, H.J. Lu wrote:
> On Thu, Jul 2, 2015 at 3:53 PM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>>
>> On 06/30/2015 06:12 PM, H.J. Lu wrote:
>>> On Tue, Jun 30, 2015 at 7:45 AM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>>> This patch changes DF_1_NODELETE path.  Do we have a testcase for
>>> sucessfully loading/unloading DF_1_NODELETE DSO with undefined
>>> symbols?
>>>
>>> It may have been asked before.  Can we reset unique symbols
>>> in _dl_open before calling _dl_close_worker?
>>>
>> Yes, I added testcases for three possible ways where we can get
>> DF_1_NODELETE:
>> 1. Unique symbols
>> 2. Load with RTLD_NODELETE flag.
>> 3. Link with '-z nodelete' option
>>
>>> Can we reset unique symbols in _dl_open before calling _dl_close_worker?
>> But I clear unique symbols exactly in
>> _dl_close_worker.<https://slovari.yandex.ru/exactly/en-ru>
>>
>>
> Looks good to me.
>
> Thanks.
>
Thanks for review.

I updated date in changelog.
If it's OK, please could you commit this patch because I don't have 
write access to glibc repo.

-Pavel

[-- Attachment #2: delete_inconsistent_objs_v6.patch --]
[-- Type: text/x-patch, Size: 8722 bytes --]

commit 440b5e328eee846bec505f5c336567f229e08a1d
Author: Pavel Kopyl <p.kopyl@samsung.com>
Date:   Fri Jul 3 22:08:13 2015 +0300

    2015-07-03  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 f21276c0..d36a3c5 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -132,7 +132,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 \
@@ -207,7 +207,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 \
@@ -591,6 +593,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.
@@ -1153,6 +1158,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

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

[Yury Gribov]

On 07/03/2015 02:46 AM, H.J. Lu wrote:
> On Thu, Jul 2, 2015 at 3:53 PM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>>
>>
>> On 06/30/2015 06:12 PM, H.J. Lu wrote:
>>>
>>> On Tue, Jun 30, 2015 at 7:45 AM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>>> This patch changes DF_1_NODELETE path.  Do we have a testcase for
>>> sucessfully loading/unloading DF_1_NODELETE DSO with undefined
>>> symbols?
>>>
>>> It may have been asked before.  Can we reset unique symbols
>>> in _dl_open before calling _dl_close_worker?
>>>
>> Yes, I added testcases for three possible ways where we can get
>> DF_1_NODELETE:
>> 1. Unique symbols
>> 2. Load with RTLD_NODELETE flag.
>> 3. Link with '-z nodelete' option
>>
>>> Can we reset unique symbols in _dl_open before calling _dl_close_worker?
>> But I clear unique symbols exactly in
>> _dl_close_worker.<https://slovari.yandex.ru/exactly/en-ru>
>>
>>
>
> Looks good to me.

Folks,

Could someone commit the patch for us?  We do not have write access to 
Glibc repo.  We'll be around to cope with bugs (if any).

Best regards,
Yury

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

[H.J. Lu]

On Tue, Jul 7, 2015 at 12:41 AM, Yury Gribov <y.gribov@samsung.com> wrote:
> On 07/03/2015 02:46 AM, H.J. Lu wrote:
>>
>> On Thu, Jul 2, 2015 at 3:53 PM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>>>
>>>
>>>
>>> On 06/30/2015 06:12 PM, H.J. Lu wrote:
>>>>
>>>>
>>>> On Tue, Jun 30, 2015 at 7:45 AM, Pavel Kopyl <p.kopyl@samsung.com>
>>>> wrote:
>>>> This patch changes DF_1_NODELETE path.  Do we have a testcase for
>>>> sucessfully loading/unloading DF_1_NODELETE DSO with undefined
>>>> symbols?
>>>>
>>>> It may have been asked before.  Can we reset unique symbols
>>>> in _dl_open before calling _dl_close_worker?
>>>>
>>> Yes, I added testcases for three possible ways where we can get
>>> DF_1_NODELETE:
>>> 1. Unique symbols
>>> 2. Load with RTLD_NODELETE flag.
>>> 3. Link with '-z nodelete' option
>>>
>>>> Can we reset unique symbols in _dl_open before calling _dl_close_worker?
>>>
>>> But I clear unique symbols exactly in
>>> _dl_close_worker.<https://slovari.yandex.ru/exactly/en-ru>
>>>
>>>
>>
>> Looks good to me.
>
>
> Folks,
>
> Could someone commit the patch for us?  We do not have write access to Glibc
> repo.  We'll be around to cope with bugs (if any).
>

Please regenerate the patch with "gcc format-patch" and resubmit it
so that I can do "git am your-patch".

Thanks.

-- 
H.J.

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

[Pavel Kopyl]

[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]



On 07/07/2015 06:30 PM, H.J. Lu wrote:
> On Tue, Jul 7, 2015 at 12:41 AM, Yury Gribov <y.gribov@samsung.com> wrote:
>> On 07/03/2015 02:46 AM, H.J. Lu wrote:
>>> On Thu, Jul 2, 2015 at 3:53 PM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>>>>
>>>>
>>>> On 06/30/2015 06:12 PM, H.J. Lu wrote:
>>>>>
>>>>> On Tue, Jun 30, 2015 at 7:45 AM, Pavel Kopyl <p.kopyl@samsung.com>
>>>>> wrote:
>>>>> This patch changes DF_1_NODELETE path.  Do we have a testcase for
>>>>> sucessfully loading/unloading DF_1_NODELETE DSO with undefined
>>>>> symbols?
>>>>>
>>>>> It may have been asked before.  Can we reset unique symbols
>>>>> in _dl_open before calling _dl_close_worker?
>>>>>
>>>> Yes, I added testcases for three possible ways where we can get
>>>> DF_1_NODELETE:
>>>> 1. Unique symbols
>>>> 2. Load with RTLD_NODELETE flag.
>>>> 3. Link with '-z nodelete' option
>>>>
>>>>> Can we reset unique symbols in _dl_open before calling _dl_close_worker?
>>>> But I clear unique symbols exactly in
>>>> _dl_close_worker.<https://slovari.yandex.ru/exactly/en-ru>
>>>>
>>>>
>>> Looks good to me.
>>
>> Folks,
>>
>> Could someone commit the patch for us?  We do not have write access to Glibc
>> repo.  We'll be around to cope with bugs (if any).
>>
> Please regenerate the patch with "gcc format-patch" and resubmit it
> so that I can do "git am your-patch".
>
> Thanks.
Thanks a lot.

-Pavel


[-- Attachment #2: 0001-2015-07-07-Pavel-Kopyl-p.kopyl-samsung.com.patch --]
[-- Type: text/x-patch, Size: 9471 bytes --]

From b6f602f16bf1e32ccc990e178f2559babad45733 Mon Sep 17 00:00:00 2001
From: Pavel Kopyl <p.kopyl@samsung.com>
Date: Tue, 7 Jul 2015 18:45:46 +0300
Subject: [PATCH] 2015-07-07  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.
---
 elf/Makefile                  |   16 +++++++++++--
 elf/dl-close.c                |   33 ++++++++++++++++++++++++--
 elf/dl-open.c                 |    2 +-
 elf/tst-nodelete-rtldmod.cc   |    6 +++++
 elf/tst-nodelete-uniquemod.cc |   14 +++++++++++
 elf/tst-nodelete-zmod.cc      |    6 +++++
 elf/tst-nodelete.cc           |   51 +++++++++++++++++++++++++++++++++++++++++
 elf/tst-znodelete-zlib.cc     |    6 +++++
 include/dlfcn.h               |    3 ++-
 9 files changed, 131 insertions(+), 6 deletions(-)
 create mode 100644 elf/tst-nodelete-rtldmod.cc
 create mode 100644 elf/tst-nodelete-uniquemod.cc
 create mode 100644 elf/tst-nodelete-zmod.cc
 create mode 100644 elf/tst-nodelete.cc
 create mode 100644 elf/tst-znodelete-zlib.cc

diff --git a/elf/Makefile b/elf/Makefile
index f21276c0..d36a3c5 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -132,7 +132,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 \
@@ -207,7 +207,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 \
@@ -591,6 +593,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.
@@ -1153,6 +1158,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
-- 
1.7.9.5

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

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1713 bytes --]

On Tue, Jul 7, 2015 at 9:27 AM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>
>
> On 07/07/2015 06:30 PM, H.J. Lu wrote:
>>
>> On Tue, Jul 7, 2015 at 12:41 AM, Yury Gribov <y.gribov@samsung.com> wrote:
>>>
>>> On 07/03/2015 02:46 AM, H.J. Lu wrote:
>>>>
>>>> On Thu, Jul 2, 2015 at 3:53 PM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 06/30/2015 06:12 PM, H.J. Lu wrote:
>>>>>>
>>>>>>
>>>>>> On Tue, Jun 30, 2015 at 7:45 AM, Pavel Kopyl <p.kopyl@samsung.com>
>>>>>> wrote:
>>>>>> This patch changes DF_1_NODELETE path.  Do we have a testcase for
>>>>>> sucessfully loading/unloading DF_1_NODELETE DSO with undefined
>>>>>> symbols?
>>>>>>
>>>>>> It may have been asked before.  Can we reset unique symbols
>>>>>> in _dl_open before calling _dl_close_worker?
>>>>>>
>>>>> Yes, I added testcases for three possible ways where we can get
>>>>> DF_1_NODELETE:
>>>>> 1. Unique symbols
>>>>> 2. Load with RTLD_NODELETE flag.
>>>>> 3. Link with '-z nodelete' option
>>>>>
>>>>>> Can we reset unique symbols in _dl_open before calling
>>>>>> _dl_close_worker?
>>>>>
>>>>> But I clear unique symbols exactly in
>>>>> _dl_close_worker.<https://slovari.yandex.ru/exactly/en-ru>
>>>>>
>>>>>
>>>> Looks good to me.
>>>
>>>
>>> Folks,
>>>
>>> Could someone commit the patch for us?  We do not have write access to
>>> Glibc
>>> repo.  We'll be around to cope with bugs (if any).
>>>
>> Please regenerate the patch with "gcc format-patch" and resubmit it
>> so that I can do "git am your-patch".
>>
>> Thanks.
>
> Thanks a lot.

I reformatted your change, fixed ChangeLog entry and added a commit log.
This is what I checked in.

Please provide proper ChangeLog and commit log next time.

Thanks.

-- 
H.J.

[-- Attachment #2: 0001-Add-forced-deletion-support-to-_dl_close_worker.patch --]
[-- Type: text/x-patch, Size: 10884 bytes --]

From 50b5a8420469dd324e58c07d9c42e286d7fe005b Mon Sep 17 00:00:00 2001
From: Pavel Kopyl <p.kopyl@samsung.com>
Date: Tue, 7 Jul 2015 18:45:46 +0300
Subject: [PATCH] Add forced deletion support to _dl_close_worker

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.

This patch adds a parameter to _dl_close_worker to implement forced object
deletion in case of dlopen() failure:

1. Clears DF_1_NODELETE bit if forced, to allow library to be removed from
memory.
2. For each unique symbol that is defined in this object clears
appropriate entry in _ns_unique_sym_table.

	[BZ #17833]
	* elf/Makefile (tests): Add tst-nodelete.
	(modules-names): Add tst-nodelete-uniquemod.
	(tst-nodelete-uniquemod.so-no-z-defs): New.
	(tst-nodelete-rtldmod.so-no-z-defs): Likewise.
	(tst-nodelete-zmod.so-no-z-defs): Likewise.
	($(objpfx)tst-nodelete): Likewise.
	($(objpfx)tst-nodelete.out): Likewise.
	(LDFLAGS-tst-nodelete): Likewise.
	(LDFLAGS-tst-nodelete-zmod.so): Likewise.
	* elf/dl-close.c (_dl_close_worker): Add a parameter to
	implement forced object deletion.
	(_dl_close): Pass false to _dl_close_worker.
	* elf/dl-open.c (_dl_open): Pass true to _dl_close_worker.
	* elf/tst-nodelete.cc: New file.
	* elf/tst-nodeletelib.cc: Likewise.
	* elf/tst-znodeletelib.cc: Likewise.
	* include/dlfcn.h (_dl_close_worker): Add a new parameter.
---
 elf/Makefile                  | 17 +++++++++++++--
 elf/dl-close.c                | 33 ++++++++++++++++++++++++++--
 elf/dl-open.c                 |  2 +-
 elf/tst-nodelete-rtldmod.cc   |  6 +++++
 elf/tst-nodelete-uniquemod.cc | 14 ++++++++++++
 elf/tst-nodelete-zmod.cc      |  6 +++++
 elf/tst-nodelete.cc           | 51 +++++++++++++++++++++++++++++++++++++++++++
 elf/tst-znodelete-zlib.cc     |  6 +++++
 include/dlfcn.h               |  3 ++-
 9 files changed, 132 insertions(+), 6 deletions(-)
 create mode 100644 elf/tst-nodelete-rtldmod.cc
 create mode 100644 elf/tst-nodelete-uniquemod.cc
 create mode 100644 elf/tst-nodelete-zmod.cc
 create mode 100644 elf/tst-nodelete.cc
 create mode 100644 elf/tst-znodelete-zlib.cc

diff --git a/elf/Makefile b/elf/Makefile
index f21276c0..bd0f24d 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -132,7 +132,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 \
@@ -207,7 +207,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 \
@@ -591,6 +593,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.
@@ -1153,6 +1158,14 @@ $(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..2104674 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
-- 
2.4.3

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

[Pavel Kopyl]

On 07/07/2015 09:10 PM, H.J. Lu wrote:
> On Tue, Jul 7, 2015 at 9:27 AM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>>
>> On 07/07/2015 06:30 PM, H.J. Lu wrote:
>>> On Tue, Jul 7, 2015 at 12:41 AM, Yury Gribov <y.gribov@samsung.com> wrote:
>>>> On 07/03/2015 02:46 AM, H.J. Lu wrote:
>>>>> On Thu, Jul 2, 2015 at 3:53 PM, Pavel Kopyl <p.kopyl@samsung.com> wrote:
>>>>>>
>>>>>>
>>>>>> On 06/30/2015 06:12 PM, H.J. Lu wrote:
>>>>>>>
>>>>>>> On Tue, Jun 30, 2015 at 7:45 AM, Pavel Kopyl <p.kopyl@samsung.com>
>>>>>>> wrote:
>>>>>>> This patch changes DF_1_NODELETE path.  Do we have a testcase for
>>>>>>> sucessfully loading/unloading DF_1_NODELETE DSO with undefined
>>>>>>> symbols?
>>>>>>>
>>>>>>> It may have been asked before.  Can we reset unique symbols
>>>>>>> in _dl_open before calling _dl_close_worker?
>>>>>>>
>>>>>> Yes, I added testcases for three possible ways where we can get
>>>>>> DF_1_NODELETE:
>>>>>> 1. Unique symbols
>>>>>> 2. Load with RTLD_NODELETE flag.
>>>>>> 3. Link with '-z nodelete' option
>>>>>>
>>>>>>> Can we reset unique symbols in _dl_open before calling
>>>>>>> _dl_close_worker?
>>>>>> But I clear unique symbols exactly in
>>>>>> _dl_close_worker.<https://slovari.yandex.ru/exactly/en-ru>
>>>>>>
>>>>>>
>>>>> Looks good to me.
>>>>
>>>> Folks,
>>>>
>>>> Could someone commit the patch for us?  We do not have write access to
>>>> Glibc
>>>> repo.  We'll be around to cope with bugs (if any).
>>>>
>>> Please regenerate the patch with "gcc format-patch" and resubmit it
>>> so that I can do "git am your-patch".
>>>
>>> Thanks.
>> Thanks a lot.
> I reformatted your change, fixed ChangeLog entry and added a commit log.
> This is what I checked in.
>
> Please provide proper ChangeLog and commit log next time.
>
> Thanks.
>
Thank you for clarification.
I wonder how should I format patch if I need to mention several authors?

-Pavel

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

[Andreas Schwab]

Pavel Kopyl <p.kopyl@samsung.com> writes:

> @@ -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;

This is a very bad idea.  It can cause libpthread to be unloaded, which
will crash ld.so on the next __rtld_lock_unlock.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Andreas Schwab]

Pavel Kopyl <p.kopyl@samsung.com> writes:

>  elf/tst-znodelete-zlib.cc     |    6 +++++

That file is not used anywhere.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Andreas Schwab]

Pavel Kopyl <p.kopyl@samsung.com> writes:

> 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;

This will remove the NODELETE flag from *all* loaded objects.  That
doesn't make sense.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Maxim Ostapenko]

[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]

Hi!

On 06/08/15 18:30, Andreas Schwab wrote:
> Pavel Kopyl <p.kopyl@samsung.com> writes:
>
>> 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;
> This will remove the NODELETE flag from *all* loaded objects.  That
> doesn't make sense.
>
> Andreas.
>

Indeed, we shouldn't remove NODELETE from all loaded objects, only for 
buggy library. Here a draft patch that should fix the issue. Andreas, 
does this look reasonable for you? If yes, I'll reformat it (e.g. add 
proper ChangeLog entry etc) and send for review as BZ#18778 fix.

-Maxim

[-- Attachment #2: bz18778.diff --]
[-- Type: text/x-patch, Size: 2427 bytes --]

diff --git a/elf/dl-close.c b/elf/dl-close.c
index 9105277..5caf530 100644
--- a/elf/dl-close.c
+++ b/elf/dl-close.c
@@ -144,6 +144,14 @@ _dl_close_worker (struct link_map *map, bool force)
   char done[nloaded];
   struct link_map *maps[nloaded];
 
+   /* Clear DF_1_NODELETE to force object deletion.  We don't need to touch
+      l_tls_dtor_count because forced object deletion only happens when an
+      error occurs during object load.  Destructor registration for TLS
+      non-POD objects should not have happened till then for this
+      object.  */
+  if (force)
+    map->l_flags_1 &= ~DF_1_NODELETE;
+
   /* Run over the list and assign indexes to the link maps and enter
      them into the MAPS array.  */
   int idx = 0;
@@ -153,13 +161,6 @@ _dl_close_worker (struct link_map *map, bool force)
       maps[idx] = l;
       ++idx;
 
-      /* Clear DF_1_NODELETE to force object deletion.  We don't need to touch
-	 l_tls_dtor_count because forced object deletion only happens when an
-	 error occurs during object load.  Destructor registration for TLS
-	 non-POD objects should not have happened till then for this
-	 object.  */
-      if (force)
-	l->l_flags_1 &= ~DF_1_NODELETE;
     }
   assert (idx == nloaded);
 
diff --git a/elf/tst-nodelete.cc b/elf/tst-nodelete.cc
index 176cb68..4152bc3 100644
--- a/elf/tst-nodelete.cc
+++ b/elf/tst-nodelete.cc
@@ -1,12 +1,15 @@
 #include "../dlfcn/dlfcn.h"
 #include <stdio.h>
 #include <stdlib.h>
+#include <gnu/lib-names.h>
 
 static int
 do_test (void)
 {
   int result = 0;
 
+  void *pthread = dlopen (LIBPTHREAD_SO, RTLD_LAZY);
+
   /* 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
@@ -30,6 +33,9 @@ do_test (void)
       result = 1;
     }
 
+ if (pthread)
+     dlclose (pthread);
+
    /* 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
diff --git a/elf/tst-znodelete-zlib.cc b/elf/tst-znodelete-zlib.cc
deleted file mode 100644
index 1e8f368..0000000
--- a/elf/tst-znodelete-zlib.cc
+++ /dev/null
@@ -1,6 +0,0 @@
-extern int not_exist (void);
-
-int foo (void)
-{
-  return  not_exist ();
-}

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

[H.J. Lu]

On Fri, Aug 7, 2015 at 8:58 AM, Maxim Ostapenko
<m.ostapenko@partner.samsung.com> wrote:
> Hi!
>
> On 06/08/15 18:30, Andreas Schwab wrote:
>>
>> Pavel Kopyl <p.kopyl@samsung.com> writes:
>>
>>> 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;
>>
>> This will remove the NODELETE flag from *all* loaded objects.  That
>> doesn't make sense.
>>
>> Andreas.
>>
>
> Indeed, we shouldn't remove NODELETE from all loaded objects, only for buggy
> library. Here a draft patch that should fix the issue. Andreas, does this
> look reasonable for you? If yes, I'll reformat it (e.g. add proper ChangeLog
> entry etc) and send for review as BZ#18778 fix.
>

Please include a testcase to verify that the bug is fixed.

-- 
H.J.

[PATCH][BZ #18778] Clear DF_1_NODELETE flag only for dlopen failed library.

[Maxim Ostapenko]

[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]



On 07/08/15 19:14, H.J. Lu wrote:
> On Fri, Aug 7, 2015 at 8:58 AM, Maxim Ostapenko
> <m.ostapenko@partner.samsung.com> wrote:
>> Hi!
>>
>> On 06/08/15 18:30, Andreas Schwab wrote:
>>> Pavel Kopyl <p.kopyl@samsung.com> writes:
>>>
>>>> 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;
>>> This will remove the NODELETE flag from *all* loaded objects.  That
>>> doesn't make sense.
>>>
>>> Andreas.
>>>
>> Indeed, we shouldn't remove NODELETE from all loaded objects, only for buggy
>> library. Here a draft patch that should fix the issue. Andreas, does this
>> look reasonable for you? If yes, I'll reformat it (e.g. add proper ChangeLog
>> entry etc) and send for review as BZ#18778 fix.
>>
> Please include a testcase to verify that the bug is fixed.
>

This patch fixes BZ #18778 issue by moving l->l_flags_1 &= 
~DF_1_NODELETE out of loop through all loaded libraries and performs 
this action only on inconsistent one.

No regressions on x86_64-unknown-linux-gnu, testcase is attached, OK for 
master?

-Maxim


[-- Attachment #2: 0001-Clear-DF_1_NODELETE-flag-only-for-failed-to-load-lib.patch --]
[-- Type: text/x-patch, Size: 7445 bytes --]

From 828eae1cbaa389b9931dbe0d2111169eede6caf3 Mon Sep 17 00:00:00 2001
From: Maxim Ostapenko <m.ostapenko@partner.samsung.com>
Date: Mon, 10 Aug 2015 10:47:54 +0300
Subject: [PATCH] Clear DF_1_NODELETE flag only for failed to load library.

https://sourceware.org/bugzilla/show_bug.cgi?id=18778

If dlopen fails to load an object that has triggered loading libpthread it
causes ld.so to unload libpthread because its DF_1_NODELETE flags has been
forcefully cleared. The next call to __rtdl_unlock_lock_recursive will crash
since pthread_mutex_unlock no longer exists.

This patch moves l->l_flags_1 &= ~DF_1_NODELETE out of loop through all loaded
libraries and performs the action only on inconsistent one.

	[BZ #18778]
	* elf/Makefile (tests): Add Add tst-nodelete2.
	(modules-names): Add tst-nodelete2mod.
	(tst-nodelete2mod.so-no-z-defs): New.
	($(objpfx)tst-nodelete2): Likewise.
	($(objpfx)tst-nodelete2.out): Likewise.
	(LDFLAGS-tst-nodelete2): Likewise.
	* elf/dl-close.c (_dl_close_worker): Move DF_1_NODELETE clearing
	out of loop through all loaded libraries.
	* elf/tst-nodelete2.c: New file.
	* elf/tst-nodelete2mod.c: Likewise.
---
 ChangeLog                 | 13 +++++++++++++
 NEWS                      |  2 +-
 elf/Makefile              | 11 +++++++++--
 elf/dl-close.c            | 15 ++++++++-------
 elf/tst-nodelete2.c       | 37 +++++++++++++++++++++++++++++++++++++
 elf/tst-nodelete2mod.c    |  7 +++++++
 elf/tst-znodelete-zlib.cc |  6 ------
 7 files changed, 75 insertions(+), 16 deletions(-)
 create mode 100644 elf/tst-nodelete2.c
 create mode 100644 elf/tst-nodelete2mod.c
 delete mode 100644 elf/tst-znodelete-zlib.cc

diff --git a/ChangeLog b/ChangeLog
index d4c9d79..4e93c73 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2015-08-10  Maxim Ostapenko  <m.ostapenko@partner.samsung.com>
+	[BZ #18778]
+	* elf/Makefile (tests): Add Add tst-nodelete2.
+	(modules-names): Add tst-nodelete2mod.
+	(tst-nodelete2mod.so-no-z-defs): New.
+	($(objpfx)tst-nodelete2): Likewise.
+	($(objpfx)tst-nodelete2.out): Likewise.
+	(LDFLAGS-tst-nodelete2): Likewise.
+	* elf/dl-close.c (_dl_close_worker): Move DF_1_NODELETE clearing
+	out of loop through all loaded libraries.
+	* elf/tst-nodelete2.c: New file.
+	* elf/tst-nodelete2mod.c: Likewise.
+
 2015-08-09  H.J. Lu  <hongjiu.lu@intel.com>
 
 	[BZ #18674]
diff --git a/NEWS b/NEWS
index 7bd785a..ae761ed 100644
--- a/NEWS
+++ b/NEWS
@@ -10,7 +10,7 @@ Version 2.23
 * The following bugs are resolved with this release:
 
   16517, 16519, 17905, 18265, 18480, 18525, 18618, 18647, 18661, 18674,
-  18787.
+  18787, 18778.
 \f
 Version 2.22
 
diff --git a/elf/Makefile b/elf/Makefile
index 4ceeaf8..71a18a1 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -148,7 +148,8 @@ tests += loadtest restest1 preloadtest loadfail multiload origtest resolvfail \
 	 tst-unique1 tst-unique2 $(if $(CXX),tst-unique3 tst-unique4 \
 	 tst-nodelete) \
 	 tst-initorder tst-initorder2 tst-relsort1 tst-null-argv \
-	 tst-ptrguard1 tst-tlsalign tst-tlsalign-extern tst-nodelete-opened
+	 tst-ptrguard1 tst-tlsalign tst-tlsalign-extern tst-nodelete-opened \
+	 tst-nodelete2
 #	 reldep9
 ifeq ($(build-hardcoded-path-in-tests),yes)
 tests += tst-dlopen-aout
@@ -218,7 +219,7 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
 		tst-initorder2d \
 		tst-relsort1mod1 tst-relsort1mod2 tst-array2dep \
 		tst-array5dep tst-null-argv-lib \
-		tst-tlsalign-lib tst-nodelete-opened-lib
+		tst-tlsalign-lib tst-nodelete-opened-lib tst-nodelete2mod
 ifeq (yes,$(have-protected-data))
 modules-names += tst-protected1moda tst-protected1modb
 tests += tst-protected1a tst-protected1b
@@ -594,6 +595,7 @@ 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
+tst-nodelete2mod.so-no-z-defs = yes
 
 ifeq ($(build-shared),yes)
 # Build all the modules even when not actually running test programs.
@@ -1164,6 +1166,11 @@ $(objpfx)tst-nodelete.out: $(objpfx)tst-nodelete-uniquemod.so \
 LDFLAGS-tst-nodelete = -rdynamic
 LDFLAGS-tst-nodelete-zmod.so = -Wl,--enable-new-dtags,-z,nodelete
 
+$(objpfx)tst-nodelete2: $(libdl)
+$(objpfx)tst-nodelete2.out: $(objpfx)tst-nodelete2mod.so
+
+LDFLAGS-tst-nodelete2 = -rdynamic
+
 $(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 9105277..c897247 100644
--- a/elf/dl-close.c
+++ b/elf/dl-close.c
@@ -144,6 +144,14 @@ _dl_close_worker (struct link_map *map, bool force)
   char done[nloaded];
   struct link_map *maps[nloaded];
 
+  /* Clear DF_1_NODELETE to force object deletion.  We don't need to touch
+     l_tls_dtor_count because forced object deletion only happens when an
+     error occurs during object load.  Destructor registration for TLS
+     non-POD objects should not have happened till then for this
+     object.  */
+  if (force)
+    map->l_flags_1 &= ~DF_1_NODELETE;
+
   /* Run over the list and assign indexes to the link maps and enter
      them into the MAPS array.  */
   int idx = 0;
@@ -153,13 +161,6 @@ _dl_close_worker (struct link_map *map, bool force)
       maps[idx] = l;
       ++idx;
 
-      /* Clear DF_1_NODELETE to force object deletion.  We don't need to touch
-	 l_tls_dtor_count because forced object deletion only happens when an
-	 error occurs during object load.  Destructor registration for TLS
-	 non-POD objects should not have happened till then for this
-	 object.  */
-      if (force)
-	l->l_flags_1 &= ~DF_1_NODELETE;
     }
   assert (idx == nloaded);
 
diff --git a/elf/tst-nodelete2.c b/elf/tst-nodelete2.c
new file mode 100644
index 0000000..388e8af
--- /dev/null
+++ b/elf/tst-nodelete2.c
@@ -0,0 +1,37 @@
+#include "../dlfcn/dlfcn.h"
+#include <stdio.h>
+#include <stdlib.h>
+#include <gnu/lib-names.h>
+
+static int
+do_test (void)
+{
+  int result = 0;
+
+  printf ("\nOpening pthread library.\n");
+  void *pthread = dlopen (LIBPTHREAD_SO, RTLD_LAZY);
+
+  /* This is a test for correct DF_1_NODELETE clearing when dlopen failure
+     happens.  We should clear DF_1_NODELETE for failed library only, because
+     doing this for others (e.g. libpthread) might cause them to be unloaded,
+     that may lead to some global references (e.g. __rtld_lock_unlock) to be
+     broken.  The dlopen should fail because of undefined symbols in shared
+     library, that cause DF_1_NODELETE to be cleared.  For libpthread, this
+     flag should be set, because if not, SIGSEGV will happen in dlclose.  */
+  if (dlopen ("tst-nodelete2mod.so", RTLD_NOW) != NULL)
+    {
+      printf ("Unique symbols test failed\n");
+      result = 1;
+    }
+
+  if (pthread)
+    dlclose (pthread);
+
+  if (result == 0)
+    printf ("SUCCESS\n");
+
+  return result;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/elf/tst-nodelete2mod.c b/elf/tst-nodelete2mod.c
new file mode 100644
index 0000000..e88c756
--- /dev/null
+++ b/elf/tst-nodelete2mod.c
@@ -0,0 +1,7 @@
+/* Undefined symbol.  */
+extern int not_exist (void);
+
+int foo (void)
+{
+  return not_exist ();
+}
diff --git a/elf/tst-znodelete-zlib.cc b/elf/tst-znodelete-zlib.cc
deleted file mode 100644
index 1e8f368..0000000
--- a/elf/tst-znodelete-zlib.cc
+++ /dev/null
@@ -1,6 +0,0 @@
-extern int not_exist (void);
-
-int foo (void)
-{
-  return  not_exist ();
-}
-- 
1.9.1

Re: [PATCH][BZ #18778] Clear DF_1_NODELETE flag only for dlopen failed library.

[Andreas Schwab]

Maxim Ostapenko <m.ostapenko@partner.samsung.com> writes:

> 	[BZ #18778]
> 	* elf/Makefile (tests): Add Add tst-nodelete2.
> 	(modules-names): Add tst-nodelete2mod.
> 	(tst-nodelete2mod.so-no-z-defs): New.
> 	($(objpfx)tst-nodelete2): Likewise.
> 	($(objpfx)tst-nodelete2.out): Likewise.
> 	(LDFLAGS-tst-nodelete2): Likewise.
> 	* elf/dl-close.c (_dl_close_worker): Move DF_1_NODELETE clearing
> 	out of loop through all loaded libraries.
> 	* elf/tst-nodelete2.c: New file.
> 	* elf/tst-nodelete2mod.c: Likewise.

Ok.  Please also cherry-pick to 2.22 branch.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH][BZ #18778] Clear DF_1_NODELETE flag only for dlopen failed library.

[Maxim Ostapenko]

On 11/08/15 10:41, Andreas Schwab wrote:
> Maxim Ostapenko <m.ostapenko@partner.samsung.com> writes:
>
>> 	[BZ #18778]
>> 	* elf/Makefile (tests): Add Add tst-nodelete2.
>> 	(modules-names): Add tst-nodelete2mod.
>> 	(tst-nodelete2mod.so-no-z-defs): New.
>> 	($(objpfx)tst-nodelete2): Likewise.
>> 	($(objpfx)tst-nodelete2.out): Likewise.
>> 	(LDFLAGS-tst-nodelete2): Likewise.
>> 	* elf/dl-close.c (_dl_close_worker): Move DF_1_NODELETE clearing
>> 	out of loop through all loaded libraries.
>> 	* elf/tst-nodelete2.c: New file.
>> 	* elf/tst-nodelete2mod.c: Likewise.
> Ok.  Please also cherry-pick to 2.22 branch.
>
> Andreas.
>

Andreas, thanks! Actually, I haven't write access to Glibc repository, 
so could you apply it for me?

-Maxim

Re: [PATCH][BZ #18778] Clear DF_1_NODELETE flag only for dlopen failed library.

[Florian Weimer]

On 08/10/2015 01:56 PM, Maxim Ostapenko wrote:

> This patch fixes BZ #18778 issue by moving l->l_flags_1 &=
> ~DF_1_NODELETE out of loop through all loaded libraries and performs
> this action only on inconsistent one.
> 
> No regressions on x86_64-unknown-linux-gnu, testcase is attached, OK for
> master?

This patch deletes elf/tst-znodelete-zlib.cc.  Is this intentional?


-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH][BZ #18778] Clear DF_1_NODELETE flag only for dlopen failed library.

[Maxim Ostapenko]

On 04/09/15 12:24, Florian Weimer wrote:
> On 08/10/2015 01:56 PM, Maxim Ostapenko wrote:
>
>> This patch fixes BZ #18778 issue by moving l->l_flags_1 &=
>> ~DF_1_NODELETE out of loop through all loaded libraries and performs
>> this action only on inconsistent one.
>>
>> No regressions on x86_64-unknown-linux-gnu, testcase is attached, OK for
>> master?
> This patch deletes elf/tst-znodelete-zlib.cc.  Is this intentional?
>
>

Yeah, according to this Andreas's comment, I've deleted it.

 >> Pavel Kopyl <p.kopyl@samsung.com> writes:
 >>   elf/tst-znodelete-zlib.cc     |    6 +++++

 > That file is not used anywhere.

 > Andreas.

Perhaps I just forgot to mention this in CL and commit message. Did you 
meet some problem here?

Re: [PATCH][BZ #18778] Clear DF_1_NODELETE flag only for dlopen failed library.

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 958 bytes --]

On 09/04/2015 11:32 AM, Maxim Ostapenko wrote:
> On 04/09/15 12:24, Florian Weimer wrote:
>> On 08/10/2015 01:56 PM, Maxim Ostapenko wrote:
>>
>>> This patch fixes BZ #18778 issue by moving l->l_flags_1 &=
>>> ~DF_1_NODELETE out of loop through all loaded libraries and performs
>>> this action only on inconsistent one.
>>>
>>> No regressions on x86_64-unknown-linux-gnu, testcase is attached, OK for
>>> master?
>> This patch deletes elf/tst-znodelete-zlib.cc.  Is this intentional?
>>
>>
> 
> Yeah, according to this Andreas's comment, I've deleted it.
> 
>>> Pavel Kopyl <p.kopyl@samsung.com> writes:
>>>   elf/tst-znodelete-zlib.cc     |    6 +++++
> 
>> That file is not used anywhere.
> 
>> Andreas.
> 
> Perhaps I just forgot to mention this in CL and commit message. Did you
> meet some problem here?

No, I just noticed the ChangeLog inconsistency.  I've committed the
attached patch to address that.

-- 
Florian Weimer / Red Hat Product Security

[-- Attachment #2: 0001-Amend-ChangeLog-to-reflect-deletion-of-elf-tst-znode.patch --]
[-- Type: text/x-patch, Size: 864 bytes --]

From f834e6dc705c80550acf610d507b3e4cdf19693e Mon Sep 17 00:00:00 2001
Message-Id: <f834e6dc705c80550acf610d507b3e4cdf19693e.1441359340.git.fweimer@redhat.com>
From: Florian Weimer <fweimer@redhat.com>
Date: Fri, 4 Sep 2015 11:35:02 +0200
Subject: [PATCH] Amend ChangeLog to reflect deletion of
 elf/tst-znodelete-zlib.cc
To: libc-alpha@sourceware.org

Commit f25238ffe0455013174438376b3ee88df496f9d1 deleted this file
because it was noted during review that it was unused.
---
 ChangeLog | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ChangeLog b/ChangeLog
index 14407f1..ab0031d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1462,6 +1462,7 @@
 	out of loop through all loaded libraries.
 	* elf/tst-nodelete2.c: New file.
 	* elf/tst-nodelete2mod.c: Likewise.
+	* elf/tst-znodelete-zlib.cc: Delete.
 
 2015-08-11  Andreas Schwab  <schwab@suse.de>
 
-- 
2.4.3

Re: [PATCH][BZ #18778] Clear DF_1_NODELETE flag only for dlopen failed library.

[Maxim Ostapenko]

On 04/09/15 12:36, Florian Weimer wrote:
> On 09/04/2015 11:32 AM, Maxim Ostapenko wrote:
>> On 04/09/15 12:24, Florian Weimer wrote:
>>> On 08/10/2015 01:56 PM, Maxim Ostapenko wrote:
>>>
>>>> This patch fixes BZ #18778 issue by moving l->l_flags_1 &=
>>>> ~DF_1_NODELETE out of loop through all loaded libraries and performs
>>>> this action only on inconsistent one.
>>>>
>>>> No regressions on x86_64-unknown-linux-gnu, testcase is attached, OK for
>>>> master?
>>> This patch deletes elf/tst-znodelete-zlib.cc.  Is this intentional?
>>>
>>>
>> Yeah, according to this Andreas's comment, I've deleted it.
>>
>>>> Pavel Kopyl <p.kopyl@samsung.com> writes:
>>>>    elf/tst-znodelete-zlib.cc     |    6 +++++
>>> That file is not used anywhere.
>>> Andreas.
>> Perhaps I just forgot to mention this in CL and commit message. Did you
>> meet some problem here?
> No, I just noticed the ChangeLog inconsistency.  I've committed the
> attached patch to address that.
>

Oh, sorry for inconvenience. Thanks!