United StatesChange Country, Oracle Worldwide Web Sites Communities I am a... I want to...
Bug ID: JDK-6469606 (process) Process.destroy() can kill wrong process (Unix)
JDK-6469606 : (process) Process.destroy() can kill wrong process (Unix)

Details
Type:
Bug
Submit Date:
2006-09-12
Status:
Closed
Updated Date:
2012-02-02
Project Name:
JDK
Resolved Date:
2011-05-17
Component:
core-libs
OS:
generic
Sub-Component:
java.lang
CPU:
generic
Priority:
P3
Resolution:
Fixed
Affected Versions:
6
Fixed Versions:

Related Reports
Backport:
Backport:

Sub Tasks

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

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



Hardware and Software, Engineered to Work Together