Fix issue #4298 "SIGSEGV_libcoreclr.so!Debugger::GetArgCount"
authorMike McLaughlin <mikem@microsoft.com>
Tue, 26 Apr 2016 23:57:31 +0000 (16:57 -0700)
committerMike McLaughlin <mikem@microsoft.com>
Fri, 29 Apr 2016 19:11:25 +0000 (12:11 -0700)
The fix is to remove the call to TerminateDebugger in the EE shutdown
path. The reason was to clean up the transport pipe files but that
still happens in coreclr_uninitialize called by the host.

Also added code to clean up the transport named pipes and semaphores
on the debugger side when it detects that the target process has
terminated before it sends the ExitProcess notification.

Plumbed the cleanup call from dbi's ExitProcess code through the
native pipe line to the transport and then to pipe code.

Add PAL_CleanupTargetProcess for the "continue" named semaphore cleanup.

Found and fixed a minor race in dbgshim register runtime startup.

src/debug/debug-pal/unix/twowaypipe.cpp
src/debug/di/dbgtransportpipeline.cpp
src/debug/di/nativepipeline.h
src/debug/di/process.cpp
src/debug/inc/dbgtransportsession.h
src/debug/inc/twowaypipe.h
src/debug/shared/dbgtransportsession.cpp
src/pal/inc/pal.h
src/pal/src/thread/process.cpp
src/vm/ceemain.cpp

index 2bf919f..a2304de 100644 (file)
@@ -17,15 +17,14 @@ static const char* PipeNameFormat = "/tmp/clr-debug-pipe-%d-%llu-%s";
 
 void TwoWayPipe::GetPipeName(char *name, DWORD id, const char *suffix)
 {
-    UINT64 disambiguationKey;
-    BOOL ret = GetProcessIdDisambiguationKey(id, &disambiguationKey);
+    BOOL ret = GetProcessIdDisambiguationKey(id, &m_disambiguationKey);
 
     // If GetProcessIdDisambiguationKey failed for some reason, it should set the value 
     // to 0. We expect that anyone else making the pipe name will also fail and thus will
     // also try to use 0 as the value.
-    _ASSERTE(ret == TRUE || disambiguationKey == 0);
+    _ASSERTE(ret == TRUE || m_disambiguationKey == 0);
 
-    int chars = _snprintf(name, MaxPipeNameLength, PipeNameFormat, id, disambiguationKey, suffix);
+    int chars = _snprintf(name, MaxPipeNameLength, PipeNameFormat, id, m_disambiguationKey, suffix);
     _ASSERTE(chars > 0 && chars < MaxPipeNameLength);
 }
 
@@ -201,3 +200,12 @@ bool TwoWayPipe::Disconnect()
     m_state = NotInitialized;
     return true;
 }
+
+// Used by debugger side (RS) to cleanup the target (LS) named pipes 
+// and semaphores when the debugger detects the debuggee process  exited.
+void TwoWayPipe::CleanupTargetProcess()
+{
+    unlink(m_inPipeName);
+    unlink(m_outPipeName);
+    PAL_CleanupTargetProcess(m_id, m_disambiguationKey);
+}
index c117ed2..e3a3a8a 100644 (file)
@@ -111,6 +111,13 @@ public:
     // Terminate the debuggee process.
     virtual BOOL TerminateProcess(UINT32 exitCode);
 
+#ifdef FEATURE_PAL
+    virtual void CleanupTargetProcess()
+    {
+        m_pTransport->CleanupTargetProcess();
+    }
+#endif
+
 private:
     // Return TRUE if the transport is up and runnning
     BOOL IsTransportRunning()
index dd6f9ba..c9560bf 100644 (file)
@@ -167,6 +167,14 @@ public:
     {
         return S_FALSE;
     } 
+
+#ifdef FEATURE_PAL
+    // Used by debugger side (RS) to cleanup the target (LS) named pipes 
+    // and semaphores when the debugger detects the debuggee process  exited.
+    virtual void CleanupTargetProcess()
+    {
+    }
+#endif
 };
 
 //
index 572eef6..ef7ede6 100644 (file)
@@ -14415,14 +14415,13 @@ void ExitProcessWorkItem::Do()
         PUBLIC_CALLBACK_IN_THIS_SCOPE0_NO_LOCK(GetProcess());
         pCordb->m_managedCallback->ExitProcess(GetProcess());
     }
+
     // This CordbProcess object now has no reservations against a client calling ICorDebug::Terminate.
     // That call may race against the CordbProcess::Neuter below, but since we already neutered the children,
     // that neuter call will not do anything interesting that will conflict with Terminate.
     
-    
     LOG((LF_CORDB, LL_INFO1000,"W32ET::EP: returned from ExitProcess callback\n"));
 
-
     {
         RSLockHolder ch(GetProcess()->GetStopGoLock());
 
@@ -14575,6 +14574,10 @@ void CordbWin32EventThread::ExitProcess(bool fDetach)
     // and dispatch it inband w/the other callbacks.
     if (!fDetach)
     {
+#ifdef FEATURE_PAL
+        // Cleanup the transport pipe and semaphore files that might be left by the target (LS) process.
+        m_pNativePipeline->CleanupTargetProcess();
+#endif
         ExitProcessWorkItem * pItem = new (nothrow) ExitProcessWorkItem(m_pProcess);
         if (pItem != NULL)
         {
index 13fd2f6..5187202 100644 (file)
@@ -319,7 +319,7 @@ public:
     // may be delivered once the session is established.
 #ifdef RIGHT_SIDE_COMPILE
     HRESULT Init(DWORD pid, HANDLE hProcessExited);
-#else // RIGHT_SIDE_COMPILE
+#else
     HRESULT Init(DebuggerIPCControlBlock * pDCB, AppDomainEnumerationIPCBlock * pADB);
 #endif // RIGHT_SIDE_COMPILE
 
@@ -331,10 +331,16 @@ public:
     // Init() may be called again to start over from the beginning).
     void Shutdown();
 
+#ifdef RIGHT_SIDE_COMPILE
+    // Used by debugger side (RS) to cleanup the target (LS) named pipes 
+    // and semaphores when the debugger detects the debuggee process  exited.
+    void CleanupTargetProcess();
+#else
     // Cleans up the named pipe connection so no tmp files are left behind. Does only
     // the minimum and must be safe to call at any time. Called during PAL ExitProcess,
     // TerminateProcess and for unhandled native exceptions and asserts.
     void AbortConnection();
+#endif // RIGHT_SIDE_COMPILE
 
     LONG AddRef()
     {
index 6c29034..402ecea 100644 (file)
@@ -73,6 +73,10 @@ public:
         return m_state;
     }
 
+    // Used by debugger side (RS) to cleanup the target (LS) named pipes 
+    // and semaphores when the debugger detects the debuggee process  exited.
+    void CleanupTargetProcess();
+
 private:
 
     State m_state;
@@ -82,12 +86,13 @@ private:
 
     static const int MaxPipeNameLength = 64;
 
-    static void GetPipeName(char *name, DWORD id, const char *suffix);
+    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
+    int m_id;                               // id that was passed to CreateServer() or Connect()
+    int m_inboundPipe, m_outboundPipe;      // two one sided pipes used for communication
+    UINT64 m_disambiguationKey;             // key to make the names more unique
+    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.
index e382773..078a7ef 100644 (file)
@@ -214,6 +214,8 @@ void DbgTransportSession::Shutdown()
     Release();
 }
 
+#ifndef RIGHT_SIDE_COMPILE
+
 // Cleans up the named pipe connection so no tmp files are left behind. Does only
 // the minimum and must be safe to call at any time. Called during PAL ExitProcess,
 // TerminateProcess and for unhandled native exceptions and asserts.
@@ -222,7 +224,6 @@ void DbgTransportSession::AbortConnection()
     m_pipe.Disconnect();
 }
 
-#ifndef RIGHT_SIDE_COMPILE
 // API used only by the LS to drive the transport into a state where it won't accept connections. This is used
 // when no proxy is detected at startup but it's too late to shutdown all of the debugging system easily. It's
 // mainly paranoia to increase the protection of your system when the proxy isn't started.
@@ -233,9 +234,16 @@ void DbgTransportSession::Neuter()
     // AV on a deallocated handle, which might happen if we simply called Shutdown()).
     m_eState = SS_Closed;
 }
-#endif // !RIGHT_SIDE_COMPILE
 
-#ifdef RIGHT_SIDE_COMPILE
+#else // RIGHT_SIDE_COMPILE
+
+// Used by debugger side (RS) to cleanup the target (LS) named pipes 
+// and semaphores when the debugger detects the debuggee process  exited.
+void DbgTransportSession::CleanupTargetProcess()
+{
+    m_pipe.CleanupTargetProcess();
+}
+
 // On the RS it may be useful to wait and see if the session can reach the SS_Open state. If the target
 // runtime has terminated for some reason then we'll never reach the open state. So the method below gives the
 // RS a way to try and establish a connection for a reasonable amount of time and to time out otherwise. They
index 749f2b9..7038d95 100644 (file)
@@ -582,6 +582,13 @@ PALAPI
 PAL_NotifyRuntimeStarted();
 
 PALIMPORT
+VOID
+PALAPI
+PAL_CleanupTargetProcess(
+    IN int pid, 
+    IN UINT64 disambiguationKey);
+
+PALIMPORT
 void
 PALAPI
 PAL_InitializeDebug(
index 95e4659..f708b88 100644 (file)
@@ -1577,40 +1577,43 @@ public:
     {
         PAL_ERROR pe = NO_ERROR;
 
-        // Enumerate all the modules in the process and invoke the callback 
-        // for the coreclr module if found.
-        DWORD count;
-        ProcessModules *listHead = CreateProcessModules(m_processId, &count);
-        if (listHead == NULL)
+        if (!m_canceled)
         {
-            TRACE("CreateProcessModules failed for pid %d\n", m_processId);
-            pe = ERROR_INVALID_PARAMETER;
-            goto exit;
-        }
+            // Enumerate all the modules in the process and invoke the callback 
+            // for the coreclr module if found.
+            DWORD count;
+            ProcessModules *listHead = CreateProcessModules(m_processId, &count);
+            if (listHead == NULL)
+            {
+                TRACE("CreateProcessModules failed for pid %d\n", m_processId);
+                pe = ERROR_INVALID_PARAMETER;
+                goto exit;
+            }
 
-        for (ProcessModules *entry = listHead; entry != NULL; entry = entry->Next)
-        {
-            if (IsCoreClrModule(entry->Name))
+            for (ProcessModules *entry = listHead; entry != NULL; entry = entry->Next)
             {
-                PAL_CPP_TRY
+                if (IsCoreClrModule(entry->Name))
                 {
-                    TRACE("InvokeStartupCallback executing callback %p %s\n", entry->BaseAddress, entry->Name);
-                    m_callback(entry->Name, entry->BaseAddress, m_parameter);
-                }
-                PAL_CPP_CATCH_ALL
-                {
-                }
-                PAL_CPP_ENDTRY
+                    PAL_CPP_TRY
+                    {
+                        TRACE("InvokeStartupCallback executing callback %p %s\n", entry->BaseAddress, entry->Name);
+                        m_callback(entry->Name, entry->BaseAddress, m_parameter);
+                    }
+                        PAL_CPP_CATCH_ALL
+                    {
+                    }
+                    PAL_CPP_ENDTRY
 
-                // Currently only the first coreclr module in a process is supported
-                break;
+                        // Currently only the first coreclr module in a process is supported
+                        break;
+                }
             }
-        }
 
-    exit:
-        if (listHead != NULL)
-        {
-            DestroyProcessModules(listHead);
+        exit:
+            if (listHead != NULL)
+            {
+                DestroyProcessModules(listHead);
+            }
         }
 
         return pe;
@@ -1642,20 +1645,17 @@ public:
             // Wait until the coreclr runtime (debuggee) starts up
             if (sem_wait(m_startupSem) == 0)
             {
-                if (!m_canceled)
+                // The continue semaphore should exists now and is needed to wake up the runtimes below
+                continueSem = sem_open(continueSemName, O_RDWR);
+                if (continueSem != SEM_FAILED) 
                 {
-                    // The continue semaphore should exists now and is needed to wake up the runtimes below
-                    continueSem = sem_open(continueSemName, O_RDWR);
-                    if (continueSem != SEM_FAILED) 
-                    {
-                        TRACE("StartupHelperThread continue sem exists after wait - invoking callback\n");
-                        pe = InvokeStartupCallback();
-                    }
-                    else
-                    {
-                        TRACE("sem_open(continue) failed: errno is %d (%s)\n", errno, strerror(errno));
-                        pe = GetSemError();
-                    }
+                    TRACE("StartupHelperThread continue sem exists after wait - invoking callback\n");
+                    pe = InvokeStartupCallback();
+                }
+                else
+                {
+                    TRACE("sem_open(continue) failed: errno is %d (%s)\n", errno, strerror(errno));
+                    pe = GetSemError();
                 }
             }
             else 
@@ -1858,6 +1858,37 @@ exit:
 }
 
 /*++
+    PAL_CleanupTargetProcess
+
+    Cleanup the target process's name continue semaphore
+    on the debugger side when the debugger detects the
+    process termination.
+
+Parameters:
+    pid - process id
+    disambiguationKey - key to make process id unique
+
+Return value:
+    None
+--*/
+VOID
+PALAPI
+PAL_CleanupTargetProcess(
+    IN int pid,
+    IN UINT64 disambiguationKey)
+{
+    char continueSemName[NAME_MAX - 4];
+
+    sprintf_s(continueSemName,
+              sizeof(continueSemName),
+              RuntimeContinueSemaphoreName,
+              pid,
+              disambiguationKey);
+
+    sem_unlink(continueSemName);
+}
+
+/*++
  Function:
   GetProcessIdDisambiguationKey
 
index ab5e340..cc2e438 100644 (file)
@@ -1965,12 +1965,6 @@ void STDMETHODCALLTYPE EEShutDownHelper(BOOL fIsDllUnloading)
         
         FastInterlockExchange((LONG*)&g_fForbidEnterEE, TRUE);
 
-#if defined(DEBUGGING_SUPPORTED) && defined(FEATURE_PAL)
-        // Terminate the debugging services in the first phase for PAL based platforms
-        // because EEDllMain's DLL_PROCESS_DETACH is NOT going to be called.
-        TerminateDebugger();
-#endif // DEBUGGING_SUPPORTED && FEATURE_PAL
-
         if (g_fProcessDetach)
         {
             ThreadStore::TrapReturningThreads(TRUE);