Fix SIGINT and SIGQUIT handling on Linux / FreeBSD

This change changes the way the SIGINT and SIGQUIT are handled. First, it
makes sure that when the signal is not handled, we call the default signal
handler that takes care of the proper default behavior.
And second, it makes debugging experience better. Before, in the external events
handler thread we were waiting for these signals and then spawning a console event
handling thread. That caused the signals to be detected in that thread even when
running under the debugger, effectively making ctrl-c break-in unusable, since
an attempt to continue after the ctrl-c caused the process to exit.
I have modified it so that the SIGINT and SIGQUIT are now handled by regular signal
handlers and use pipe to notify the external events handler thread about the
events. That way, the debugger has a chance to swallow the SIGINT signal and work
as expected.

diff --git a/src/pal/src/exception/console.cpp b/src/pal/src/exception/console.cpp
index 821330a..95de10e 100644 (file)
--- a/src/pal/src/exception/console.cpp
+++ b/src/pal/src/exception/console.cpp
@@ -253,8 +253,9 @@ Parameters :
 
 Notes :
     Handlers are called on a last-installed, first called basis, until a
-    handler returns TRUE. If no handler returns TRUE (or no hanlder is
-    installed), the default behavior is to call TerminateProcess
+    handler returns TRUE. If no handler returns TRUE (or no handler is
+    installed), the default behavior is to call the default handler of
+    the corresponding signal.
 --*/
 void SEHHandleControlEvent(DWORD event, LPVOID eip)
 {
@@ -369,17 +370,28 @@ done:
 #endif
     if(!fHandled)
     {
+        int signalCode;
+
         if(CTRL_C_EVENT == event)
         {
             TRACE("Control-C not handled; terminating.\n");
+            signalCode = SIGINT;
         }
         else
         {
             TRACE("Control-Break not handled; terminating.\n");
+            signalCode = SIGQUIT;
         }
-        /* tested in Win32 : this is the exit code when ^C and ^Break are not 
-           handled */
-        TerminateProcess(GetCurrentProcess(),CONTROL_C_EXIT);
+        
+        // The proper behavior for unhandled SIGINT/SIGQUIT is to set the signal handler to the default one
+        // and then send the SIGINT/SIGQUIT to self and let the default handler do its work.
+        struct sigaction action;
+        action.sa_handler = SIG_DFL;
+        action.sa_flags = 0;
+        sigemptyset(&action.sa_mask);
+        sigaction(signalCode, &action, NULL);
+
+        kill(getpid(), signalCode);    
     }
 }
 

diff --git a/src/pal/src/exception/seh.cpp b/src/pal/src/exception/seh.cpp
index ab48cca..82a16c5 100644 (file)
--- a/src/pal/src/exception/seh.cpp
+++ b/src/pal/src/exception/seh.cpp
@@ -105,7 +105,12 @@ SEHInitialize (CPalThread *pthrCurrent, DWORD flags)
     }
 
 #if !HAVE_MACH_EXCEPTIONS
-    SEHInitializeSignals();
+    if (!SEHInitializeSignals())
+    {
+        ERROR("SEHInitializeSignals failed!\n");
+        SEHCleanup();
+        goto SEHInitializeExit;
+    }
 
     if (flags & PAL_INITIALIZE_SIGNAL_THREAD)
     {

diff --git a/src/pal/src/exception/signal.cpp b/src/pal/src/exception/signal.cpp
index d0259b4..af9cc47 100644 (file)
--- a/src/pal/src/exception/signal.cpp
+++ b/src/pal/src/exception/signal.cpp
@@ -63,6 +63,8 @@ static void sigfpe_handler(int code, siginfo_t *siginfo, void *context);
 static void sigsegv_handler(int code, siginfo_t *siginfo, void *context);
 static void sigtrap_handler(int code, siginfo_t *siginfo, void *context);
 static void sigbus_handler(int code, siginfo_t *siginfo, void *context);
+static void sigint_handler(int code, siginfo_t *siginfo, void *context);
+static void sigquit_handler(int code, siginfo_t *siginfo, void *context);
 #if USE_SIGNALS_FOR_THREAD_SUSPENSION
 void CorUnix::suspend_handler(int code, siginfo_t *siginfo, void *context);
 void CorUnix::resume_handler(int code, siginfo_t *siginfo, void *context);
@@ -81,12 +83,18 @@ struct sigaction g_previous_sigtrap;
 struct sigaction g_previous_sigfpe;
 struct sigaction g_previous_sigbus;
 struct sigaction g_previous_sigsegv;
+struct sigaction g_previous_sigint;
+struct sigaction g_previous_sigquit;
 
 #if USE_SIGNALS_FOR_THREAD_SUSPENSION
 struct sigaction g_previous_sigusr1;
 struct sigaction g_previous_sigusr2;
 #endif // USE_SIGNALS_FOR_THREAD_SUSPENSION
 
+// Pipe used for sending SIGINT / SIGQUIT signals notifications to a helper thread
+// that invokes the actual handler.
+int g_signalPipe[2];
+
 /* public function definitions ************************************************/
 
 /*++
@@ -98,9 +106,10 @@ Function :
 Parameters :
     None
 
-    (no return value)
+Return :
+    TRUE in case of a success, FALSE otherwise
 --*/
-void SEHInitializeSignals()
+BOOL SEHInitializeSignals()
 {
     TRACE("Initializing signal handlers\n");
 
@@ -122,6 +131,8 @@ void SEHInitializeSignals()
     handle_signal(SIGFPE, sigfpe_handler, &g_previous_sigfpe);
     handle_signal(SIGBUS, sigbus_handler, &g_previous_sigbus);
     handle_signal(SIGSEGV, sigsegv_handler, &g_previous_sigsegv);
+    handle_signal(SIGINT, sigint_handler, &g_previous_sigint);
+    handle_signal(SIGQUIT, sigquit_handler, &g_previous_sigquit);
 #if USE_SIGNALS_FOR_THREAD_SUSPENSION
     handle_signal(SIGUSR1, suspend_handler, &g_previous_sigusr1);
     handle_signal(SIGUSR2, resume_handler, &g_previous_sigusr2);
@@ -136,6 +147,10 @@ void SEHInitializeSignals()
        issued a SIGPIPE will, instead, report an error and set errno to EPIPE.
     */
     signal(SIGPIPE, SIG_IGN);
+
+    int status = pipe(g_signalPipe);
+    
+    return (status == 0);
 }
 
 /*++
@@ -165,6 +180,8 @@ void SEHCleanupSignals()
     restore_signal(SIGFPE, &g_previous_sigfpe);
     restore_signal(SIGBUS, &g_previous_sigbus);
     restore_signal(SIGSEGV, &g_previous_sigsegv);
+    restore_signal(SIGINT, &g_previous_sigint);
+    restore_signal(SIGQUIT, &g_previous_sigquit);
 }
 
 /* internal function definitions **********************************************/
@@ -401,6 +418,91 @@ static void sigtrap_handler(int code, siginfo_t *siginfo, void *context)
 
 /*++
 Function :
+    HandleExternalSignal
+
+    Handle the SIGINT and SIGQUIT signals.
+
+
+Parameters :
+    signalCode - code of the external signal
+
+    (no return value)
+--*/
+static void HandleExternalSignal(int signalCode)
+{
+    BYTE signalCodeByte = (BYTE)signalCode;
+    ssize_t writtenBytes;
+    do
+    {
+        writtenBytes = write(g_signalPipe[1], &signalCodeByte, 1);
+    }
+    while ((writtenBytes == -1) && (errno == EINTR));
+
+    if (writtenBytes == -1)
+    {
+        // Fatal error
+        abort();
+    }
+}
+
+/*++
+Function :
+    sigint_handler
+
+    handle SIGINT signal
+
+Parameters :
+    POSIX signal handler parameter list ("man sigaction" for details)
+
+    (no return value)
+--*/
+static void sigint_handler(int code, siginfo_t *siginfo, void *context)
+{
+    if (PALIsInitialized())
+    {
+        HandleExternalSignal(code);
+    }
+    else 
+    {
+        TRACE("SIGINT signal was unhandled; chaining to previous sigaction\n");
+
+        if (g_previous_sigint.sa_sigaction != NULL)
+        {
+            g_previous_sigint.sa_sigaction(code, siginfo, context);
+        }
+    }
+}
+
+/*++
+Function :
+    sigquit_handler
+
+    handle SIGQUIT signal
+
+Parameters :
+    POSIX signal handler parameter list ("man sigaction" for details)
+
+    (no return value)
+--*/
+static void sigquit_handler(int code, siginfo_t *siginfo, void *context)
+{
+    if (PALIsInitialized())
+    {
+        HandleExternalSignal(code);
+    }
+    else 
+    {
+        TRACE("SIGQUIT signal was unhandled; chaining to previous sigaction\n");
+
+        if (g_previous_sigquit.sa_sigaction != NULL)
+        {
+            g_previous_sigquit.sa_sigaction(code, siginfo, context);
+        }
+    }
+}
+
+/*++
+Function :
     sigbus_handler
 
     handle SIGBUS signal (EXCEPTION_ACCESS_VIOLATION?)
@@ -648,8 +750,6 @@ done:
     return palError;        
 }
 
-static const int c_iShutdownWaitTime = 5;
-
 static
 DWORD
 PALAPI
@@ -660,41 +760,8 @@ ExternalSignalHandlerThreadRoutine(
     DWORD dwThreadId;
     bool fContinue = TRUE;
     HANDLE hThread;
-    int iError;
-    int iSignal;
     PAL_ERROR palError = NO_ERROR;
     CPalThread *pthr = InternalGetCurrentThread();
-    sigset_t sigsetAll;
-    sigset_t sigsetWait;
-
-    //
-    // Setup our signal masks
-    //
-
-    //
-    // SIGPROF is not masked by this thread, and thus not waited for
-    // in sigwait since SIGPROF is used by the BSD thread scheduler.
-    // Masking SIGPROF in this thread leads to a significant 
-    // reduction in performance.
-    //
-
-    (void)sigfillset(&sigsetAll); 
-    (void)sigdelset(&sigsetAll, SIGPROF);
-
-    (void)sigemptyset(&sigsetWait);
-    (void)sigaddset(&sigsetWait, SIGINT);  
-    (void)sigaddset(&sigsetWait, SIGQUIT);  
-
-    //
-    // Mask off all signals for this thread
-    //
-    
-    iError = pthread_sigmask(SIG_SETMASK, &sigsetAll, NULL);
-    if (0 != iError)
-    {
-        ASSERT("pthread sigmask(sigsetAll) failed\n");
-        fContinue = FALSE;
-    }
 
     //
     // Wait for a signal to occur
@@ -702,28 +769,22 @@ ExternalSignalHandlerThreadRoutine(
 
     while (fContinue)
     {
-        iError = sigwait(&sigsetWait, &iSignal);
-        if (0 != iError)
+        BYTE signalCode;
+        ssize_t bytesRead;
+
+        do
         {
-            ASSERT("sigwait(sigsetWait, iSignal) failed\n");
-            fContinue = FALSE;
-            break;
+            bytesRead = read(g_signalPipe[0], &signalCode, 1);
         }
+        while ((bytesRead == -1) && (errno == EINTR));
 
-        //
-        // If the PAL is shutting down we want to exit after waiting
-        // a few seconds (in the hopes that the normal shutdown
-        // finishes...)
-        //
-
-        if (PALIsShuttingDown())
+        if (bytesRead == -1)
         {
-            sleep(c_iShutdownWaitTime);
-            fContinue = FALSE;
-            break;
+            // Fatal error 
+            abort();
         }
 
-        switch (iSignal)
+        switch (signalCode)
         {
             case SIGINT:
             case SIGQUIT:
@@ -748,7 +809,7 @@ ExternalSignalHandlerThreadRoutine(
                 //
 
                 PVOID pvCtrlCode = UintToPtr(
-                    SIGINT == iSignal ? CTRL_C_EVENT : CTRL_BREAK_EVENT
+                    SIGINT == signalCode ? CTRL_C_EVENT : CTRL_BREAK_EVENT
                     );
 
                 palError = InternalCreateThread(
@@ -765,6 +826,12 @@ ExternalSignalHandlerThreadRoutine(
 
                 if (NO_ERROR != palError)
                 {
+                    if (!PALIsShuttingDown())
+                    {
+                        // If PAL is not shutting down, failure to create a thread is 
+                        // a fatal error.
+                        abort();
+                    }
                     fContinue = FALSE;
                     break;
                 }
@@ -774,8 +841,8 @@ ExternalSignalHandlerThreadRoutine(
             }
 
             default:
-                ASSERT("Unexpect signal %d in signal thread\n", iSignal);
-                fContinue = FALSE;
+                ASSERT("Unexpected signal %d in signal thread\n", signalCode);
+                abort();
                 break;
         }
     }

diff --git a/src/pal/src/exception/signal.hpp b/src/pal/src/exception/signal.hpp
index 70a5145..abb293e 100644 (file)
--- a/src/pal/src/exception/signal.hpp
+++ b/src/pal/src/exception/signal.hpp
@@ -32,9 +32,10 @@ Function :
 Parameters :
     None
 
-    (no return value)
+Return :
+    TRUE in case of a success, FALSE otherwise
 --*/
-void SEHInitializeSignals();
+BOOL SEHInitializeSignals();
 
 /*++
 Function :

diff --git a/src/pal/src/thread/threadsusp.cpp b/src/pal/src/thread/threadsusp.cpp
index fa4abeb..e196654 100644 (file)
--- a/src/pal/src/thread/threadsusp.cpp
+++ b/src/pal/src/thread/threadsusp.cpp
@@ -1299,8 +1299,6 @@ CThreadSuspensionInfo::InitializeSignalSets()
     // Note that SIGPROF is used by the BSD thread scheduler and masking it caused a 
     // significant reduction in performance.
     sigaddset(&smDefaultmask, SIGHUP);  
-    sigaddset(&smDefaultmask, SIGINT);
-    sigaddset(&smDefaultmask, SIGQUIT); 
     sigaddset(&smDefaultmask, SIGABRT); 
 #ifdef SIGEMT
     sigaddset(&smDefaultmask, SIGEMT);