Blocks :
|
|
Duplicate :
|
|
Duplicate :
|
|
Relates :
|
|
Relates :
|
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); } } } } }
|