8155808: Process.getInputStream().skip() method is faulty

For Process input streams, implement skip as read

Reviewed-by: prappo, dfuchs

jdk/src/java.base/share/classes/java/lang/Process.java

@@ -548,5 +548,36 @@ public abstract class Process {
         return toHandle().descendants();
     }
 
+    /**
+     * An input stream for a subprocess pipe that skips by reading bytes
+     * instead of seeking, the underlying pipe does not support seek.
+     */
+    static class PipeInputStream extends FileInputStream {
 
+        PipeInputStream(FileDescriptor fd) {
+            super(fd);
+        }
+
+        @Override
+        public long skip(long n) throws IOException {
+            long remaining = n;
+            int nr;
+
+            if (n <= 0) {
+                return 0;
+            }
+
+            int size = (int)Math.min(2048, remaining);
+            byte[] skipBuffer = new byte[size];
+            while (remaining > 0) {
+                nr = read(skipBuffer, 0, (int)Math.min(size, remaining));
+                if (nr < 0) {
+                    break;
+                }
+                remaining -= nr;
+            }
+
+            return n - remaining;
+        }
+    }
 }

jdk/src/java.base/unix/classes/java/lang/ProcessImpl.java

@@ -649,7 +649,7 @@ final class ProcessImpl extends Process {
         private final Object closeLock = new Object();
 
         ProcessPipeInputStream(int fd) {
-            super(new FileInputStream(newFileDescriptor(fd)));
+            super(new PipeInputStream(newFileDescriptor(fd)));
         }
         private static byte[] drainInputStream(InputStream in)
                 throws IOException {
@@ -725,8 +725,7 @@ final class ProcessImpl extends Process {
     // behavior.  By deferring the close we allow any pending reads to see -1
     // (EOF) as they did before.
     //
-    private static class DeferredCloseInputStream extends FileInputStream
+    private static class DeferredCloseInputStream extends PipeInputStream {
-    {
         DeferredCloseInputStream(FileDescriptor fd) {
             super(fd);
         }
@@ -848,7 +847,7 @@ final class ProcessImpl extends Process {
         private boolean closePending = false;
 
         DeferredCloseProcessPipeInputStream(int fd) {
-            super(new FileInputStream(newFileDescriptor(fd)));
+            super(new PipeInputStream(newFileDescriptor(fd)));
         }
 
         private InputStream drainInputStream(InputStream in)

jdk/src/java.base/windows/classes/java/lang/ProcessImpl.java

@@ -443,7 +443,7 @@ final class ProcessImpl extends Process {
                 FileDescriptor stdout_fd = new FileDescriptor();
                 fdAccess.setHandle(stdout_fd, stdHandles[1]);
                 stdout_stream = new BufferedInputStream(
-                    new FileInputStream(stdout_fd));
+                    new PipeInputStream(stdout_fd));
             }
 
             if (stdHandles[2] == -1L)
@@ -451,7 +451,7 @@ final class ProcessImpl extends Process {
             else {
                 FileDescriptor stderr_fd = new FileDescriptor();
                 fdAccess.setHandle(stderr_fd, stdHandles[2]);
-                stderr_stream = new FileInputStream(stderr_fd);
+                stderr_stream = new PipeInputStream(stderr_fd);
             }
 
             return null; }});

jdk/test/java/lang/ProcessBuilder/SkipTest.java

@@ -0,0 +1,175 @@
+/*
+ * Copyright (c) 2016, 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 8155808
+ * @run main SkipTest
+ * @summary verify skip method of Process Input Stream
+ */
+
+import java.io.BufferedOutputStream;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+
+public class SkipTest {
+    private static final String javaExe =
+            System.getProperty("java.home") +
+                    File.separator + "bin" + File.separator + "java";
+
+    private static final String classpath =
+            System.getProperty("java.class.path");
+
+    static final int BLOCK_SIZE = 10000;
+
+
+    public static void main(String[] args) throws Throwable {
+
+        // Start a Process to generate the test data to stdout and stderr
+        ProcessBuilder pb = new ProcessBuilder(javaExe, "-classpath", classpath, "SkipTest$GenerateData");
+        System.out.printf("cmd: %s%n", pb.command());
+        Process process = pb.start();
+
+        /*
+         * Verify the data expected is received mixing reads and skip.
+         */
+        try (InputStream in = process.getInputStream()) {
+
+            // Note: Process.getInputStream() actually returns a BufferedInputStream whose
+            // skip() method works perfectly, partially obscuring the bug. Only when the
+            // BufferedInputStream's buffer is drained, and it passes the skip() call to
+            // the underlying native stream, does the problem occur.
+
+            long n = in.skip(-1);
+            if (n != 0) {
+                throw new AssertionError("skip(-1) should return 0");
+            }
+            n = in.skip(0);
+            if (n != 0) {
+                throw new AssertionError("skip(0) should return 0");
+            }
+
+            // Now iterate all the data blocks
+            int header;
+            for (int expectedHeader = 'A'; (header = in.read()) != -1; expectedHeader++) {
+                // The header byte should be simple 'A' to 'Z'.
+                // When the bug hits, we will get lowercase letters instead.
+                if (header != expectedHeader) {
+                    throw new AssertionError("header char wrong, expected: " +
+                            expectedHeader + ", actual: " + header);
+                }
+
+                // Handle the data bytes.
+                // If the correct number of bytes are not skipped,
+                // then subsequent reads become out-of-sync;
+                int remaining = BLOCK_SIZE;
+                do {
+                    remaining -= in.skip(remaining);
+                } while (remaining != 0);
+            }
+            n = in.skip(1);
+            if (n != 0) {
+                throw new AssertionError("skip(1) at eof should return 0");
+            }
+        }
+
+        /**
+         * Do the same for the standard error stream.
+         */
+        try (InputStream in = process.getErrorStream()) {
+            long n = in.skip(-1);
+            if (n != 0) {
+                throw new AssertionError("skip(-1) should return 0");
+            }
+            n = in.skip(0);
+            if (n != 0) {
+                throw new AssertionError("skip(0) should return 0");
+            }
+
+            // Now iterate all the data blocks
+            int header;
+            for (int expectedHeader = 'A'; (header = in.read()) != -1; expectedHeader++) {
+                // The header byte should be simple 'A' to 'Z'.
+                // When the bug hits, we will get lowercase letters instead.
+                if (header != expectedHeader) {
+                    throw new AssertionError("header char wrong, expected: " +
+                            expectedHeader + ", actual: " + header);
+                }
+
+                // Handle the data bytes.
+                // If the correct number of bytes are not skipped,
+                // then subsequent reads become out-of-sync;
+                int remaining = BLOCK_SIZE;
+                do {
+                    remaining -= in.skip(remaining);
+                } while (remaining != 0);
+            }
+            n = in.skip(1);
+            if (n != 0) {
+                throw new AssertionError("skip(1) at eof should return 0");
+            }
+        }
+    }
+
+    /**
+     * Alternate main to generate the test data to standard output
+     * and standard error.
+     */
+    static class GenerateData {
+
+        public static void main(String[] args) {
+            // Generate test data containing a series of data blocks of length BLOCK_SIZE,
+            // each with a one-byte header. For example's sake, the "header" is a capital letter,
+            // and the "data" is the lowercase version of that letter repeated BLOCK_SIZE times:
+            try (OutputStream out = new BufferedOutputStream(System.out)) {
+                for (int header = 'A'; header <= 'Z'; header++) {
+                    out.write(header);
+
+                    int data = Character.toLowerCase(header);
+                    for (int i = 0; i < BLOCK_SIZE; i++) {
+                        out.write(data);
+                    }
+                }
+            } catch (IOException ioe) {
+                ioe.printStackTrace();
+                System.exit(1);
+            }
+            // Generate the same data to the error output
+            try (OutputStream err = new BufferedOutputStream(System.err)) {
+                for (int header = 'A'; header <= 'Z'; header++) {
+                    err.write(header);
+
+                    int data = Character.toLowerCase(header);
+                    for (int i = 0; i < BLOCK_SIZE; i++) {
+                        err.write(data);
+                    }
+                }
+            } catch (IOException ioe) {
+                ioe.printStackTrace();
+                System.exit(1);
+            }
+        }
+    }
+}