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