From patchwork Wed Feb 11 21:04:11 2015
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
X-Patchwork-Submitter: Pavel Kopyl
X-Patchwork-Id: 438942
Return-Path:
X-Original-To: incoming@patchwork.ozlabs.org
Delivered-To: patchwork-incoming@bilbo.ozlabs.org
Received: from sourceware.org (server1.sourceware.org [209.132.180.131])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256
bits)) (No client certificate requested)
by ozlabs.org (Postfix) with ESMTPS id DBA2614011D
for ;
Thu, 12 Feb 2015 08:15:07 +1100 (AEDT)
DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id
:list-unsubscribe:list-subscribe:list-archive:list-post
:list-help:sender:message-id:date:from:mime-version:to:cc
:subject:references:in-reply-to:content-type; q=dns; s=default; b=
ik7EteLKnbOdbAlmHBoTRP/IZq+Ngs2I2Nis32veKRJd6CnJ1+1lZcNs7eDVTVoh
CNqV4QOq6LxKvglE3sNiw++WTbSMHwRJuZhDVGq1IyxVxJ2s4LzWbCmKUeVz3b2x
Cs0QOpI9ThZ80qMtjZAex4M+RfHKqz0exgyd4K2nIMk=
DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id
:list-unsubscribe:list-subscribe:list-archive:list-post
:list-help:sender:message-id:date:from:mime-version:to:cc
:subject:references:in-reply-to:content-type; s=default; bh=7j4h
uZfHCLAmdbHewv21hUZgg/4=; b=SO7hgrTyoTG6SAvnvKhkWPPLDdCYyaUZiqlM
dKAlYN56Cj5jita01KSlXi+oTpogLrS0Pg/qGAlLm2Fj4TEe5UF91Gfnzpwoj1zg
n9XXr1JZ2TbbtZnz6PnNUHYyG3VTlyU2F+V6yi8WfpVCGb7opMr3WdpfAXhEeVe6
TLeAqoE=
Received: (qmail 13202 invoked by alias); 11 Feb 2015 21:04:15 -0000
Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Id:
List-Unsubscribe:
List-Subscribe:
List-Archive:
List-Post:
List-Help: ,
Sender: libc-alpha-owner@sourceware.org
Delivered-To: mailing list libc-alpha@sourceware.org
Received: (qmail 13171 invoked by uid 89); 11 Feb 2015 21:04:13 -0000
Authentication-Results: sourceware.org; auth=none
X-Virus-Found: No
X-Spam-SWARE-Status: No, score=1.9 required=5.0 tests=AWL, BAYES_00,
SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2
X-HELO: mailout1.w1.samsung.com
Message-id: <54DBC3CB.5080703@samsung.com>
Date: Thu, 12 Feb 2015 00:04:11 +0300
From: Pavel Kopyl
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101
Thunderbird/31.4.0
MIME-version: 1.0
To: Carlos O'Donell , libc-alpha@sourceware.org
Cc: Yury Gribov , m.ilin@samsung.com,
Viacheslav Garbuzov
Subject: [PATCH][V2][BZ #17833] _dl_close_worker() does not release
inconsistent objects.
References: <54BD4F65.2090108@samsung.com> <54BEF851.70902@redhat.com>
In-reply-to: <54BEF851.70902@redhat.com>
Content-type: multipart/mixed; boundary=------------040004070606070107010307
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.
Regards,
Pavel
commit 386f49f30c90af764b720cc6ef01fc559a63de35
Author: Pavel Kopyl
Date: Wed Feb 11 18:24:07 2015 +0300
2015-02-11 Pavel Kopyl
Mikhail Ilin
[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.
diff --git a/elf/Makefile b/elf/Makefile
index f2d1781..2a186ba 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -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)
diff --git a/elf/dl-close.c b/elf/dl-close.c
index cf8f9e0..d640254 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;
}
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));
}
diff --git a/elf/dl-open.c b/elf/dl-open.c
index 47b4cb5..d71c454 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -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);
diff --git a/elf/tst-unique5.cc b/elf/tst-unique5.cc
new file mode 100644
index 0000000..95a25d6
--- /dev/null
+++ b/elf/tst-unique5.cc
@@ -0,0 +1,11 @@
+#include "../dlfcn/dlfcn.h"
+#include
+
+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;
+}
diff --git a/elf/tst-unique5lib.cc b/elf/tst-unique5lib.cc
new file mode 100644
index 0000000..7d17c3b
--- /dev/null
+++ b/elf/tst-unique5lib.cc
@@ -0,0 +1,13 @@
+
+extern int not_exist ();
+
+inline int make_unique ()
+{
+ static int unique;
+ return ++unique;
+}
+
+int foo ()
+{
+ return make_unique () + not_exist ();
+}
diff --git a/include/dlfcn.h b/include/dlfcn.h
index a67b2e3..0ce0af5 100644
--- a/include/dlfcn.h
+++ b/include/dlfcn.h
@@ -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