Bug 106489

Summary:	Win32 version of osl_terminateProcess not "safe"
Product:	LibreOffice	Reporter:	Chris Sherlock <chris.sherlock79>
Component:	framework	Assignee:	Chris Sherlock <chris.sherlock79>
Status:	RESOLVED FIXED
Severity:	normal	CC:	chris.sherlock79, sberg.fun, thomas.lendo, xiscofauli
Priority:	medium
Version:	Inherited From OOo
Hardware:	All
OS:	All
See Also:	https://bugs.documentfoundation.org/show_bug.cgi?id=97359
Whiteboard:	target:5.4.0
Crash report or crash signature:		Regression By:

Description Chris Sherlock 2017-03-11 00:17:10 UTC

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 I 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 hack then :-)

Comment 1 Chris Sherlock 2017-03-11 00:18:16 UTC

I will fix this and submit a commit to gerrit for review.

Comment 2 Chris Sherlock 2017-03-11 05:53:00 UTC

I have submitted a code change to gerrit. Can a Windows developer please have a look at this? Seems to pass all the OSL process tests...

https://gerrit.libreoffice.org/#/c/35071/

Comment 3 Stephan Bergmann 2017-03-13 16:39:25 UTC

What is the perceived problem here?  Looking at the Unix implementation of osl_terminateProcess (sal/osl/unx/process.cxx), it appears that osl_terminateProcess isn't intended to wait for the given process to actually terminate (use osl_joinProcess for that).

Comment 4 Commit Notification 2017-03-15 07:12:02 UTC

Chris Sherlock committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=7fafd1aea08ad036ef48f415db5df93df218bf6e

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

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.

Comment 5 Xisco Faulí 2017-06-09 10:05:06 UTC

Hello,
Is this bug fixed?
If so, could you please close it as RESOLVED FIXED?

Comment 6 Chris Sherlock 2017-06-25 07:26:20 UTC

This is now resolved. Changes that were 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.