[PATCHv5,PING^3,BZ,#17833] _dl_close_worker() does not release inconsistent objects.
Commit Message
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
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.
@@ -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);
@@ -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
deleted file mode 100644
@@ -1,6 +0,0 @@
-extern int not_exist (void);
-
-int foo (void)
-{
- return not_exist ();
-}