Fix possibility of deadlock in the SIGSEGV handler

The SIGSEGV handler was transitively calling pthread_attr_get_np to
get current stack limit no matter where the exception happened. The
problem was that if the segmentation violation happened in a C
runtime function, this could lead to deadlock since pthread_attr_get_np
is not signal safe.

The fix is to add callback that will figure out if the failing instruction
was in managed code or in one of the jit helpers and if not, it would
not attempt to handle the signal.

I have also removed non signal safe calls from the signal handlers code
path where we don't know yet whether the interrupted code was in a function
that could collide with a function that we call from the signal handler
or not. This includes TRACE and ASSERT calls that were calling string
formatting functions.

2 files changed, 29 insertions, 32 deletions

diff --git a/src/debug/debug-pal/unix/twowaypipe.cpp b/src/debug/debug-pal/unix/twowaypipe.cpp
index 3eeb15bb78..2bf919feeb 100644
--- a/src/debug/debug-pal/unix/twowaypipe.cpp
+++ b/src/debug/debug-pal/unix/twowaypipe.cpp
@@ -15,7 +15,7 @@
 
 static const char* PipeNameFormat = "/tmp/clr-debug-pipe-%d-%llu-%s";
 
-static void GetPipeName(char *name, DWORD id, const char *suffix)
+void TwoWayPipe::GetPipeName(char *name, DWORD id, const char *suffix)
 {
     UINT64 disambiguationKey;
     BOOL ret = GetProcessIdDisambiguationKey(id, &disambiguationKey);
@@ -25,8 +25,8 @@ static void GetPipeName(char *name, DWORD id, const char *suffix)
     // also try to use 0 as the value.
     _ASSERTE(ret == TRUE || disambiguationKey == 0);
 
-    int chars = _snprintf(name, PATH_MAX, PipeNameFormat, id, disambiguationKey, suffix);
-    _ASSERTE(chars > 0 && chars < PATH_MAX);
+    int chars = _snprintf(name, MaxPipeNameLength, PipeNameFormat, id, disambiguationKey, suffix);
+    _ASSERTE(chars > 0 && chars < MaxPipeNameLength);
 }
 
 // Creates a server side of the pipe. 
@@ -39,19 +39,17 @@ bool TwoWayPipe::CreateServer(DWORD id)
         return false;
 
     m_id = id;
-    char inPipeName[PATH_MAX];
-    char outPipeName[PATH_MAX];
-    GetPipeName(inPipeName, id, "in");
-    GetPipeName(outPipeName, id, "out");
+    GetPipeName(m_inPipeName, id, "in");
+    GetPipeName(m_outPipeName, id, "out");
 
-    if (mkfifo(inPipeName, S_IRWXU) == -1)
+    if (mkfifo(m_inPipeName, S_IRWXU) == -1)
     {
         return false;
     }
 
-    if (mkfifo(outPipeName, S_IRWXU) == -1)
+    if (mkfifo(m_outPipeName, S_IRWXU) == -1)
     {
-        unlink(inPipeName);
+        unlink(m_inPipeName);
         return false;
     }
 
@@ -69,21 +67,19 @@ bool TwoWayPipe::Connect(DWORD id)
         return false;
 
     m_id = id;
-    char inPipeName[PATH_MAX];
-    char outPipeName[PATH_MAX];
     //"in" and "out" are switched deliberately, because we're on the client
-    GetPipeName(inPipeName, id, "out");
-    GetPipeName(outPipeName, id, "in");
+    GetPipeName(m_inPipeName, id, "out");
+    GetPipeName(m_outPipeName, id, "in");
 
     // Pipe opening order is reversed compared to WaitForConnection()
     // in order to avaid deadlock.
-    m_outboundPipe = open(outPipeName, O_WRONLY);
+    m_outboundPipe = open(m_outPipeName, O_WRONLY);
     if (m_outboundPipe == INVALID_PIPE)
     {
         return false;
     }
 
-    m_inboundPipe = open(inPipeName, O_RDONLY);
+    m_inboundPipe = open(m_inPipeName, O_RDONLY);
     if (m_inboundPipe == INVALID_PIPE)
     {
         close(m_outboundPipe);
@@ -104,18 +100,13 @@ bool TwoWayPipe::WaitForConnection()
     if (m_state != Created)
         return false;
 
-    char inPipeName[PATH_MAX];
-    char outPipeName[PATH_MAX];
-    GetPipeName(inPipeName, m_id, "in");
-    GetPipeName(outPipeName, m_id, "out");
-
-    m_inboundPipe = open(inPipeName, O_RDONLY);
+    m_inboundPipe = open(m_inPipeName, O_RDONLY);
     if (m_inboundPipe == INVALID_PIPE)
     {
         return false;
     }
 
-    m_outboundPipe = open(outPipeName, O_WRONLY);
+    m_outboundPipe = open(m_outPipeName, O_WRONLY);
     if (m_outboundPipe == INVALID_PIPE)
     {
         close(m_inboundPipe);
@@ -185,6 +176,10 @@ int TwoWayPipe::Write(const void *data, DWORD dataSize)
 // true - success, false - failure (use GetLastError() for more details)
 bool TwoWayPipe::Disconnect()
 {
+    // IMPORTANT NOTE: This function must not call any signal unsafe functions
+    // since it is called from signal handlers.
+    // That includes ASSERT and TRACE macros.
+
     if (m_outboundPipe != INVALID_PIPE && m_outboundPipe != 0)
     {
         close(m_outboundPipe);
@@ -199,13 +194,8 @@ bool TwoWayPipe::Disconnect()
 
     if (m_state == ServerConnected || m_state == Created)
     {
-        char inPipeName[PATH_MAX];
-        GetPipeName(inPipeName, m_id, "in");
-        unlink(inPipeName);
-
-        char outPipeName[PATH_MAX];
-        GetPipeName(outPipeName, m_id, "out");
-        unlink(outPipeName);
+        unlink(m_inPipeName);
+        unlink(m_outPipeName);
     }
 
     m_state = NotInitialized;
diff --git a/src/debug/inc/twowaypipe.h b/src/debug/inc/twowaypipe.h
index 9ec38452c1..6c2903440f 100644
--- a/src/debug/inc/twowaypipe.h
+++ b/src/debug/inc/twowaypipe.h
@@ -79,8 +79,15 @@ private:
 
 
 #ifdef FEATURE_PAL
-    int m_id;                          //id that was passed to CreateServer() or Connect()
-    int m_inboundPipe, m_outboundPipe; //two one sided pipes used for communication
+
+    static const int MaxPipeNameLength = 64;
+
+    static void GetPipeName(char *name, DWORD id, const char *suffix);
+
+    int m_id;                              //id that was passed to CreateServer() or Connect()
+    int m_inboundPipe, m_outboundPipe;     //two one sided pipes used for communication
+    char m_inPipeName[MaxPipeNameLength];  //filename of the inbound pipe
+    char m_outPipeName[MaxPipeNameLength]; //filename of the outbound pipe
 
 #else
     // Connects to a one sided pipe previously created by CreateOneWayPipe.