JDK-8027348 : (process) Enhancement of handling async close of ProcessInputStream
  • Type: Enhancement
  • Component: core-libs
  • Sub-Component: java.lang
  • Affected Version: 7u60,8
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2013-10-25
  • Updated: 2014-10-15
  • Resolved: 2014-01-31
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.
JDK 7 JDK 8 JDK 9
7u76Fixed 8u20Fixed 9 b03Fixed
Related Reports
Blocks :  
Duplicate :  
Duplicate :  
Relates :  
Relates :  
Description
Here's proposal by Martin Buchholz:

After rebasing, I propose this change which is shorter and is more obviously correct.

diff --git a/src/solaris/classes/java/lang/UNIXProcess.java.linux b/src/solaris/classes/java/lang/UNIXProcess.java.linux
--- a/src/solaris/classes/java/lang/UNIXProcess.java.linux
+++ b/src/solaris/classes/java/lang/UNIXProcess.java.linux
@@ -344,47 +344,39 @@
         ProcessPipeInputStream(int fd) {
             super(new FileInputStream(newFileDescriptor(fd)));
         }
-
-        private InputStream drainInputStream(InputStream in)
+        private static byte[] drainInputStream(InputStream in)
                 throws IOException {
             int n = 0;
             int j;
             byte[] a = null;
-            synchronized (closeLock) {
-                if (buf == null) // asynchronous close()?
-                    return null; // discard
-                j = in.available();
+            while ((j = in.available()) > 0) {
+                a = (a == null) ? new byte[j] : Arrays.copyOf(a, n + j);
+                n += in.read(a, n, j);
             }
-            while (j > 0) {
-                a = (a == null) ? new byte[j] : Arrays.copyOf(a, n + j);
-                synchronized (closeLock) {
-                    if (buf == null) // asynchronous close()?
-                        return null; // discard
-                    n += in.read(a, n, j);
-                    j = in.available();
-                }
-            }
-            return (a == null) ?
-                    ProcessBuilder.NullInputStream.INSTANCE :
-                    new ByteArrayInputStream(n == a.length ? a : Arrays.copyOf(a, n));
+            return (a == null || n == a.length) ? a : Arrays.copyOf(a, n);
         }
 
         /** Called by the process reaper thread when the process exits. */
         synchronized void processExited() {
-            try {
-                InputStream in = this.in;
-                if (in != null) {
-                    InputStream stragglers = drainInputStream(in);
-                    in.close();
-                    this.in = stragglers;
-                }
-            } catch (IOException ignored) { }
+            synchronized (closeLock) {
+                try {
+                    InputStream in = this.in;
+                    // this stream is closed if and only if: in == null
+                    if (in != null) {
+                        byte[] stragglers = drainInputStream(in);
+                        in.close();
+                        this.in = (stragglers == null) ?
+                            ProcessBuilder.NullInputStream.INSTANCE :
+                            new ByteArrayInputStream(stragglers);
+                    }
+                } catch (IOException ignored) {}
+            }
         }
 
         @Override
         public void close() throws IOException {
             // BufferedInputStream#close() is not synchronized unlike most other methods.
-            // Synchronizing helps avoid racing with drainInputStream().
+            // Synchronizing helps avoid race with processExited().
             synchronized (closeLock) {
                 super.close();
             }




I propose the test for this be in test/java/lang/ProcessBuilder/ instead of test/java/lang/Runtime/exec and that

- it runs for only 2 seconds by default.  This is the shortest time that a failure was detected. 20 seconds is way too long.
- it is much more likely to fail quickly than the original version.

Only tested on Linux.  There is porting work to be done (testing other platforms).
I would like y'all (Ivan?) to adopt this.

/*
 * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved.
 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 *
 * This code is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2 only, as
 * published by the Free Software Foundation.
 *
 * This code is distributed in the hope that it will be useful, but WITHOUT
 * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
 * version 2 for more details (a copy is included in the LICENSE file that
 * accompanied this code).
 *
 * You should have received a copy of the GNU General Public License version
 * 2 along with this work; if not, write to the Free Software Foundation,
 * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
 *
 * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
 * or visit www.oracle.com if you need additional information or have any
 * questions.
 */

/**
 * @test
 * @bug 8024521
 * @summary Closing ProcessPipeInputStream at the time the process exits is racy
 *          and leads to data corruption. Run this test manually (as
 *          an ordinary java program) with  -Xmx8M  to repro bug 8024521.
 * @run main/othervm -Xmx8M -Dtest.duration=2 CloseRace
 */

import java.io.*;
import java.util.ArrayList;
import java.util.List;

public class CloseRace {
    private static final String BIG_FILE = "bigfile";

    private static final int[] procFDs = new int[6];

    /** default value sufficient to repro bug 8024521. */
    private static final int testDurationSeconds
        = Integer.getInteger("test.duration", 600);
    
    static boolean fdInUse(int i) {
        return new File("/proc/self/fd/" + i).exists();
    }

    static boolean[] procFDsInUse() {
        boolean[] inUse = new boolean[procFDs.length];
        for (int i = 0; i < procFDs.length; i++)
            inUse[i] = fdInUse(procFDs[i]);
        return inUse;
    }

    static int count(boolean[] bits) {
        int count = 0;
        for (int i = 0; i < bits.length; i++)
            count += bits[i] ? 1 : 0;
        return count;
    }

    public static void main(String args[]) throws Exception {
        if (!(new File("/proc/self/fd").isDirectory()))
            return;

        // Catch Errors from process reaper
        Thread.setDefaultUncaughtExceptionHandler
            ((t, e) -> { e.printStackTrace(); System.exit(1); });

        try (RandomAccessFile f = new RandomAccessFile(BIG_FILE, "rw")) {
            f.setLength(Runtime.getRuntime().maxMemory()); // provoke OOME
        }

        for (int i = 0, j = 0; j < procFDs.length; i++)
            if (!fdInUse(i))
                procFDs[j++] = i;

        Thread[] threads = {
            new Thread(new OpenLoop()),
            new Thread(new ExecLoop()),
        };
        for (Thread thread : threads)
            thread.start();

        Thread.sleep(testDurationSeconds * 1000);

        for (Thread thread : threads)
            thread.interrupt();
        for (Thread thread : threads)
            thread.join();
    }

    static class OpenLoop implements Runnable {
        public void run() {
            while (!Thread.interrupted()) {
                try {
                    // wait for ExecLoop to finish creating process
                    do {} while (count(procFDsInUse()) != 3);
                    List<InputStream> iss = new ArrayList<>(4);

                    // eat up three "holes" (closed ends of pipe fd pairs)
                    for (int i = 0; i < 3; i++)
                        iss.add(new FileInputStream(BIG_FILE));
                    do {} while (count(procFDsInUse()) == procFDs.length);
                    // hopefully this will racily occupy empty fd slot
                    iss.add(new FileInputStream(BIG_FILE));
                    Thread.sleep(1); // Widen race window
                    for (InputStream is : iss)
                        is.close();
                } catch (InterruptedException e) {
                    break;
                } catch (Exception e) {
                    throw new Error(e);
                }
            }
        }
    }

    static class ExecLoop implements Runnable {
        public void run() {
            ProcessBuilder builder = new ProcessBuilder("/bin/true");
            while (!Thread.interrupted()) {
                try {
                    // wait for OpenLoop to finish
                    do {} while (count(procFDsInUse()) > 0);
                    Process process = builder.start();
                    InputStream is = process.getInputStream();
                    process.waitFor();
                    is.close();
                } catch (InterruptedException e) {
                    break;
                } catch (Exception e) {
                    throw new Error(e);
                }
            }
        }
    }
}



Comments
Re-requested the approval for this fix to be included into jdk9: http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-January/024537.html
20-01-2014

Release team: Rejecting fix as it's too late in JDK8. Deferring to 8u20.
03-12-2013

8-critical-request justification: This fix includes the enhanced test which reproduces the problem much faster (a few seconds instead of ~10 minutes) and more reliably. The fix also includes some code cleanup, which makes the code more obviously correct. Also, this variant of the fix matches with what was provided to the customer for testing (and the customer confirmed the fix). Risk Assessment: Low. This is a simplified version of the fix JDK-8024521. Level of testing coverage: The fix includes the enhanced jtreg test. The fix affects linux only and was tested with JPRT on both 32 and 64 bit linux platforms. Size of fix: ~30 lines of the library code. ~130 lines of the test.
02-12-2013