[review] Lazy binding failures during dlopen/dlclose must be fatal [BZ #24304]
diff mbox series

Message ID gerrit.1572549639000.I6b1addfe2e30f50a1781595f046f44173db9491a@gnutoolchain-gerrit.osci.io
State New
Headers show
Series
  • [review] Lazy binding failures during dlopen/dlclose must be fatal [BZ #24304]
Related show

Commit Message

Carlos O'Donell (Code Review) Oct. 31, 2019, 7:20 p.m. UTC
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/467
......................................................................

Lazy binding failures during dlopen/dlclose must be fatal [BZ #24304]

If a lazy binding failure happens during the execution of an ELF
constructor or destructor, the dynamic loader catches the error
and reports it using the dlerror mechanism.  This is undesirable
because there could be other constructors and destructors that
need processing (which are skipped), and the process is in an
inconsistent state at this point.  Therefore, we have to issue
a fatal dynamic loader error error and terminate the process.

Note that the _dl_catch_exception in _dl_open is just an inner catch,
to roll back some state locally.  If called from dlopen, there is
still an outer catch, which is why calling _dl_init via call_dl_init
and a no-exception is required and cannot be avoiding by moving the
_dl_init call directly into _dl_open.

_dl_fini does not need changes because it does not install an error
handler, so errors are already fatal there.

Change-Id: I6b1addfe2e30f50a1781595f046f44173db9491a
---
M elf/Makefile
M elf/dl-close.c
M elf/dl-open.c
A elf/tst-finilazyfailmod.c
A elf/tst-initfinilazyfail.c
A elf/tst-initlazyfailmod.c
6 files changed, 210 insertions(+), 21 deletions(-)

Comments

Carlos O'Donell (Code Review) Nov. 13, 2019, 12:57 p.m. UTC | #1
Florian Weimer has posted comments on this change.

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


Patch Set 2:

This change is ready for review.
Carlos O'Donell (Code Review) Nov. 18, 2019, 4:23 p.m. UTC | #2
Carlos O'Donell has posted comments on this change.

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


Patch Set 3: Code-Review+2

(16 comments)

This patch moves us in the right direction with respect to failure semantics. In the past, and I tested this with various small semantic tests, we used to attempt to back out of the failure in the dlopen case. The result of trying to back out of the dlopen failure has lead to several bugs where we have incomplete state rollback, and some of this is really really hard to get right once you start going past a certain point. What I like about this change is that we draw a line in the sand at the point just before we run foreign functions from the loaded object, so once the foreign functions (constructors) are running, we immediately decide the failures are going to be fatal. This is good because it actually matches what normally happens when you don't use dlopen, so the semantics are better aligned IMO. Terminating the process on lazy symbol resolution during the loading of the object is a change in semantics, but I think this is the correct way forward to some of the complex state roll
 back problems we've seen. Once the foreign functions start running there is no way to rollback state correctly on failure. The rollback there is going to be at the process level e.g. restart the process and try again.

Please update the packaging changes here:
https://www.sourceware.org/glibc/wiki/Release/2.31
to indicate that the semantic change has happened and to watch out for it.

I think we should have a NEWS item for this, but you can commit that in a followup if you want.

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-dlopen-self tst-auditmany tst-initfinilazyfail

PS3, Line 196:

OK. New test tst-initfinilazyfail. Good.

|  #	 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

 ...

| @@ -280,17 +280,18 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
|  		tst-nodelete-dlclose-dso tst-nodelete-dlclose-plugin \
|  		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-auditmanymod7 tst-auditmanymod8 tst-auditmanymod9 \
| +		tst-initlazyfailmod tst-finilazyfailmod

PS3, Line 288:

OK. Two new dsos for the test.

|  # 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

 ...

| @@ -1575,8 +1576,18 @@ # Avoid creating an ABI tag note, which may come before the
|  # artificial, large note in tst-big-note-lib.o and invalidate the
|  # test.
|  $(objpfx)tst-big-note-lib.so: $(objpfx)tst-big-note-lib.o
|  	$(LINK.o) -shared -o $@ $(LDFLAGS.so) $<
|  
|  $(objpfx)tst-unwind-ctor: $(objpfx)tst-unwind-ctor-lib.so
|  
|  CFLAGS-tst-unwind-main.c += -funwind-tables -DUSE_PTHREADS=0
| +
| +$(objpfx)tst-initfinilazyfail: $(libdl)
| +$(objpfx)tst-initfinilazyfail.out: \
| +  $(objpfx)tst-initlazyfailmod.so $(objpfx)tst-finilazyfailmod.so
| +# Override -z defs, so that we can reference an undefined symbol.
| +# Force lazy binding for the same reason.
| +LDFLAGS-tst-initlazyfailmod.so = \
| +  -Wl,-z,lazy -Wl,--unresolved-symbols=ignore-all
| +LDFLAGS-tst-finilazyfailmod.so = \
| +  -Wl,-z,lazy -Wl,--unresolved-symbols=ignore-all

PS3, Line 1593:

OK. Add dso deps and force lazy.

| --- elf/dl-close.c
| +++ elf/dl-close.c
| @@ -100,17 +100,41 @@ remove_slotinfo (size_t idx, struct dtv_slotinfo_list *listp, size_t disp,
|  	  GL(dl_tls_max_dtv_idx) = idx;
|  	  return true;
|  	}
|      }
|  
|    /* No non-entry in this list element.  */
|    return false;
|  }
|  
| +/* Invoke dstructors for CLOSURE (a struct link_map *).  Called with
| +   exception handling temporarily disabled, to make errors fatal.  */

PS3, Line 110:

OK. Good comment.

| +static void
| +call_destructors (void *closure)
| +{
| +  struct link_map *map = closure;
| +
| +  if (map->l_info[DT_FINI_ARRAY] != NULL)
| +    {
| +      ElfW(Addr) *array =
| +	(ElfW(Addr) *) (map->l_addr
| +			+ map->l_info[DT_FINI_ARRAY]->d_un.d_ptr);
| +      unsigned int sz = (map->l_info[DT_FINI_ARRAYSZ]->d_un.d_val
| +			 / sizeof (ElfW(Addr)));
| +
| +      while (sz-- > 0)
| +	((fini_t) array[sz]) ();

PS3, Line 125:

OK. Run the destructors from DT_FINI_ARRAY.

| +    }
| +
| +  /* Next try the old-style destructor.  */
| +  if (map->l_info[DT_FINI] != NULL)
| +    DL_CALL_DT_FINI (map, ((void *) map->l_addr
| +			   + map->l_info[DT_FINI]->d_un.d_ptr));

PS3, Line 131:

OK. Run the other set of desctrutors, old-style, from DT_FINI.

| +}
|  
|  void
|  _dl_close_worker (struct link_map *map, bool force)
|  {
|    /* One less direct use.  */
|    --map->l_direct_opencount;
|  
|    /* If _dl_close is called recursively (some destructor call dlclose),

 ...

| @@ -260,20 +284,20 @@ #endif
|  
|        /* 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);
|  
|  	  /* Call its termination function.  Do not do it for
| -	     half-cooked objects.  */
| +	     half-cooked objects.  Temporarily disable exception
| +	     handling, so that errors are fatal.  */

PS3, Line 295:

OK. The point here is that a failure in a destructor is past the point
of no return for unloading the objects. If we fail to unload the
objects then the entire state of the dynamic loader is in question.
There is a balance here, either this was a programming error on the
part of the user, or it was a real dynamic loader bug. The problem is
that we want the loader to be as robust as possible in these cases,
and handle all forms of partial state is actually really hard and can
lead to subtle bugs, some of which may be security bugs. I'm for the
case where we have better consistency in the loader for handling these
issues. That is to say that a failure in running destructors is fatal,
and while this is a subtle change in the failure semantics, I think
this is a good thing. We immediately fail at the first point of
failure and make this easier to diagnose. This is in line with our
usual principle of fail early and catastrophically.

|  	  if (imap->l_init_called)
|  	    {
|  	      /* When debugging print a message first.  */
|  	      if (__builtin_expect (GLRO(dl_debug_mask) & DL_DEBUG_IMPCALLS,
|  				    0))
|  		_dl_debug_printf ("\ncalling fini: %s [%lu]\n\n",
|  				  imap->l_name, nsid);
|  
| -	      if (imap->l_info[DT_FINI_ARRAY] != NULL)

 ...

| @@ -286,18 +304,12 @@ #endif
| -
| -		  while (sz-- > 0)
| -		    ((fini_t) array[sz]) ();
| -		}
| -
| -	      /* Next try the old-style destructor.  */
| -	      if (imap->l_info[DT_FINI] != NULL)
| -		DL_CALL_DT_FINI (imap, ((void *) imap->l_addr
| -			 + imap->l_info[DT_FINI]->d_un.d_ptr));
| +	      if (imap->l_info[DT_FINI_ARRAY] != NULL
| +		  || imap->l_info[DT_FINI] != NULL)
| +		_dl_catch_exception (NULL, call_destructors, imap);

PS3, Line 306:

OK. Nice refactoring into _dl_catch_exception with a NULL so all
failures are fatal.

|  	    }
|  
|  #ifdef SHARED
|  	  /* Auditing checkpoint: we remove an object.  */
|  	  if (__glibc_unlikely (do_audit))
|  	    {
|  	      struct audit_ifaces *afct = GLRO(dl_audit);
|  	      for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
|  		{
| --- elf/dl-open.c
| +++ elf/dl-open.c
| @@ -217,8 +218,24 @@ /* struct dl_init_args and call_dl_init are used to call _dl_init with
| +   exception handling disabled.  */
| +struct dl_init_args
| +{
| +  struct link_map *new;
| +  int argc;
| +  char **argv;
| +  char **env;
| +};
| +
| +static void
| +call_dl_init (void *closure)
| +{
| +  struct dl_init_args *args = closure;
| +  _dl_init (args->new, args->argc, args->argv, args->env);
| +}

PS3, Line 232:

OK. This is the same noexcept setup but for initializers.`

| +
|  static void
|  dl_open_worker (void *a)
|  {
|    struct dl_open_args *args = a;
|    const char *file = args->file;
|    int mode = args->mode;
|    struct link_map *call_map = NULL;
|  

 ...

| @@ -563,9 +580,20 @@ #endif
| +     fatal.  */
| +  {
| +    struct dl_init_args init_args =
| +      {
| +        .new = new,
| +        .argc = args->argc,
| +        .argv = args->argv,
| +        .env = args->env
| +      };
| +    _dl_catch_exception (NULL, call_dl_init, &init_args);
| +  }

PS3, Line 590:

OK. Use NULL to run with noexcept so exceptions thrown are fatal.

|  
|    /* Now we can make the new map available in the global scope.  */
|    if (mode & RTLD_GLOBAL)
|      add_to_global_update (new);
|  
|  #ifndef SHARED
|    /* We must be the static _dl_open in libc.a.  A static program that
|       has loaded a dynamic object now has competition.  */
|    __libc_multiple_libcs = 1;
| --- /dev/null
| +++ elf/tst-finilazyfailmod.c
| @@ -1,0 +17,11 @@ /* Helper module for tst-initfinilazyfail: lazy binding failure in destructor.
| +   <https://www.gnu.org/licenses/>.  */
| +
| +/* An undefined function.  Calling it will cause a lazy binding
| +   failure.  */
| +void undefined_function (void);
| +
| +static void __attribute__ ((destructor))
| +fini (void)
| +{
| +  undefined_function ();
| +}

PS3, Line 27:

OK. Clear comment. Calling an undefined function lazily which will
result in an error.

| --- /dev/null
| +++ elf/tst-initfinilazyfail.c
| @@ -1,0 +19,60 @@ /* Test that lazy binding failures in constructors and destructors are fatal.
| +#include <dlfcn.h>
| +#include <string.h>
| +#include <support/capture_subprocess.h>
| +#include <support/check.h>
| +#include <support/xdlfcn.h>
| +
| +static void
| +test_constructor (void *closure)
| +{
| +  void *handle = dlopen ("tst-initlazyfailmod.so", RTLD_LAZY);

PS3, Line 28:

OK. Previously this would cause a fatal error to be thrown in the
constructor because it had a lazy binding failure, and that would in
turn cause the dlopen to fail and return NULL, but leave the half-
constructed link map lying around. Now with your semantics changes
this is fully fatal at it really should be, similar to the situation
you get if you do not use dlopen. I think this is the right behaviour
because we're past the point of no return for cleaning up the mess
that is left because of the failure. Worst case we will need a new
flag for this if it turns out someone really really needs the ability
to load a failing DSO and leave it in a failed state (though that's
fairly dangerous).

| +  if (handle == NULL)
| +    FAIL_EXIT (2, "dlopen did not terminate the process: %s", dlerror ());
| +  else
| +    FAIL_EXIT (2, "dlopen did not terminate the process (%p)", handle);
| +}
| +
| +static void
| +test_destructor (void *closure)
| +{
| +  void *handle = xdlopen ("tst-finilazyfailmod.so", RTLD_LAZY);

PS3, Line 38:

OK. Same test but for the desctructors.

| +  int ret = dlclose (handle);
| +  const char *message = dlerror ();
| +  if (message != NULL)
| +    FAIL_EXIT (2, "dlclose did not terminate the process: %d, %s",
| +               ret, message);
| +  else
| +    FAIL_EXIT (2, "dlopen did not terminate the process: %d", ret);
| +}
| +
| +static int
| +do_test (void)
| +{
| +  {
| +    struct support_capture_subprocess proc

PS3, Line 52:

OK. Run in a subprocess for both constructor and destructor tests
because they are going to die.

| +      = support_capture_subprocess (test_constructor, NULL);
| +    support_capture_subprocess_check (&proc, "constructor", 127,
| +                                      sc_allow_stderr);
| +    printf ("info: constructor failure output: [[%s]]\n", proc.err.buffer);
| +    TEST_VERIFY (strstr (proc.err.buffer,
| +                         "tst-initfinilazyfail: symbol lookup error: ")
| +                 != NULL);
| +    TEST_VERIFY (strstr (proc.err.buffer,
| +                         "tst-initlazyfailmod.so: undefined symbol:"
| +                         " undefined_function\n") != NULL);
| +    support_capture_subprocess_free (&proc);
| +  }
| +
| +  {
| +    struct support_capture_subprocess proc
| +      = support_capture_subprocess (test_destructor, NULL);
| +    support_capture_subprocess_check (&proc, "destructor", 127,

PS3, Line 69:

OK. Same thing but with destructor test.

| +                                      sc_allow_stderr);
| +    printf ("info: destructor failure output: [[%s]]\n", proc.err.buffer);
| +    TEST_VERIFY (strstr (proc.err.buffer,
| +                         "tst-initfinilazyfail: symbol lookup error: ")
| +                 != NULL);
| +    TEST_VERIFY (strstr (proc.err.buffer,
| +                         "tst-finilazyfailmod.so: undefined symbol:"
| +                         " undefined_function\n") != NULL);
| +    support_capture_subprocess_free (&proc);
| --- /dev/null
| +++ elf/tst-initlazyfailmod.c
| @@ -1,0 +17,11 @@ /* Helper module for tst-initfinilazyfail: lazy binding failure in constructor.
| +   <https://www.gnu.org/licenses/>.  */
| +
| +/* An undefined function.  Calling it will cause a lazy binding
| +   failure.  */
| +void undefined_function (void);
| +
| +static void __attribute__ ((constructor))
| +init (void)
| +{
| +  undefined_function ();

PS3, Line 26:

OK. Same DSO but for constructor failure.

| +}
Carlos O'Donell (Code Review) Nov. 18, 2019, 5:01 p.m. UTC | #3
Florian Weimer has posted comments on this change.

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


Patch Set 3: Code-Review-2

> Please update the packaging changes here:
> https://www.sourceware.org/glibc/wiki/Release/2.31
> to indicate that the semantic change has happened and to watch out for it.
> 
> I think we should have a NEWS item for this, but you can commit that in a followup if you want.

I need to rebase the series anyway, so I can as well add a NEWS entry.
Carlos O'Donell (Code Review) Nov. 27, 2019, 3:20 p.m. UTC | #4
Florian Weimer has removed a vote from this change. ( https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/467 )


Change subject: Lazy binding failures during dlopen/dlclose must be fatal [BZ #24304]
......................................................................


Removed Code-Review-2 by Florian Weimer <fweimer@redhat.com>
Carlos O'Donell (Code Review) Nov. 27, 2019, 6:34 p.m. UTC | #5
Carlos O'Donell has posted comments on this change.

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


Patch Set 4: Code-Review+2
Carlos O'Donell (Code Review) Nov. 27, 2019, 7:04 p.m. UTC | #6
Carlos O'Donell has posted comments on this change.

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


Patch Set 5: Code-Review+2

(1 comment)

Additional news entry is OK (matches what Adhemerval is doing). Please feel free to split this out as a distinct commit if you like though. Otherwise it slows down this review. 

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

| --- NEWS
| +++ NEWS
| @@ -74,15 +74,21 @@ * The settimeofday function can still be used to set a system-wide time
|    Note that settimeofday itself is obsolescent according to POSIX.
|    Programs that set the system time should use clock_settime and/or
|    the adjtime family of functions instead.  We may also cease to make
|    settimeofday available to newly linked binaries after there is a
|    replacement for Linux's time-zone-like offset API.
|  
|  * The obsolete functions ftime has been deprecated and will be removed from
|    a future version of glibc.  Application should use clock_gettime instead.
|  
|  * The sparc*-*linux-gnu configurations targeting v7 or order architecture
|    are no longer supported.  For v8 only implementations with native CAS
|    instruction are still supported (such as LEON).

PS5, Line 85:

OK.

|  
| +* If a lazy binding failure happens during dlopen, during the execution of
| +  an ELF constructor, the process is now terminated.  Previously, the
| +  dynamic loader would return NULL from dlopen, with the lazy binding error
| +  captured in a dlerror message.  In general, this is unsafe because
| +  resetting the stack in an arbitrary function call is not possible.
| +
|  Changes to build and runtime requirements:
|

Patch
diff mbox series

diff --git a/elf/Makefile b/elf/Makefile
index 5e4cdb4..a09b650 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -193,7 +193,7 @@ 
 	 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-dlopen-self tst-initfinilazyfail
 #	 reldep9
 tests-internal += loadtest unload unload2 circleload1 \
 	 neededtest neededtest2 neededtest3 neededtest4 \
@@ -281,7 +281,8 @@ 
 		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-sonamemove-runmod1 tst-sonamemove-runmod2 \
+		tst-initlazyfailmod tst-finilazyfailmod
 # 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%,\
@@ -1569,3 +1570,13 @@ 
 $(objpfx)tst-unwind-ctor: $(objpfx)tst-unwind-ctor-lib.so
 
 CFLAGS-tst-unwind-main.c += -funwind-tables -DUSE_PTHREADS=0
+
+$(objpfx)tst-initfinilazyfail: $(libdl)
+$(objpfx)tst-initfinilazyfail.out: \
+  $(objpfx)tst-initlazyfailmod.so $(objpfx)tst-finilazyfailmod.so
+# Override -z defs, so that we can reference an undefined symbol.
+# Force lazy binding for the same reason.
+LDFLAGS-tst-initlazyfailmod.so = \
+  -Wl,-z,lazy -Wl,--unresolved-symbols=ignore-all
+LDFLAGS-tst-finilazyfailmod.so = \
+  -Wl,-z,lazy -Wl,--unresolved-symbols=ignore-all
diff --git a/elf/dl-close.c b/elf/dl-close.c
index de91c28..2e38943 100644
--- a/elf/dl-close.c
+++ b/elf/dl-close.c
@@ -106,6 +106,30 @@ 
   return false;
 }
 
+/* Invoke dstructors for CLOSURE (a struct link_map *).  Called with
+   exception handling temporarily disabled, to make errors fatal.  */
+static void
+call_destructors (void *closure)
+{
+  struct link_map *map = closure;
+
+  if (map->l_info[DT_FINI_ARRAY] != NULL)
+    {
+      ElfW(Addr) *array =
+	(ElfW(Addr) *) (map->l_addr
+			+ map->l_info[DT_FINI_ARRAY]->d_un.d_ptr);
+      unsigned int sz = (map->l_info[DT_FINI_ARRAYSZ]->d_un.d_val
+			 / sizeof (ElfW(Addr)));
+
+      while (sz-- > 0)
+	((fini_t) array[sz]) ();
+    }
+
+  /* Next try the old-style destructor.  */
+  if (map->l_info[DT_FINI] != NULL)
+    DL_CALL_DT_FINI (map, ((void *) map->l_addr
+			   + map->l_info[DT_FINI]->d_un.d_ptr));
+}
 
 void
 _dl_close_worker (struct link_map *map, bool force)
@@ -267,7 +291,8 @@ 
 		  && (imap->l_flags_1 & DF_1_NODELETE) == 0);
 
 	  /* Call its termination function.  Do not do it for
-	     half-cooked objects.  */
+	     half-cooked objects.  Temporarily disable exception
+	     handling, so that errors are fatal.  */
 	  if (imap->l_init_called)
 	    {
 	      /* When debugging print a message first.  */
@@ -276,22 +301,9 @@ 
 		_dl_debug_printf ("\ncalling fini: %s [%lu]\n\n",
 				  imap->l_name, nsid);
 
-	      if (imap->l_info[DT_FINI_ARRAY] != NULL)
-		{
-		  ElfW(Addr) *array =
-		    (ElfW(Addr) *) (imap->l_addr
-				    + imap->l_info[DT_FINI_ARRAY]->d_un.d_ptr);
-		  unsigned int sz = (imap->l_info[DT_FINI_ARRAYSZ]->d_un.d_val
-				     / sizeof (ElfW(Addr)));
-
-		  while (sz-- > 0)
-		    ((fini_t) array[sz]) ();
-		}
-
-	      /* Next try the old-style destructor.  */
-	      if (imap->l_info[DT_FINI] != NULL)
-		DL_CALL_DT_FINI (imap, ((void *) imap->l_addr
-			 + imap->l_info[DT_FINI]->d_un.d_ptr));
+	      if (imap->l_info[DT_FINI_ARRAY] != NULL
+		  || imap->l_info[DT_FINI] != NULL)
+		_dl_catch_exception (NULL, call_destructors, imap);
 	    }
 
 #ifdef SHARED
diff --git a/elf/dl-open.c b/elf/dl-open.c
index cc802fd..aad22ef 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -214,6 +214,23 @@ 
 }
 rtld_hidden_def (_dl_find_dso_for_object);
 
+/* struct dl_init_args and call_dl_init are used to call _dl_init with
+   exception handling disabled.  */
+struct dl_init_args
+{
+  struct link_map *new;
+  int argc;
+  char **argv;
+  char **env;
+};
+
+static void
+call_dl_init (void *closure)
+{
+  struct dl_init_args *args = closure;
+  _dl_init (args->new, args->argc, args->argv, args->env);
+}
+
 static void
 dl_open_worker (void *a)
 {
@@ -555,8 +572,19 @@ 
   if (mode & RTLD_GLOBAL)
     add_to_global_resize (new);
 
-  /* Run the initializer functions of new objects.  */
-  _dl_init (new, args->argc, args->argv, args->env);
+  /* Run the initializer functions of new objects.  Temporarily
+     disable the exception handler, so that lazy binding failures are
+     fatal.  */
+  {
+    struct dl_init_args init_args =
+      {
+        .new = new,
+        .argc = args->argc,
+        .argv = args->argv,
+        .env = args->env
+      };
+    _dl_catch_exception (NULL, call_dl_init, &init_args);
+  }
 
   /* Now we can make the new map available in the global scope.  */
   if (mode & RTLD_GLOBAL)
diff --git a/elf/tst-finilazyfailmod.c b/elf/tst-finilazyfailmod.c
new file mode 100644
index 0000000..2670bd1
--- /dev/null
+++ b/elf/tst-finilazyfailmod.c
@@ -0,0 +1,27 @@ 
+/* Helper module for tst-initfinilazyfail: lazy binding failure in destructor.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+/* An undefined function.  Calling it will cause a lazy binding
+   failure.  */
+void undefined_function (void);
+
+static void __attribute__ ((destructor))
+fini (void)
+{
+  undefined_function ();
+}
diff --git a/elf/tst-initfinilazyfail.c b/elf/tst-initfinilazyfail.c
new file mode 100644
index 0000000..9b4a3d0
--- /dev/null
+++ b/elf/tst-initfinilazyfail.c
@@ -0,0 +1,84 @@ 
+/* Test that lazy binding failures in constructors and destructors are fatal.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <dlfcn.h>
+#include <string.h>
+#include <support/capture_subprocess.h>
+#include <support/check.h>
+#include <support/xdlfcn.h>
+
+static void
+test_constructor (void *closure)
+{
+  void *handle = dlopen ("tst-initlazyfailmod.so", RTLD_LAZY);
+  if (handle == NULL)
+    FAIL_EXIT (2, "dlopen did not terminate the process: %s", dlerror ());
+  else
+    FAIL_EXIT (2, "dlopen did not terminate the process (%p)", handle);
+}
+
+static void
+test_destructor (void *closure)
+{
+  void *handle = xdlopen ("tst-finilazyfailmod.so", RTLD_LAZY);
+  int ret = dlclose (handle);
+  const char *message = dlerror ();
+  if (message != NULL)
+    FAIL_EXIT (2, "dlclose did not terminate the process: %d, %s",
+               ret, message);
+  else
+    FAIL_EXIT (2, "dlopen did not terminate the process: %d", ret);
+}
+
+static int
+do_test (void)
+{
+  {
+    struct support_capture_subprocess proc
+      = support_capture_subprocess (test_constructor, NULL);
+    support_capture_subprocess_check (&proc, "constructor", 127,
+                                      sc_allow_stderr);
+    printf ("info: constructor failure output: [[%s]]\n", proc.err.buffer);
+    TEST_VERIFY (strstr (proc.err.buffer,
+                         "tst-initfinilazyfail: symbol lookup error: ")
+                 != NULL);
+    TEST_VERIFY (strstr (proc.err.buffer,
+                         "tst-initlazyfailmod.so: undefined symbol:"
+                         " undefined_function\n") != NULL);
+    support_capture_subprocess_free (&proc);
+  }
+
+  {
+    struct support_capture_subprocess proc
+      = support_capture_subprocess (test_destructor, NULL);
+    support_capture_subprocess_check (&proc, "destructor", 127,
+                                      sc_allow_stderr);
+    printf ("info: destructor failure output: [[%s]]\n", proc.err.buffer);
+    TEST_VERIFY (strstr (proc.err.buffer,
+                         "tst-initfinilazyfail: symbol lookup error: ")
+                 != NULL);
+    TEST_VERIFY (strstr (proc.err.buffer,
+                         "tst-finilazyfailmod.so: undefined symbol:"
+                         " undefined_function\n") != NULL);
+    support_capture_subprocess_free (&proc);
+  }
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/elf/tst-initlazyfailmod.c b/elf/tst-initlazyfailmod.c
new file mode 100644
index 0000000..36348b5
--- /dev/null
+++ b/elf/tst-initlazyfailmod.c
@@ -0,0 +1,27 @@ 
+/* Helper module for tst-initfinilazyfail: lazy binding failure in constructor.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+/* An undefined function.  Calling it will cause a lazy binding
+   failure.  */
+void undefined_function (void);
+
+static void __attribute__ ((constructor))
+init (void)
+{
+  undefined_function ();
+}