From patchwork Mon Jun 15 13:07:50 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yury Gribov X-Patchwork-Id: 7183 Received: (qmail 14976 invoked by alias); 15 Jun 2015 13:07:58 -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 14852 invoked by uid 89); 15 Jun 2015 13:07:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.2 required=5.0 tests=AWL, BAYES_50, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=no version=3.3.2 X-HELO: mailout4.w1.samsung.com Message-id: <557ECE26.4040906@samsung.com> Date: Mon, 15 Jun 2015 16:07:50 +0300 From: Yury Gribov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-version: 1.0 To: GNU C Library Cc: Pavel Kopyl , "H.J. Lu" , Carlos O'Donell , Roland McGrath , Viacheslav Garbuzov Subject: [PATCHv5][PING^2][BZ #17833] _dl_close_worker() does not release inconsistent objects. References: <54BD4F65.2090108@samsung.com> <54BEF851.70902@redhat.com> <54DBC3CB.5080703@samsung.com> <54F071DB.9040106@samsung.com> <20150301191710.GB19363@vapier> <54F57B52.6080202@samsung.com> <553A1BEE.6070705@samsung.com> <5553382F.3020906@samsung.com> <5565B5E5.7060101@samsung.com> <5565C2A8.60306@samsung.com> <5565C862.1040003@samsung.com> <5566395A.3090605@samsung.com> <5567892C.4070004@samsung.com> <5568A408.2080903@samsung.com> <55714557.1020304@samsung.com> In-reply-to: <55714557.1020304@samsung.com> Content-type: multipart/mixed; boundary=------------090105080002040500070807 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 >>>> 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 >>> #include >>> #include >>> >>> 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 Date: Fri May 29 20:15:33 2015 +0300 2015-05-29 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-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. diff --git a/elf/Makefile b/elf/Makefile index dedf3c7..a46eaff 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -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) 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; } 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)); } diff --git a/elf/dl-open.c b/elf/dl-open.c index 2d0e082..027c1e0 100644 --- a/elf/dl-open.c +++ b/elf/dl-open.c @@ -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); diff --git a/elf/tst-nodelete-rtldmod.cc b/elf/tst-nodelete-rtldmod.cc new file mode 100644 index 0000000..740e1d8 --- /dev/null +++ b/elf/tst-nodelete-rtldmod.cc @@ -0,0 +1,6 @@ +extern int not_exist (void); + +int foo (void) +{ + return not_exist (); +} diff --git a/elf/tst-nodelete-uniquemod.cc b/elf/tst-nodelete-uniquemod.cc new file mode 100644 index 0000000..632b303 --- /dev/null +++ b/elf/tst-nodelete-uniquemod.cc @@ -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 (); +} diff --git a/elf/tst-nodelete-zmod.cc b/elf/tst-nodelete-zmod.cc new file mode 100644 index 0000000..740e1d8 --- /dev/null +++ b/elf/tst-nodelete-zmod.cc @@ -0,0 +1,6 @@ +extern int not_exist (void); + +int foo (void) +{ + return not_exist (); +} diff --git a/elf/tst-nodelete.cc b/elf/tst-nodelete.cc new file mode 100644 index 0000000..176cb68 --- /dev/null +++ b/elf/tst-nodelete.cc @@ -0,0 +1,51 @@ +#include "../dlfcn/dlfcn.h" +#include +#include + +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" diff --git a/elf/tst-znodelete-zlib.cc b/elf/tst-znodelete-zlib.cc new file mode 100644 index 0000000..1e8f368 --- /dev/null +++ b/elf/tst-znodelete-zlib.cc @@ -0,0 +1,6 @@ +extern int not_exist (void); + +int foo (void) +{ + return 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