[PATCH] [MinGW]: ConcreteProcess Refactoring and Pipe Bugfix

[Mohan Embar <gnustuff at thisiscool dot com>]

Hi People,

This attempts to fix this issue:

http://gcc.gnu.org/ml/java/2003-10/msg00074.html

POSIX people please read on too. In particular, can you
read the FIXME comment in natWin32Process.cc / cleanup()
and tell me what you think of my alternate approach, which
would be applicable to POSIX too?

There were two issues. The primary one was that the child
process was being spawned with inheritable handles to the parent
pipe ends. Q190351 discusses this in detail:

http://support.microsoft.com/default.aspx?scid=http://support.microsoft.com:80/support/kb/articles/q190/3/51.asp&NoWebContent=1&NoWebContent=1

I'm very pleased with the helper class (ChildProcessPipe) I wrote
which attempts to add an element of sanity to all of this. Compare
the post-patch libgcj code with the code in Microsoft's sample.

The other issue is that Erik's example was closing the process'
streams, which is legal, but the cleanup() method in natWin32Process.cc closed
these too. My workaround is to no longer close the streams in cleanup(),
but I philosophize about this in the comment.

I'm including the text from Erik's original email about his
sample because it never seemed to make it to the list. The patch
is at the very end of this message. I've probably I've committed some
formatting crimes here.

-- Mohan
http://www.thisiscool.com/
http://www.animalsong.org/

10/9/2003 6:21:27 PM, Erik Poupaert <erik.poupaert@skynet.be> wrote:

>
>Hi Mohan,
>
>I've created my own dumb utility which simply reverses stdin to stdout: 
>
>$ echo "hello" | ./reversh
>olleh
>
>I call it from ./test, which forks ./reversh and sends "hello" its stdin, and then
>captures its stdout:
>
>$ ./test
>stdout=olleh
>
>It works fine on linux.
>
>I' added two bash scripts to build the example. free-build-lin32.sh for the linux
>build and free-build-win32.sh for the windows build.
>
>$ cd bug0410src/app-test
>$ ../free-build-lin32.sh		#-->builds the executables for linux
>$ cd ../bld-app-test/binlin32
>$ ls -l
>total 40
>-rwxr-xr-x    1 erik     users       12646 Oct  9 23:07 reversh
>-rwxr-xr-x    1 erik     users       24195 Oct  9 23:07 test
>
>
>It works no hassle for linux, while I can't make it work on win32. Let me know if you
>need more information to reproduce this issue.
>
>Greetings
>Erik
>

ChangeLog
2003-10-25  Mohan Embar  <gnustuff@thisiscool.com>

	* java/lang/Win32Process.java (hasExited) Changed from
	public to private.
	(startProcess): Likewise.
	(cleanup): Likewise.
	* java/lang/natWin32Process.cc (cleanup) Don't close
	input, output and error streams.
	(ChildProcessPipe): New helper class.
	(startProcess): Refactored to use ChildProcessPipe.

Index: java/lang/Win32Process.java
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/lang/Win32Process.java,v
retrieving revision 1.7
diff -u -2 -r1.7 Win32Process.java
--- java/lang/Win32Process.java	27 Jul 2003 04:13:03 -0000	1.7
+++ java/lang/Win32Process.java	26 Oct 2003 01:44:15 -0000
@@ -14,4 +14,7 @@
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.io.FileDescriptor;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
 import java.io.IOException;
 
@@ -29,6 +32,4 @@
   public native void destroy ();
 
-  public native boolean hasExited ();
-
   public int exitValue ()
   {
@@ -56,11 +57,4 @@
   public native int waitFor () throws InterruptedException;
 
-  public native void startProcess (String[] progarray,
-				   String[] envp,
-				   File dir)
-    throws IOException;
-
-  public native void cleanup ();
-
   public ConcreteProcess (String[] progarray,
                           String[] envp,
@@ -90,3 +84,10 @@
   // Exit code of the child if it has exited.
   private int exitCode;
+
+  private native boolean hasExited ();
+  private native void startProcess (String[] progarray,
+           String[] envp,
+           File dir)
+    throws IOException;
+  private native void cleanup ();
 }
Index: java/lang/natWin32Process.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/lang/natWin32Process.cc,v
retrieving revision 1.6
diff -u -2 -r1.6 natWin32Process.cc
--- java/lang/natWin32Process.cc	19 Sep 2003 08:28:43 -0000	1.6
+++ java/lang/natWin32Process.cc	26 Oct 2003 01:44:15 -0000
@@ -30,21 +30,30 @@
 java::lang::ConcreteProcess::cleanup (void)
 {
-  if (inputStream != NULL)
-    {
-      inputStream->close ();
-      inputStream = NULL;
-    }
-
-  if (outputStream != NULL)
-    {
-      outputStream->close ();
-      outputStream = NULL;
-    }
-
-  if (errorStream != NULL)
-    {
-      errorStream->close ();
-      errorStream = NULL;
-    }
+  // FIXME:
+  // We used to close the input, output and
+  // error streams here, but we can't do that
+  // because the caller also has the right
+  // to close these and FileInputStream and FileOutputStream
+  // scream if you attempt to close() them twice. Presently,
+  // we use _Jv_platform_close_on_exec, which is similar
+  // to the POSIX approach. _Jv_platform_close_on_exec
+  // doesn't do anything on Win32, but since
+  // the handles to our ends of the pipe are non-inheritable,
+  // this basically has the same effect.
+  //
+  // What I wanted to do is have private nested
+  // classes in ConcreteProcess which extend FileInputStream
+  // and FileOutputStream, respectively, but override
+  // close() to permit multiple calls to close(). This
+  // led to class header and platform configury issues
+  // that I didn't feel like dealing with. However,
+  // this approach could conceivably be a good multiplatform
+  // one since delaying the pipe close until process
+  // termination could be wasteful if many child processes
+  // are spawned within the parent process' lifetime.
+  inputStream = NULL;
+  outputStream = NULL;
+  errorStream = NULL;
+  
   if (procHandle)
     {
@@ -130,4 +139,94 @@
 }
 
+
+// Helper class for creating and managing the pipes
+// used for I/O redirection for child processes.
+class ChildProcessPipe
+{
+public:
+  // Indicates from the child process' point of view
+  // whether the pipe is for reading or writing.
+  enum EType {INPUT, OUTPUT};
+
+  ChildProcessPipe(EType eType);
+  ~ChildProcessPipe();
+  
+  // Returns a pipe handle suitable for use by the parent process
+  HANDLE getParentHandle();
+  
+  // Returns a pipe handle suitable for use by the child process.
+  HANDLE getChildHandle();
+  
+private:
+  EType m_eType;
+  HANDLE m_hRead, m_hWrite;
+};
+
+ChildProcessPipe::ChildProcessPipe(EType eType):
+  m_eType(eType)
+{
+  SECURITY_ATTRIBUTES sAttrs;
+
+  // Explicitly allow the handles to the pipes to be inherited.
+  sAttrs.nLength = sizeof (SECURITY_ATTRIBUTES);
+  sAttrs.bInheritHandle = 1;
+  sAttrs.lpSecurityDescriptor = NULL;
+
+  if (CreatePipe (&m_hRead, &m_hWrite, &sAttrs, 0) == 0)
+    {
+      DWORD dwErrorCode = GetLastError ();
+      throw new java::io::IOException (
+        _Jv_WinStrError ("Error creating pipe", dwErrorCode));
+    }
+
+  // If this is the read end of the child, we need
+  // to make the parent write end non-inheritable. Similarly,
+  // if this is the write end of the child, we need to make
+  // the parent read end non-inheritable. If we didn't
+  // do this, the child would inherit these ends and we wouldn't
+  // be able to close them from our end. For full details,
+  // do a Google search on "Q190351".
+  HANDLE& rhStd = m_eType==INPUT ? m_hWrite : m_hRead;
+  HANDLE hStdNew;
+  if (DuplicateHandle (GetCurrentProcess (), rhStd,
+    GetCurrentProcess (), &hStdNew, 0, 0,
+    DUPLICATE_SAME_ACCESS) == 0)
+    {
+      DWORD dwErrorCode = GetLastError ();
+      throw new java::io::IOException (
+        _Jv_WinStrError ("Error duplicating handle", dwErrorCode));
+    }
+
+  // Close the inheritable handle before spawning
+  // the child process.
+  CloseHandle(rhStd);
+
+  // Indicate that we want the non-inheritable handle closed
+  // when the parent process exits. See the comment
+  // in cleanup() for more details.
+  _Jv_platform_close_on_exec ((jint) hStdNew);
+
+  // Only remember the non-inheritable one.
+  rhStd = hStdNew;
+}
+
+ChildProcessPipe::~ChildProcessPipe()
+{
+  // Close the parent end of the pipe. This
+  // destructor is called after the child process
+  // has been spawned.
+  CloseHandle(getChildHandle());
+}
+
+HANDLE ChildProcessPipe::getParentHandle()
+{
+  return m_eType==INPUT ? m_hWrite : m_hRead;
+}
+
+HANDLE ChildProcessPipe::getChildHandle()
+{
+  return m_eType==INPUT ? m_hRead : m_hWrite;
+}
+
 void
 java::lang::ConcreteProcess::startProcess (jstringArray progarray,
@@ -198,44 +297,14 @@
       // We create anonymous pipes to communicate with the child
       // on each of standard streams.
-
-      HANDLE cldStdInRd, cldStdInWr;
-      HANDLE cldStdOutRd, cldStdOutWr;
-      HANDLE cldStdErrRd, cldStdErrWr;
-
-      SECURITY_ATTRIBUTES sAttrs;
-
-      // Explicitly allow the handles to the pipes to be inherited.
-      sAttrs.nLength = sizeof (SECURITY_ATTRIBUTES);
-      sAttrs.bInheritHandle = 1;
-      sAttrs.lpSecurityDescriptor = NULL;
-
-
-      if (CreatePipe (&cldStdInRd, &cldStdInWr, &sAttrs, 0) == 0)
-        {
-          DWORD dwErrorCode = GetLastError ();
-          throw new IOException (_Jv_WinStrError ("Error creating stdin pipe",
-            dwErrorCode));
-        }
-
-      if (CreatePipe (&cldStdOutRd, &cldStdOutWr, &sAttrs, 0) == 0)
-        {
-          DWORD dwErrorCode = GetLastError ();
-          throw new IOException (_Jv_WinStrError ("Error creating stdout pipe",
-            dwErrorCode));
-        }
-
-      if (CreatePipe (&cldStdErrRd, &cldStdErrWr, &sAttrs, 0) == 0)
-        {
-          DWORD dwErrorCode = GetLastError ();
-          throw new IOException (_Jv_WinStrError ("Error creating stderr pipe",
-            dwErrorCode));
-        }
-
-      outputStream = new FileOutputStream
-                         (new FileDescriptor ((jint) cldStdInWr));
-      inputStream = new FileInputStream
-                        (new FileDescriptor ((jint) cldStdOutRd));
-      errorStream = new FileInputStream
-                        (new FileDescriptor ((jint) cldStdErrRd));
+      ChildProcessPipe aChildStdIn(ChildProcessPipe::INPUT);
+      ChildProcessPipe aChildStdOut(ChildProcessPipe::OUTPUT);
+      ChildProcessPipe aChildStdErr(ChildProcessPipe::OUTPUT);
+
+      outputStream = new FileOutputStream (new FileDescriptor (
+                           (jint) aChildStdIn.getParentHandle ()));
+      inputStream = new FileInputStream (new FileDescriptor (
+                           (jint) aChildStdOut.getParentHandle ()));
+      errorStream = new FileInputStream (new FileDescriptor (
+                           (jint) aChildStdErr.getParentHandle ()));
 
       // Now create the child process.
@@ -251,7 +320,7 @@
       si.dwFlags |= STARTF_USESTDHANDLES;
 
-      si.hStdInput = cldStdInRd;
-      si.hStdOutput = cldStdOutWr;
-      si.hStdError = cldStdErrWr;
+      si.hStdInput = aChildStdIn.getChildHandle();
+      si.hStdOutput = aChildStdOut.getChildHandle();
+      si.hStdError = aChildStdErr.getChildHandle();
 
       if (CreateProcess (NULL,
@@ -272,9 +341,4 @@
 
       procHandle = (jint ) pi.hProcess;
-
-      // Close the wrong ends (for the parent) of the pipes.
-      CloseHandle (cldStdInRd);
-      CloseHandle (cldStdOutWr);
-      CloseHandle (cldStdErrWr);
 
       _Jv_Free (cmdLine);