[PATCH] pthread_once hangs when init routine throws an exception [BZ #18435]

This is another attempt at making pthread_once handle throwing exceptions
from the init routine callback.  As the new testcases show, just switching
to the cleanup attribute based cleanup does fix the tst-once5 test, but
breaks the new tst-oncey3 test.  That is because when throwing exceptions,
only the unwind info registered cleanups (i.e. C++ destructors or cleanup
attribute), when cancelling threads and there has been unwind info from the
cancellation point up to whatever needs cleanup both unwind info registered
cleanups and THREAD_SETMEM (self, cleanup, ...) registered cleanups are
invoked, but once we hit some frame with no unwind info, only the
THREAD_SETMEM (self, cleanup, ...) registered cleanups are invoked.
So, to stay fully backwards compatible (allow init routines without
unwind info which encounter cancellation points) and handle exception throwing
we actually need to register the pthread_once cleanups in both unwind info
and in the THREAD_SETMEM (self, cleanup, ...) way.
If an exception is thrown, only the former will happen and we in that case
need to also unregister the THREAD_SETMEM (self, cleanup, ...) registered
handler, because otherwise after catching the exception the user code could
call deeper into the stack some cancellation point, get cancelled and then
a stale cleanup handler would clobber stack and probably crash.
If a thread calling init routine is cancelled and unwind info ends before
the pthread_once frame, it will be cleaned up through self->cleanup as
before.  And if unwind info is present, unwind_stop first calls the
self->cleanup registered handler for the frame, then it will call the
unwind info registered handler but that will already see __do_it == 0
and do nothing.

diff --git a/nptl/Makefile b/nptl/Makefile
index 5f85dd7854e33982eb76c1088a62d11bb235a7af..33766eaf7a4630151e1e2322a852b86daddee968 100644 (file)
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -386,10 +386,6 @@ xtests += tst-eintr1
 
 test-srcs = tst-oddstacklimit
 
-# Test expected to fail on most targets (except x86_64) due to bug
-# 18435 - pthread_once hangs when init routine throws an exception.
-test-xfail-tst-once5 = yes
-
 gen-as-const-headers = unwindbuf.sym \
                       pthread-pi-defines.sym
 

diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index d2fd0826fe43825c219009b9247af5a797fb9af4..93f3cef00fc45a051d3a611112fb2b221bc2ef84 100644 (file)
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -604,6 +604,67 @@ extern void __pthread_cleanup_pop (struct _pthread_cleanup_buffer *buffer,
 # undef pthread_cleanup_pop
 # define pthread_cleanup_pop(execute)                   \
   __pthread_cleanup_pop (&_buffer, (execute)); }
+
+# if defined __EXCEPTIONS && !defined __cplusplus
+/* Structure to hold the cleanup handler information.  */
+struct __pthread_cleanup_combined_frame
+{
+  void (*__cancel_routine) (void *);
+  void *__cancel_arg;
+  int __do_it;
+  struct _pthread_cleanup_buffer __buffer;
+};
+
+/* Special cleanup macros which register cleanup both using
+   __pthread_cleanup_{push,pop} and using cleanup attribute.  This is needed
+   for pthread_once, so that it supports both throwing exceptions from the
+   pthread_once callback (only cleanup attribute works there) and cancellation
+   of the thread running the callback if the callback or some routines it
+   calls don't have unwind information.  */
+
+static __always_inline void
+__pthread_cleanup_combined_routine (struct __pthread_cleanup_combined_frame
+                                   *__frame)
+{
+  if (__frame->__do_it)
+    {
+      __frame->__cancel_routine (__frame->__cancel_arg);
+      __frame->__do_it = 0;
+      __pthread_cleanup_pop (&__frame->__buffer, 0);
+    }
+}
+
+static inline void
+__pthread_cleanup_combined_routine_voidptr (void *__arg)
+{
+  struct __pthread_cleanup_combined_frame *__frame
+    = (struct __pthread_cleanup_combined_frame *) __arg;
+  if (__frame->__do_it)
+    {
+      __frame->__cancel_routine (__frame->__cancel_arg);
+      __frame->__do_it = 0;
+    }
+}
+
+#  define pthread_cleanup_combined_push(routine, arg) \
+  do {                                                                       \
+    void (*__cancel_routine) (void *) = (routine);                           \
+    struct __pthread_cleanup_combined_frame __clframe                        \
+      __attribute__ ((__cleanup__ (__pthread_cleanup_combined_routine)))      \
+      = { .__cancel_routine = __cancel_routine, .__cancel_arg = (arg),       \
+         .__do_it = 1 };                                                     \
+    __pthread_cleanup_push (&__clframe.__buffer,                             \
+                           __pthread_cleanup_combined_routine_voidptr,       \
+                           &__clframe);
+
+#  define pthread_cleanup_combined_pop(execute) \
+    __pthread_cleanup_pop (&__clframe.__buffer, 0);                          \
+    __clframe.__do_it = 0;                                                   \
+    if (execute)                                                             \
+      __cancel_routine (__clframe.__cancel_arg);                             \
+  } while (0)
+
+# endif
 #endif
 
 extern void __pthread_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer,

diff --git a/nptl/pthread_once.c b/nptl/pthread_once.c
index 28d97097c75f992d6b7c7e00f36155e040e2dcfa..7645da222a65bc3d3bec3715ba705ccacc5ee07b 100644 (file)
--- a/nptl/pthread_once.c
+++ b/nptl/pthread_once.c
@@ -111,11 +111,11 @@ __pthread_once_slow (pthread_once_t *once_control, void (*init_routine) (void))
       /* This thread is the first here.  Do the initialization.
         Register a cleanup handler so that in case the thread gets
         interrupted the initialization can be restarted.  */
-      pthread_cleanup_push (clear_once_control, once_control);
+      pthread_cleanup_combined_push (clear_once_control, once_control);
 
       init_routine ();
 
-      pthread_cleanup_pop (0);
+      pthread_cleanup_combined_pop (0);
 
 
       /* Mark *once_control as having finished the initialization.  We need

diff --git a/nptl/tst-once5.cc b/nptl/tst-once5.cc
index b797ab35622e78eb67d6a55bf9e4ed6c0ac61e1b..60fe1ef820f3832caf53cddd31d0f6955bd5caf0 100644 (file)
--- a/nptl/tst-once5.cc
+++ b/nptl/tst-once5.cc
@@ -59,7 +59,7 @@ do_test (void)
                " throwing an exception", stderr);
     }
     catch (OnceException) {
-      if (1 < niter)
+      if (niter > 1)
         fputs ("pthread_once unexpectedly threw", stderr);
       result = 0;
     }
@@ -75,7 +75,5 @@ do_test (void)
   return result;
 }
 
-// The test currently hangs and is XFAILed.  Reduce the timeout.
-#define TIMEOUT 1
 #define TEST_FUNCTION do_test ()
 #include "../test-skeleton.c"

diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
index eeb64f9fb0480ccc9919bb9a25a789278b6fc623..53b65ef3499ef8c90014e9416469d443dad13a9b 100644 (file)
--- a/sysdeps/pthread/Makefile
+++ b/sysdeps/pthread/Makefile
@@ -231,7 +231,7 @@ generated += $(objpfx)tst-atfork2.mtrace \
 
 tests-internal += tst-cancel25 tst-robust8
 
-tests += tst-oncex3 tst-oncex4
+tests += tst-oncex3 tst-oncex4 tst-oncey3 tst-oncey4
 
 modules-names += tst-join7mod
 
@@ -242,6 +242,8 @@ endif
 
 CFLAGS-tst-oncex3.c += -fexceptions
 CFLAGS-tst-oncex4.c += -fexceptions
+CFLAGS-tst-oncey3.c += -fno-exceptions -fno-asynchronous-unwind-tables
+CFLAGS-tst-oncey4.c += -fno-exceptions -fno-asynchronous-unwind-tables
 
 $(objpfx)tst-join7: $(libdl) $(shared-thread-library)
 $(objpfx)tst-join7.out: $(objpfx)tst-join7mod.so

diff --git a/sysdeps/pthread/tst-oncey3.c b/sysdeps/pthread/tst-oncey3.c
new file mode 100644 (file)
index 0000000..08225b8
--- /dev/null
+++ b/sysdeps/pthread/tst-oncey3.c
@@ -0,0 +1 @@
+#include "tst-once3.c"

diff --git a/sysdeps/pthread/tst-oncey4.c b/sysdeps/pthread/tst-oncey4.c
new file mode 100644 (file)
index 0000000..9b4d98f
--- /dev/null
+++ b/sysdeps/pthread/tst-oncey4.c
@@ -0,0 +1 @@
+#include "tst-once4.c"