JDK-6469606 : (process) Process.destroy() can kill wrong process (Unix)
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.lang
  • Affected Version: 6
  • Priority: P3
  • Status: Closed
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2006-09-12
  • Updated: 2012-02-02
  • Resolved: 2011-05-17
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
Other JDK 6 JDK 7
5.0u11Fixed 6u1Fixed 7 b03Fixed
Description
The current implementation of java.lang.Process.destroy() is incorrect.
Now it is possible to call destroy and cause some other process to be terminated.

Current java.lang.Process implementation is:

--- src/solaris/native/java/lang/UNIXProcess_md.c ---
JNIEXPORT void JNICALL
Java_java_lang_UNIXProcess_destroyProcess(JNIEnv *env, jobject junk, jint pid)
{
   kill(pid, SIGTERM);
}
--- src/solaris/native/java/lang/UNIXProcess_md.c ---


--- src/solaris/classes/java/lang/UNIXProcess.java.linux ---
   public void destroy() {
       destroyProcess(pid);
       try {
           stdin_stream.close();
           stdout_stream.close();
           stderr_stream.close();
       } catch (IOException e) {
           // ignore
       }
   }
--- src/solaris/classes/java/lang/UNIXProcess.java.linux ---

As result, the following situation is possible:

Thread1                     |  Thread2
----------------------------------------------------------------
createProcess               |
process.pid=X               |

process.waitFor()           |
process has been terminated |
                            |
                            |
                            |  createProcess
                            |  process.pid=X  (I know that OS is supposed to avoid assigning same pid for some time but ...)
process.destroy()           |
-> kill(X, SIGTERM)         |
                            |  got signal and terminated with -1 (according to Java_java_lang_UNIXProcess_waitForProcessExit)
                            |
----------------------------------------------------------------


It would be better if 'destroyProcess(pid)' will be called only in case the process has not been terminated yet.

Something like:
   public void destroy() {
       synchronized(this)
       {
           if (!hasExited)
               destroyProcess(pid);
       }

       try {
           stdin_stream.close();
           stdout_stream.close();
           stderr_stream.close();
       } catch (IOException e) {
           // ignore
       }
   }


I will attach the test case after it will be ready.

Comments
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); } } }
13-09-2006

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();
12-09-2006

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.
12-09-2006

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.
12-09-2006