[GHC] #7997: waitForProcess and getProcessExitCode are unsafe against asynchronous exceptions

[GHC]

#7997: waitForProcess  and getProcessExitCode are unsafe against asynchronous
exceptions
----------------------------------------+-----------------------------------
Reporter:  dfranke                      |          Owner:                   
    Type:  bug                          |         Status:  new              
Priority:  normal                       |      Component:  libraries/process
 Version:  7.6.3                        |       Keywords:                   
      Os:  Unknown/Multiple             |   Architecture:  Unknown/Multiple 
 Failure:  Incorrect result at runtime  |      Blockedby:                   
Blocking:                               |        Related:                   
----------------------------------------+-----------------------------------
 In this description of the current behavior of ''waitForProcess'', assume
 for simplicity the following:

 1. The process being waited on has already terminated.

 2. Neither ''waitForProcess'' nor ''getProcessExitCode'' has previously
 been called for this ''ProcessHandle''.

 3. ''waitpid()'' returns success on the first try; it does not get
 interrupted by a signal.

 Under these assumptions, ''waitForProcess'' currently behaves as follows:

 1. It is passed a ''ProcessHandle'' named ''ph''.  ''ProcessHandle'' is
 defined like so:

 {{{
 data ProcessHandle__ = OpenHandle PHANDLE | ClosedHandle ExitCode
 newtype ProcessHandle = ProcessHandle (MVar ProcessHandle__)
 }}}

 2. It allocate a ''CInt'' using ''alloca''; the pointer to it is named
 ''pret''.

 3. It passes ''pret'' to ''c_waitForProcess''.  ''c_waitForProcess'' makes
 a system call to ''waitpid()'', and populates ''pret'' with the result.

 4. ''waitForProcess'' peeks into ''pret'' and then mutates ''ph'',
 changing it from an ''OpenHandle'' to a ''ClosedHandle''.

 There is already a comment in the source code mentioning that this
 approach is unsafe:


 {{{
         -- don't hold the MVar while we call c_waitForProcess...
         -- (XXX but there's a small race window here during which another
         -- thread could close the handle or call waitForProcess)
 }}}

 , which is correct. However, it is also unsafe against asynchronous
 exceptions, and would remain so even if the ''MVar'' were held during the
 ''c_waitForProcess'' call.  If an asynchronous exception occurs in between
 steps 3 and 4, then the system will be left in a state in which the child
 process has been successfully waited on by the OS, but the
 ''ProcessHandle'' is still in an ''OpenHandle'' state and the exit code
 has been lost. A subsequent call to ''waitForProcess'' will result in
 ''waitpid()'' unexpectedly returning ECHILD, or worse, the OS will have
 recycled the child process's PID and ''waitpid()'' will wait on the wrong
 process.

 ''getProcessExitCode'' has the same bug.

 I propose fixing this by redefining ''ProcessHandle'' like so:

 {{{
 data ProcessHandle = ProcessHandle PHANDLE (Ptr CInt) (MVar (Ptr CInt))
 }}}

 , where the second argument maybe points to the process's return code, and
 the third argument contains a pointer to a boolean flag indicating whether
 the second argument points to something meaningful.  Extend the signature
 of ''c_waitForProcess'' to take both pointers. Then, let
 ''c_waitForProcess'' provide the following contract.  If called as
 ''c_waitForProcess ph pret pflag'':

 1. ''*pflag'' is required to be false at the start of the call.

 2. If and only if the call to ''waitpid()'' returns successfully, then
 ''*pflag'' will be set to true and ''*pret'' will be populated with the
 exit code.

 Then, ''waitForProcess'' can behave as follows:

 1. The entire routine is wrapped in ''withMVar''.  Asynchronous exceptions
 are allowed to occur.

 2. If ''*pflag'' is true, then construct an ''ExitCode'' from ''*pret''
 and return it.

 3. Otherwise, call ''c_waitForProcess'' (with the same retry behavior as
 presently), and then construct and return an ''ExitCode'' after a
 successful return.

 And ''getProcessExitCode'' can:

 1. Mask exceptions.

 2. ''tryTakeMVar''. If the ''MVar'' is already held, unmask exceptions and
 return ''Nothing''.

 3. Peek at ''*pflag''.

 4. Replace the ''MVar''.

 5. Unmask exceptions.

 6. If ''*pflag'' is true, then construct and return an ''ExitCode'' from
 ''*pret''.

 7. Otherwise, return ''Nothing''.

-- 
Ticket URL: <http://hackage.haskell.org/trac/ghc/ticket/7997>
GHC <http://www.haskell.org/ghc/>
The Glasgow Haskell Compiler