[PATCH, libiberty] Pex pipes block under win32 when parent process dies

[PATCH, libiberty] Pex pipes block under win32 when parent process dies

[Julian Brown]

[-- Attachment #1: Type: text/plain, Size: 1775 bytes --]

Hi,

This patch addresses a problem where child processes started using the
libiberty 'pex' mechanism could block indefinitely when trying to write
to the parent if the parent exits unexpectedly. We (at CodeSourcery)
observed this happening in gdb when starting a debug stub like so:

  (gdb) target remote | /path/to/stub ...

though it's possible it could occur under other circumstances too
(though I'm not sure which). In the above case, if gdb is killed using
e.g. the task manager in Windows, the stub (which tries to output to
stderr then fflush()) will block and fail to exit.

The rationale behind the patch is borrowed from the MSDN page:

  http://msdn.microsoft.com/en-us/library/aa298531.aspx

IIUC, at the moment, the child is spawned and inherits open descriptors
for both ends of the pipe. Under Win32, you don't get EOF on a pipe
until all of the copies of the write end of the pipe are closed, so
that won't happen if the parent alone dies.

The change in pex_win32_pipe() means that the child will get neither
end of the pipe by default. So we duplicate the in/out/error
descriptors in pex_win32_exec_child() to get inheritable versions to
pass to the child, then close the originals. After spawning, we close
the duplicates. Then since the parent ends up with no copies of the
write end of the pipe, if it exits it won't cause the child to block.

This seems to suffice to fix the problem described above. OK to apply
to the Sourceware/GCC copies of libiberty?

Cheers,

Julian

ChangeLog

    libiberty/
    * pex-win32.c (pex_win32_pipe): Add _O_NOINHERIT.    
    (pex_win32_exec_child): Ensure each process has only one handle
    open on pipe endpoints. Close standard input after creating child
    for symmetry with standard output/standard error.

[-- Attachment #2: pex-no-inherit-2.diff --]
[-- Type: text/x-patch, Size: 2363 bytes --]

Index: libiberty/pex-win32.c
===================================================================
--- libiberty/pex-win32.c	(revision 246292)
+++ libiberty/pex-win32.c	(working copy)
@@ -730,7 +730,7 @@ spawn_script (const char *executable, ch
 /* Execute a child.  */
 
 static pid_t
-pex_win32_exec_child (struct pex_obj *obj ATTRIBUTE_UNUSED, int flags,
+pex_win32_exec_child (struct pex_obj *obj, int flags,
 		      const char *executable, char * const * argv,
                       char* const* env,
 		      int in, int out, int errdes,
@@ -746,6 +746,25 @@ pex_win32_exec_child (struct pex_obj *ob
   OSVERSIONINFO version_info;
   STARTUPINFO si;
   PROCESS_INFORMATION pi;
+  int orig_out, orig_in, orig_err;
+  BOOL separate_stderr = !(flags & PEX_STDERR_TO_STDOUT);
+
+  /* Ensure we have inheritable descriptors to pass to the child, and close the
+     original descriptors.  */
+  orig_in = in;
+  in = _dup (orig_in);
+  obj->funcs->close (obj, orig_in);
+  
+  orig_out = out;
+  out = _dup (orig_out);
+  obj->funcs->close (obj, orig_out);
+  
+  if (separate_stderr)
+    {
+      orig_err = errdes;
+      errdes = _dup (orig_err);
+      obj->funcs->close (obj, orig_err);
+    }
 
   stdin_handle = INVALID_HANDLE_VALUE;
   stdout_handle = INVALID_HANDLE_VALUE;
@@ -753,7 +772,7 @@ pex_win32_exec_child (struct pex_obj *ob
 
   stdin_handle = (HANDLE) _get_osfhandle (in);
   stdout_handle = (HANDLE) _get_osfhandle (out);
-  if (!(flags & PEX_STDERR_TO_STDOUT))
+  if (separate_stderr)
     stderr_handle = (HANDLE) _get_osfhandle (errdes);
   else
     stderr_handle = stdout_handle;
@@ -822,8 +841,11 @@ pex_win32_exec_child (struct pex_obj *ob
       *errmsg = "CreateProcess";
     }
 
-  /* Close the standard output and standard error handles in the
-     parent.  */ 
+  /* Close the standard input, standard output and standard error handles
+     in the parent.  */ 
+
+  if (in != STDIN_FILENO)
+    obj->funcs->close (obj, in);
   if (out != STDOUT_FILENO)
     obj->funcs->close (obj, out);
   if (errdes != STDERR_FILENO)
@@ -883,7 +905,7 @@ static int
 pex_win32_pipe (struct pex_obj *obj ATTRIBUTE_UNUSED, int *p,
 		int binary)
 {
-  return _pipe (p, 256, binary ? _O_BINARY : _O_TEXT);
+  return _pipe (p, 256, (binary ? _O_BINARY : _O_TEXT) | _O_NOINHERIT);
 }
 
 /* Get a FILE pointer to read from a file descriptor.  */

Re: [PATCH, libiberty] Pex pipes block under win32 when parent process dies

[Ian Lance Taylor]

Julian Brown <julian@codesourcery.com> writes:

>     libiberty/
>     * pex-win32.c (pex_win32_pipe): Add _O_NOINHERIT.    
>     (pex_win32_exec_child): Ensure each process has only one handle
>     open on pipe endpoints. Close standard input after creating child
>     for symmetry with standard output/standard error.

> +  /* Ensure we have inheritable descriptors to pass to the child, and close the
> +     original descriptors.  */
> +  orig_in = in;
> +  in = _dup (orig_in);
> +  obj->funcs->close (obj, orig_in);

You can just call _close, here and below (i.e., change the existing
code).  obj->funcs->close is always going to be pex_win32_close.  I
don't see any need to do the indirection.  That would mean not removing
the ATTRIBUTE_UNUSED on obj.

OK with that change.

Thanks.

Ian