sal/osl/w32/process.cxx | 97 +++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 95 insertions(+), 2 deletions(-)
New commits:
commit 7fafd1aea08ad036ef48f415db5df93df218bf6e
Author: Chris Sherlock <chris.sherloc...@gmail.com>
Date: Sat Mar 11 14:03:38 2017 +1100
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 <c...@libreoffice.org>
Reviewed-by: Chris Sherlock <chris.sherloc...@gmail.com>
diff --git a/sal/osl/w32/process.cxx b/sal/osl/w32/process.cxx
index bdaaf11..3f0aa66 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;
}
/***************************************************************************/
_______________________________________________
Libreoffice-commits mailing list
libreoffice-comm...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
