EVALUATION

Here is a smaller test for reproducing this bug,
which succeeds in demonstrating the fix, but is still not
suitable for a regression test, given that it creates 2**15 processes.

import java.io.*;

public class FeelingLucky {

    public static void main(String[] args) throws Throwable {
	final Runtime rt = Runtime.getRuntime();
	final String[] pidPrinter = {"/bin/sh", "-c", "echo $$"};
        final Process minedProcess = rt.exec(pidPrinter);
	int minedPid = 0;
	final InputStream is = minedProcess.getInputStream();
	int c;
	while ((c = is.read()) >= '0' && c <= '9')
	    minedPid = 10 * minedPid + (c - '0');
	System.out.printf("minedPid=%d%n", minedPid);
	minedProcess.waitFor();

	final String[] magnum = {
	    "perl", "-e",
	    "my $punk = $PPID;" +
	    "open TTY, '> /dev/tty';" +
	    "for (my $i = 0; $i < 32768; $i++) {" +
	    "  my $pid = fork();" +
	    "  if ($pid == 0) {" +
	    "    if ($$ == " + minedPid + ") {" +
	    "      print TTY \"MATCH $$\\n\";" +
	    "      $SIG{TERM} = sub {" +
	    "        print TTY \"Got TERM $$\\n\";" +
	    "        kill -9, $punk;" +
	    "        exit; };" +
	    "      $| = 1; print 'Go ahead.  Make my day.';" +
	    "      sleep 100;" +
	    "    }" +
	    "    exit;" +
	    "  } else { " +
	    "    waitpid($pid,0);" +
	    "    break if $pid == " + minedPid + ";" +
	    "  }" +
	    "}"
	};

	//System.out.println(java.util.Arrays.toString(magnum));
	Process p = rt.exec(magnum);
	if (p.getInputStream().read() != -1) {
	    System.out.println("got a char");
	    minedProcess.destroy();
	    Thread.sleep(1000);
	}
    }
}
                                     

EVALUATION

Before calling "kill", we should check that the process is indeed our child.
We may have to install a non-trivial SIGCHLD handler that notifies the process 
when it is dead.  Global state is always difficult.

It would also be nice if there was a reasonable way to check the state of a process,
and to wait for process termination with timeout.
                                     

WORK AROUND

Use Process.exitValue to check for whether process has
terminated (by catching IllegalThreadStateException !)
and only calling destroy if process has not.

There is still a very small window where the process may have died
and the OS has recycled the pid, causing the wrong process to die,
but OSes usually try hard to prevent this kind of instantaneous pid reuse.
                                     

EVALUATION

It is easier than I thought to check for "hasExited".

--- /u/martin/ws/mustang/src/solaris/classes/java/lang/UNIXProcess.java.linux	2005-09-06 19:31:17.735153000 -0700
+++ /u/martin/ws/process/src/solaris/classes/java/lang/UNIXProcess.java.linux	2006-09-12 10:40:50.139388000 -0700
@@ -176,7 +176,16 @@
 
     private static native void destroyProcess(int pid);
     public void destroy() {
+	// There is a risk that pid will be recycled, causing us to
+	// kill the wrong process!  So we only terminate processes
+	// that appear to still be running.  Even with this check,
+	// there is an unavoidable race condition here, but the window
+	// is very small, and OSes try hard to not recycle pids too
+	// soon, so this is quite safe.
+	synchronized (this) {
+	    if (!hasExited)
 	destroyProcess(pid);
+	}
         try {
             stdin_stream.close();
             stdout_stream.close();
--- /u/martin/ws/mustang/src/solaris/classes/java/lang/UNIXProcess.java.solaris	2005-09-06 19:31:17.929052000 -0700
+++ /u/martin/ws/process/src/solaris/classes/java/lang/UNIXProcess.java.solaris	2006-09-12 10:50:29.665935000 -0700
@@ -125,8 +125,14 @@
     }
 
     private static native void destroyProcess(int pid);
-
     public synchronized void destroy() {
+	// There is a risk that pid will be recycled, causing us to
+	// kill the wrong process!  So we only terminate processes
+	// that appear to still be running.  Even with this check,
+	// there is an unavoidable race condition here, but the window
+	// is very small, and OSes try hard to not recycle pids too
+	// soon, so this is quite safe.
+	if (!hasExited)
 	destroyProcess(pid);
 	try {
             stdin_stream.close();