[review v3] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]

["Carlos O'Donell (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>]

Carlos O'Donell has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471
......................................................................


Patch Set 3: Code-Review+2

(43 comments)

This change is complex to review because we touch so many aspects of NODELETE handling, but in the end many of the changes are somewhat mechanical. The clearly important aspects are when pending nodelete is turned into active nodelete. These changes look logically correct to me. The place where I ad one worry was the possibly failing malloc in add_dependency, but it is not a showstopper to getting better semantics for NODELETE handling and all of the debug printfs you add are immensely helpful in determining what is going on with the internal loader state and why an object won't be removed (this is very useful to C++ developers suffering from STB_GNU_UNIQUE issues and trying to figure out why a particular plugin won't be unloaded, so thanks).

I think this should go in immediately. I appreciate this going in now rather than much closer to the release window. We'll be able to get some Fedora Rawhide testing with this.

OK for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

| --- elf/Makefile
| +++ elf/Makefile
| @@ -188,18 +188,18 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
|  	 tst-nodelete) \
|  	 tst-initorder tst-initorder2 tst-relsort1 tst-null-argv \
|  	 tst-tlsalign tst-tlsalign-extern tst-nodelete-opened \
|  	 tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
|  	 tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
|  	 tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
|  	 tst-unwind-ctor tst-unwind-main tst-audit13 \
|  	 tst-sonamemove-link tst-sonamemove-dlopen tst-dlopen-tlsmodid \
| -	 tst-dlopen-self tst-auditmany tst-initfinilazyfail
| +	 tst-dlopen-self tst-auditmany tst-initfinilazyfail tst-dlopenfail

PS3, Line 196:

OK. New test case.

|  #	 reldep9
|  tests-internal += loadtest unload unload2 circleload1 \
|  	 neededtest neededtest2 neededtest3 neededtest4 \
|  	 tst-tls3 tst-tls6 tst-tls7 tst-tls8 tst-dlmopen2 \
|  	 tst-ptrguard1 tst-stackguard1 tst-libc_dlvsym \
|  	 tst-create_format1
|  tests-container += tst-pldd tst-dlopen-tlsmodid-container \
|    tst-dlopen-self-container
|  test-srcs = tst-pathopt

 ...

| @@ -281,17 +281,18 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
|  		tst-main1mod tst-libc_dlvsym-dso tst-absolute-sym-lib \
|  		tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib \
|  		tst-audit13mod1 tst-sonamemove-linkmod1 \
|  		tst-sonamemove-runmod1 tst-sonamemove-runmod2 \
|  		tst-auditmanymod1 tst-auditmanymod2 tst-auditmanymod3 \
|  		tst-auditmanymod4 tst-auditmanymod5 tst-auditmanymod6 \
|  		tst-auditmanymod7 tst-auditmanymod8 tst-auditmanymod9 \
| -		tst-initlazyfailmod tst-finilazyfailmod
| +		tst-initlazyfailmod tst-finilazyfailmod \
| +		tst-dlopenfailmod1 tst-dlopenfaillinkmod tst-dlopenfailmod2

PS3, Line 289:

OK. Two 3 new dsos.

|  # Most modules build with _ISOMAC defined, but those filtered out
|  # depend on internal headers.
|  modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\
|  				   $(modules-names))
|  
|  ifeq (yes,$(have-mtls-dialect-gnu2))
|  tests += tst-gnu2-tls1
|  modules-names += tst-gnu2-tls1mod
|  $(objpfx)tst-gnu2-tls1: $(objpfx)tst-gnu2-tls1mod.so

 ...

| @@ -1594,0 +1595,10 @@ LDFLAGS-tst-finilazyfailmod.so = \
| +
| +$(objpfx)tst-dlopenfail: $(libdl)
| +$(objpfx)tst-dlopenfail.out: \
| +  $(objpfx)tst-dlopenfailmod1.so $(objpfx)tst-dlopenfailmod2.so
| +# Order matters here.  tst-dlopenfaillinkmod.so's soname ensures
| +# a run-time loader failure.
| +$(objpfx)tst-dlopenfailmod1.so: \
| +  $(shared-thread-library) $(objpfx)tst-dlopenfaillinkmod.so
| +LDFLAGS-tst-dlopenfaillinkmod.so = -Wl,-soname,tst-dlopenfail-missingmod.so
| +$(objpfx)tst-dlopenfailmod2.so: $(shared-thread-library)

PS3, Line 1604:

OK. Setup the test case flags.

| --- elf/dl-close.c
| +++ elf/dl-close.c
| @@ -168,19 +168,11 @@ _dl_close_worker (struct link_map *map, bool force)
|    char done[nloaded];
|    struct link_map *maps[nloaded];
|  
| -  /* Clear DF_1_NODELETE to force object deletion.  We don't need to touch
| -     l_tls_dtor_count because forced object deletion only happens when an
| -     error occurs during object load.  Destructor registration for TLS
| -     non-POD objects should not have happened till then for this
| -     object.  */
| -  if (force)
| -    map->l_flags_1 &= ~DF_1_NODELETE;

PS3, Line 177:

OK, remove clearing of DF_1_NODLETE in _dl_close_worker, this is all
going to be handled in a more structured way via l_nodelete.

| -
|    /* Run over the list and assign indexes to the link maps and enter
|       them into the MAPS array.  */
|    int idx = 0;
|    for (struct link_map *l = ns->_ns_loaded; l != NULL; l = l->l_next)
|      {
|        l->l_idx = idx;
|        maps[idx] = l;
|        ++idx;

 ...

| @@ -200,18 +192,18 @@ _dl_close_worker (struct link_map *map, bool force)
|  
|        if (done[done_index])
|  	/* Already handled.  */
|  	continue;
|  
|        /* Check whether this object is still used.  */
|        if (l->l_type == lt_loaded
|  	  && l->l_direct_opencount == 0
| -	  && (l->l_flags_1 & DF_1_NODELETE) == 0
| +	  && l->l_nodelete != link_map_nodelete_active

PS3, Line 200:

OK. First check where instead of using l_flags_1 we use l_nodelete. We
check to see if nodelete semantics are active (rather than pending).

|  	  /* See CONCURRENCY NOTES in cxa_thread_atexit_impl.c to know why
|  	     acquire is sufficient and correct.  */
|  	  && atomic_load_acquire (&l->l_tls_dtor_count) == 0
|  	  && !used[done_index])
|  	continue;
|  
|        /* We need this object and we handle it now.  */
|        done[done_index] = 1;
|        used[done_index] = 1;

 ...

| @@ -283,18 +275,18 @@ #endif
|        struct link_map *imap = maps[i];
|  
|        /* All elements must be in the same namespace.  */
|        assert (imap->l_ns == nsid);
|  
|        if (!used[i])
|  	{
|  	  assert (imap->l_type == lt_loaded
| -		  && (imap->l_flags_1 & DF_1_NODELETE) == 0);
| +		  && imap->l_nodelete != link_map_nodelete_active);

PS3, Line 283:

OK. Likwise, check for nodelete active (rather than pending).

|  
|  	  /* Call its termination function.  Do not do it for
|  	     half-cooked objects.  Temporarily disable exception
|  	     handling, so that errors are fatal.  */
|  	  if (imap->l_init_called)
|  	    {
|  	      /* When debugging print a message first.  */
|  	      if (__builtin_expect (GLRO(dl_debug_mask) & DL_DEBUG_IMPCALLS,
|  				    0))

 ...

| @@ -833,18 +825,18 @@ _dl_close (void *_map)
|       concurrent dlopens.  */
|    __rtld_lock_lock_recursive (GL(dl_load_lock));
|  
|    /* At this point we are guaranteed nobody else is touching the list of
|       loaded maps, but a concurrent dlclose might have freed our map
|       before we took the lock. There is no way to detect this (see below)
|       so we proceed assuming this isn't the case.  First see whether we
|       can remove the object at all.  */
| -  if (__glibc_unlikely (map->l_flags_1 & DF_1_NODELETE))
| +  if (__glibc_unlikely (map->l_nodelete == link_map_nodelete_active))

PS3, Line 833:

OK. Likewise check for nodelete active (rather than pending).

|      {
|        /* Nope.  Do nothing.  */
|        __rtld_lock_unlock_recursive (GL(dl_load_lock));
|        return;
|      }
|  
|    /* At present this is an unreliable check except in the case where the
|       caller has recursively called dlclose and we are sure the link map
|       has not been freed.  In a non-recursive dlclose the map itself
| --- elf/dl-lookup.c
| +++ elf/dl-lookup.c

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ib2a3d86af6f92d75baca65431d74783ee0dbc292
Gerrit-Change-Number: 471
Gerrit-PatchSet: 3
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Christian Häggström <gnugerrit@kalvdans.no-ip.org>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 12:57:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment