diff mbox

[libiberty] avoid closing files twice on Windows when exec fails

Message ID 5011C109.9090503@codesourcery.com
State New
Headers show

Commit Message

Sandra Loosemore July 26, 2012, 10:13 p.m. UTC 
This is a libiberty patch for Windows that we've had in our local tree for a couple of years now.  The problem it addresses showed up in Windows-hosted GDB; we were getting a crash inside _close() in the case where creation of the child process has failed. 

Apparently Windows gets very unhappy if you try to close an already-closed file descriptor a second time.  This documentation, which turned up at the top of a Google search,

http://msdn.microsoft.com/en-us/library/5fzwd5ss%28v=vs.80%29.aspx
http://msdn.microsoft.com/en-us/library/ksazx244%28v=vs.80%29.aspx

seems to indicate that crashing is the default behavior if "fd is a bad file descriptor".

The duplicate calls causing the crash in pex_run_in_environment are pretty easy to avoid.  In the unmodified code, the stdin/stdout/stderr file descriptors are closed once in pex_win32_exec_child, immediately after they are dup'ed for the child; if process creation fails, they get closed a second time in pex_run_in_environment.  This patch fixes pex_win32_exec_child to only close the original file descriptors if process creation succeeds, leaving pex_run_in_environment to take care of the failure case.

We haven't tried to monitor other uses of _close in libiberty.  I was looking around for a Windows function to test whether a file descriptor is already closed that we could guard the calls in pex_win32_close with, but didn't see anything.  In any case, this patch has been working well enough for us, as far as it goes, and is at least an incremental improvement in robustness.  OK for mainline?

-Sandra


2012-07-26  Kazu Hirata  <kazu@codesourcery.com>
	    Sandra Loosemore  <sandra@codesourcery.com>

	libiberty/
	* pex-win32.c (pex_win32_exec_child): Only close original file
	descriptors if child is launched successfully.

Comments

DJ Delorie July 26, 2012, 10:53 p.m. UTC | #1 
I wonder if registering a handler for invalid parameters, at least
around those calls, so that we can enforce the posix-like "return an
error" semantics?

> OK for mainline?

Ok.

> 2012-07-26  Kazu Hirata  <kazu@codesourcery.com>
> 	    Sandra Loosemore  <sandra@codesourcery.com>
> 
> 	libiberty/
> 	* pex-win32.c (pex_win32_exec_child): Only close original file
> 	descriptors if child is launched successfully.
diff mbox

Patch

Index: libiberty/pex-win32.c
===================================================================
--- libiberty/pex-win32.c	(revision 189895)
+++ libiberty/pex-win32.c	(working copy)
@@ -741,24 +741,17 @@  pex_win32_exec_child (struct pex_obj *ob
   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.  */
+  /* Ensure we have inheritable descriptors to pass to the child.  */
   orig_in = in;
   in = _dup (orig_in);
-  if (orig_in != STDIN_FILENO)
-    _close (orig_in);
   
   orig_out = out;
   out = _dup (orig_out);
-  if (orig_out != STDOUT_FILENO)
-    _close (orig_out);
   
   if (separate_stderr)
     {
       orig_err = errdes;
       errdes = _dup (orig_err);
-      if (orig_err != STDERR_FILENO)
-	_close (orig_err);
     }
 
   stdin_handle = INVALID_HANDLE_VALUE;
@@ -836,6 +829,22 @@  pex_win32_exec_child (struct pex_obj *ob
       *errmsg = "CreateProcess";
     }
 
+  /* If the child was created successfully, close the original file
+     descriptors.  If the process creation fails, these are closed by
+     pex_run_in_environment instead.  We must not close them twice as
+     that seems to cause a Windows exception.  */
+     
+  if (pid != (pid_t) -1)
+    {
+      if (orig_in != STDIN_FILENO)
+	_close (orig_in);
+      if (orig_out != STDOUT_FILENO)
+	_close (orig_out);
+      if (separate_stderr
+	  && orig_err != STDERR_FILENO)
+	_close (orig_err);
+    }
+
   /* Close the standard input, standard output and standard error handles
      in the parent.  */