diff mbox

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

Message ID 55C4D58D.8010307@partner.samsung.com
State New, archived
Headers show

Commit Message

Maxim Ostapenko Aug. 7, 2015, 3:58 p.m. UTC 
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

Comments

H.J. Lu Aug. 7, 2015, 4:14 p.m. UTC | #1 
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.
diff mbox

Patch

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 ();
-}