[review] Block signals during the initial part of dlopen
diff mbox series

Message ID gerrit.1572549639000.Iad079080ebe7442c13313ba11dc2797953faef35@gnutoolchain-gerrit.osci.io
State New
Headers show
Series
  • [review] Block signals during the initial part of dlopen
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/+/472
......................................................................

Block signals during the initial part of dlopen

Lazy binding during dlopen from a signal handler sees intermediate
dynamic linker state.  This has likely been always unsafe, but with
the new pending NODELETE state, this is clearly incorrect.  Other
threads are excluded via the loader lock, but the current thread is
not.  Blocking signals until right before ELF constructors run is
the safe thing to do.

Change-Id: Iad079080ebe7442c13313ba11dc2797953faef35
---
M elf/dl-open.c
1 file changed, 26 insertions(+), 2 deletions(-)

Comments

Carlos O'Donell (Code Review) Nov. 6, 2019, 1:45 p.m. UTC | #1
Christian Brauner has posted comments on this change.

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


Patch Set 1: Code-Review+1
Carlos O'Donell (Code Review) Nov. 6, 2019, 3:16 p.m. UTC | #2
Christian Brauner has posted comments on this change.

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


Patch Set 1: Code-Review+2
Carlos O'Donell (Code Review) Nov. 7, 2019, 11:18 a.m. UTC | #3
Christian Brauner has posted comments on this change.

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


Patch Set 1:

Just out of curiosity. It's still not possible to commit changes via Gerrit, right?
Carlos O'Donell (Code Review) Nov. 7, 2019, 11:43 a.m. UTC | #4
Florian Weimer has posted comments on this change.

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


Patch Set 1:

> Just out of curiosity. It's still not possible to commit changes via Gerrit, right?

No, it's just a demo for now, so that we can see how it feels like, both as a patch submitter and as a reviewer.
Carlos O'Donell (Code Review) Nov. 13, 2019, 1:09 p.m. UTC | #5
Florian Weimer has posted comments on this change.

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


Patch Set 2:

This change is ready for review.
Carlos O'Donell (Code Review) Nov. 15, 2019, 4:08 p.m. UTC | #6
Christian Brauner has posted comments on this change.

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


Patch Set 3: Code-Review+2

Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>

Patch
diff mbox series

diff --git a/elf/dl-open.c b/elf/dl-open.c
index 9b42f2b..7322c5d 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -34,6 +34,7 @@ 
 #include <atomic.h>
 #include <libc-internal.h>
 #include <array_length.h>
+#include <internal-signals.h>
 
 #include <dl-dst.h>
 #include <dl-prop.h>
@@ -52,6 +53,10 @@ 
   /* Namespace ID.  */
   Lmid_t nsid;
 
+  /* Original signal mask.  Used for unblocking signal handlers before
+     running ELF constructors.  */
+  sigset_t original_signal_mask;
+
   /* Original value of _ns_global_scope_pending_adds.  Set by
      dl_open_worker.  Only valid if nsid is a real namespace
      (non-negative).  */
@@ -510,12 +515,16 @@ 
   if (new == NULL)
     {
       assert (mode & RTLD_NOLOAD);
+      __libc_signal_restore_set (&args->original_signal_mask);
       return;
     }
 
   if (__glibc_unlikely (mode & __RTLD_SPROF))
-    /* This happens only if we load a DSO for 'sprof'.  */
-    return;
+    {
+      /* This happens only if we load a DSO for 'sprof'.  */
+      __libc_signal_restore_set (&args->original_signal_mask);
+      return;
+    }
 
   /* This object is directly loaded.  */
   ++new->l_direct_opencount;
@@ -551,6 +560,7 @@ 
 
       assert (_dl_debug_initialize (0, args->nsid)->r_state == RT_CONSISTENT);
 
+      __libc_signal_restore_set (&args->original_signal_mask);
       return;
     }
 
@@ -730,6 +740,10 @@ 
   if (mode & RTLD_GLOBAL)
     add_to_global_resize (new);
 
+  /* Unblock signals.  Data structures are now consistent, and
+     application code may run.  */
+  __libc_signal_restore_set (&args->original_signal_mask);
+
   /* Run the initializer functions of new objects.  Temporarily
      disable the exception handler, so that lazy binding failures are
      fatal.  */
@@ -819,6 +833,10 @@ 
   args.argv = argv;
   args.env = env;
 
+  /* Recursive lazy binding during manipulation of the dynamic loader
+     structures may result in incorrect behavior.  */
+  __libc_signal_block_all (&args.original_signal_mask);
+
   struct dl_exception exception;
   int errcode = _dl_catch_exception (&exception, dl_open_worker, &args);
 
@@ -859,10 +877,16 @@ 
 
 	  _dl_close_worker (args.map, true);
 
+	  /* Restore the signal mask.  In the success case, this
+	     happens inside dl_open_worker.  */
+	  __libc_signal_restore_set (&args.original_signal_mask);
+
 	  /* All link_map_nodelete_pending objects should have been
 	     deleted at this point, which is why it is not necessary
 	     to reset the flag here.  */
 	}
+      else
+	__libc_signal_restore_set (&args.original_signal_mask);
 
       assert (_dl_debug_initialize (0, args.nsid)->r_state == RT_CONSISTENT);