diff mbox

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

Message ID 55C89171.8090307@partner.samsung.com
State New
Headers show

Commit Message

max Aug. 10, 2015, 11:56 a.m. UTC
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

Comments

Andreas Schwab Aug. 11, 2015, 7:41 a.m. UTC | #1
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.
max Aug. 11, 2015, 8:02 a.m. UTC | #2
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
Florian Weimer Sept. 4, 2015, 9:24 a.m. UTC | #3
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?
max Sept. 4, 2015, 9:32 a.m. UTC | #4
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?
diff mbox

Patch

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