[V2,BZ,#17833,PING] _dl_close_worker() does not release inconsistent objects.
Commit Message
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.
Comments
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
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.
@@ -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)
@@ -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));
}
@@ -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);
new file mode 100644
@@ -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;
+}
new file mode 100644
@@ -0,0 +1,13 @@
+
+extern int not_exist ();
+
+inline int make_unique ()
+{
+ static int unique;
+ return ++unique;
+}
+
+int foo ()
+{
+ return make_unique () + 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