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