tdf#106489 - Win32 version of osl_terminateProcess not "safe"

The current Win32 implementation of osl_terminateProcess calls directly
upon the Win32 TerminateProcess() API function. When I looked up this
function on MSDN I was a bit surprised to note that this is an
asynchronous function that returns immediately before the process
actually fully terminates, and does not necessarily succeed cleanly.

As we are not checking that the process actually fully terminates,
this is a bit of a problem, albeit one I think may well be somewhat
obscure and non-obvious. In fact, it is better that we call on
TerminateProcess() and use WaitForSingleObject() until the process
actually fully terminates. If the termination ends abnormally, then we
should pass a more appropriate exit status.

Dr Dobbs magazine, bless its ancient archive, details a "safe" version
of TerminateProcess(), which has been implemented it seems in dozens of
projects already so it's basically at this point a pattern. The article
can be found here:

http://www.drdobbs.com/a-safer-alternative-to-terminateprocess/184416547

Thankfully, the flaws they point out are only valid due to the age of
the article as the Windows 9x line of operating systems was still a
thing way back then :-)

Changes that are made:

1. If the PID is 0x0 then this can't be ended, so return an invalid
status

2. We need to elevate access to the process handle to allow it to
create a new thread in a potentially remote process, so we use
DuplicateHandle to do so

3. The core change is to change from TerminateProcess() to
ExitProcess(), I'm letting it timeout forever and the end user can
kill off the process via a Windows process management tool if
they need it killed entirely, hence WaitForSingleObject() has an
infinite timeout.

This way, the termination handler is now called and the attached
dlls are notified that they are being detached from the process.

Change-Id: Icfa7c60c35a088b8f9834351bc7953a910247fb8
Reviewed-on: https://gerrit.libreoffice.org/35071
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Chris Sherlock <chris.sherlock79@gmail.com>

1 files changed, 95 insertions, 2 deletions

diff --git a/sal/osl/w32/process.cxx b/sal/osl/w32/process.cxx
index bdaaf1156d59..3f0aa66a5724 100644
--- a/sal/osl/w32/process.cxx
+++ b/sal/osl/w32/process.cxx
@@ -55,10 +55,103 @@ oslProcessError SAL_CALL osl_terminateProcess(oslProcess Process)
     if (Process == nullptr)
         return osl_Process_E_Unknown;
 
-    if (TerminateProcess(static_cast<oslProcessImpl*>(Process)->m_hProcess, 0))
+    HANDLE hProcess = static_cast<oslProcessImpl*>(Process)->m_hProcess;
+    DWORD dwPID = GetProcessId(hProcess);
+
+    // cannot be System Process (0x00000000)
+    if (dwPID == 0x0)
+        return osl_Process_E_InvalidError;
+
+    // Test to see if we can create a thread in a process... adapted from:
+    // * https://support.microsoft.com/en-us/help/178893/how-to-terminate-an-application-cleanly-in-win32
+    // * http://www.drdobbs.com/a-safer-alternative-to-terminateprocess/184416547
+
+    // TODO: we really should firstly check to see if we have access to create threads and only
+    // duplicate the handle with elevated access if we don't have access... this can be done, but
+    // it's not exactly easy - an example can be found here:
+    // http://windowsitpro.com/site-files/windowsitpro.com/files/archive/windowsitpro.com/content/content/15989/listing_01.txt
+
+    HANDLE hDupProcess = NULL;
+
+
+    // we need to make sure we can create a thread in the remote process, if the handle was created
+    // in something that doesn't give us appropriate levels of access then we will need to give it the
+    // desired level of access - if the process handle was grabbed from OpenProcess it's quite possible
+    // that the handle doesn't have the appropriate level of access...
+
+    // see https://msdn.microsoft.com/en-au/library/windows/desktop/ms684880(v=vs.85).aspx
+    DWORD dwAccessFlags = (PROCESS_CREATE_THREAD | PROCESS_QUERY_INFORMATION | PROCESS_VM_OPERATION
+                                    | PROCESS_VM_WRITE | PROCESS_VM_READ);
+
+    BOOL bHaveDuplHdl = DuplicateHandle(GetCurrentProcess(),    // handle to process that has handle
+                                    hProcess,                   // handle to be duplicated
+                                    GetCurrentProcess(),        // process that will get the dup handle
+                                    &hDupProcess,               // store duplicate process handle here
+                                    dwAccessFlags,              // desired access
+                                    FALSE,                      // handle can't be inherited
+                                    0);                         // zero means no additional action needed
+
+    if (bHaveDuplHdl)
+        hProcess = hDupProcess;     // so we were able to duplicate the handle, all good...
+    else
+        SAL_WARN("sal.osl", "Could not duplicate process handle, let's hope for the best...");
+
+    DWORD dwProcessStatus = 0;
+    HANDLE hRemoteThread = NULL;
+
+    if (GetExitCodeProcess(hProcess, &dwProcessStatus) && (dwProcessStatus == STILL_ACTIVE))
+    {
+        // We need to get the address of the Win32 procedure ExitProcess, can't call it
+        // directly because we'll be calling the thunk and that will probably lead to an
+        // access violation. Once we have the address, then we need to create a new
+        // thread in the process (which we might need to run in the address space of
+        // another process) and then call on ExitProcess to try to cleanly terminate that
+        // process
+
+        DWORD dwTID = 0;    // dummy variable as we don't need to track the thread ID
+        UINT uExitCode = 0; // dummy variable... ExitProcess has no return value
+
+        // Note: we want to call on ExitProcess() and not TerminateProcess() - this is
+        // because with ExitProcess() Windows notifies all attached dlls that the process
+        // is detaching from the dll, but TerminateProcess() terminates all threads
+        // immediately, doesn't call any termination handlers and doesn't notify any dlls
+        // that it is detaching from them
+
+        HINSTANCE hKernel = GetModuleHandleA("kernel32.dll");
+        FARPROC pfnExitProc = GetProcAddress(hKernel, "ExitProcess");
+        hRemoteThread = CreateRemoteThread(
+                            hProcess,           /* process handle */
+                            NULL,               /* default security descriptor */
+                            0,                  /* initial size of stack in bytes is default
+                                                   size for executable */
+                            (LPTHREAD_START_ROUTINE)pfnExitProc, /* Win32 ExitProcess() */
+                            (PVOID)uExitCode,   /* ExitProcess() dummy return... */
+                            0,                  /* value of 0 tells thread to run immediately
+                                                   after creation */
+                            &dwTID);            /* new remote thread's identifier */
+
+    }
+
+    bool bHasExited = false;
+
+    if (hRemoteThread)
+    {
+        WaitForSingleObject(hProcess, INFINITE); // wait for process to terminate, never stop waiting...
+        CloseHandle(hRemoteThread);              // close the thread handle to allow the process to exit
+        bHasExited = true;
+    }
+
+    // need to close this duplicated process handle...
+    if (bHaveDuplHdl)
+        CloseHandle(hProcess);
+
+    if (bHasExited)
         return osl_Process_E_None;
 
-    return osl_Process_E_Unknown;
+    // fallback - given that we we wait for an infinite time on WaitForSingleObject, this should
+    // never occur... unless CreateRemoteThread failed
+    SAL_WARN("sal.osl", "TerminateProcess(hProcess, 0) called - we should never get here!");
+    return (TerminateProcess(hProcess, 0) == FALSE) ? osl_Process_E_Unknown : osl_Process_E_None;
 }
 
 /***************************************************************************/