diff --git a/src/java.base/unix/native/jspawnhelper/jspawnhelper.c b/src/java.base/unix/native/jspawnhelper/jspawnhelper.c index c05a6fec8b0..16845901589 100644 --- a/src/java.base/unix/native/jspawnhelper/jspawnhelper.c +++ b/src/java.base/unix/native/jspawnhelper/jspawnhelper.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2013, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2013, 2023, 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 @@ -23,6 +23,7 @@ * questions. */ +#include #include #include #include @@ -83,11 +84,15 @@ void initChildStuff (int fdin, int fdout, ChildStuff *c) { error (fdout, ERR_PIPE); } - if (readFully (fdin, c, sizeof(*c)) == -1) { +#ifdef DEBUG + jtregSimulateCrash(0, 5); +#endif + + if (readFully (fdin, c, sizeof(*c)) != sizeof(*c)) { error (fdout, ERR_PIPE); } - if (readFully (fdin, &sp, sizeof(sp)) == -1) { + if (readFully (fdin, &sp, sizeof(sp)) != sizeof(sp)) { error (fdout, ERR_PIPE); } @@ -96,7 +101,7 @@ void initChildStuff (int fdin, int fdout, ChildStuff *c) { ALLOC(buf, bufsize); - if (readFully (fdin, buf, bufsize) == -1) { + if (readFully (fdin, buf, bufsize) != bufsize) { error (fdout, ERR_PIPE); } @@ -132,12 +137,15 @@ int main(int argc, char *argv[]) { ChildStuff c; struct stat buf; /* argv[0] contains the fd number to read all the child info */ - int r, fdin, fdout; + int r, fdinr, fdinw, fdout; sigset_t unblock_signals; - r = sscanf (argv[argc-1], "%d:%d", &fdin, &fdout); - if (r == 2 && fcntl(fdin, F_GETFD) != -1) { - fstat(fdin, &buf); +#ifdef DEBUG + jtregSimulateCrash(0, 4); +#endif + r = sscanf (argv[argc-1], "%d:%d:%d", &fdinr, &fdinw, &fdout); + if (r == 3 && fcntl(fdinr, F_GETFD) != -1 && fcntl(fdinw, F_GETFD) != -1) { + fstat(fdinr, &buf); if (!S_ISFIFO(buf.st_mode)) shutItDown(); } else { @@ -148,7 +156,18 @@ int main(int argc, char *argv[]) { sigemptyset(&unblock_signals); sigprocmask(SIG_SETMASK, &unblock_signals, NULL); - initChildStuff (fdin, fdout, &c); + // Close the writing end of the pipe we use for reading from the parent. + // We have to do this before we start reading from the parent to avoid + // blocking in the case the parent exits before we finished reading from it. + close(fdinw); // Deliberately ignore errors (see https://lwn.net/Articles/576478/). + initChildStuff (fdinr, fdout, &c); + // Now set the file descriptor for the pipe's writing end to -1 + // for the case that somebody tries to close it again. + assert(c.childenv[1] == fdinw); + c.childenv[1] = -1; + // The file descriptor for reporting errors back to our parent we got on the command + // line should be the same like the one in the ChildStuff struct we've just read. + assert(c.fail[1] == fdout); childProcess (&c); return 0; /* NOT REACHED */ diff --git a/src/java.base/unix/native/libjava/ProcessImpl_md.c b/src/java.base/unix/native/libjava/ProcessImpl_md.c index fe3101e25ce..66c44956b21 100644 --- a/src/java.base/unix/native/libjava/ProcessImpl_md.c +++ b/src/java.base/unix/native/libjava/ProcessImpl_md.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 1995, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1995, 2023, 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 @@ -487,14 +487,14 @@ static pid_t spawnChild(JNIEnv *env, jobject process, ChildStuff *c, const char *helperpath) { pid_t resultPid; int i, offset, rval, bufsize, magic; - char *buf, buf1[(2 * 11) + 2]; // "%d:%d\0" + char *buf, buf1[(3 * 11) + 3]; // "%d:%d:%d\0" char *hlpargs[2]; SpawnInfo sp; /* need to tell helper which fd is for receiving the childstuff * and which fd to send response back on */ - snprintf(buf1, sizeof(buf1), "%d:%d", c->childenv[0], c->fail[1]); + snprintf(buf1, sizeof(buf1), "%d:%d:%d", c->childenv[0], c->childenv[1], c->fail[1]); /* put the fd string as argument to the helper cmd */ hlpargs[0] = buf1; hlpargs[1] = 0; @@ -527,7 +527,7 @@ spawnChild(JNIEnv *env, jobject process, ChildStuff *c, const char *helperpath) if (c->fds[i] != -1) { int flags = fcntl(c->fds[i], F_GETFD); if (flags & FD_CLOEXEC) { - fcntl(c->fds[i], F_SETFD, flags & (~1)); + fcntl(c->fds[i], F_SETFD, flags & (~FD_CLOEXEC)); } } } @@ -538,6 +538,10 @@ spawnChild(JNIEnv *env, jobject process, ChildStuff *c, const char *helperpath) return -1; } +#ifdef DEBUG + jtregSimulateCrash(resultPid, 1); +#endif + /* now the lengths are known, copy the data */ buf = NEW(char, bufsize); if (buf == 0) { @@ -553,11 +557,24 @@ spawnChild(JNIEnv *env, jobject process, ChildStuff *c, const char *helperpath) magic = magicNumber(); /* write the two structs and the data buffer */ - write(c->childenv[1], (char *)&magic, sizeof(magic)); // magic number first - write(c->childenv[1], (char *)c, sizeof(*c)); - write(c->childenv[1], (char *)&sp, sizeof(sp)); - write(c->childenv[1], buf, bufsize); + if (writeFully(c->childenv[1], (char *)&magic, sizeof(magic)) != sizeof(magic)) { // magic number first + return -1; + } +#ifdef DEBUG + jtregSimulateCrash(resultPid, 2); +#endif + if (writeFully(c->childenv[1], (char *)c, sizeof(*c)) != sizeof(*c) || + writeFully(c->childenv[1], (char *)&sp, sizeof(sp)) != sizeof(sp) || + writeFully(c->childenv[1], buf, bufsize) != bufsize) { + return -1; + } + /* We're done. Let jspwanhelper know he can't expect any more data from us. */ + close(c->childenv[1]); + c->childenv[1] = -1; free(buf); +#ifdef DEBUG + jtregSimulateCrash(resultPid, 3); +#endif /* In this mode an external main() in invoked which calls back into * childProcess() in this file, rather than directly @@ -610,6 +627,8 @@ Java_java_lang_ProcessImpl_forkAndExec(JNIEnv *env, in[0] = in[1] = out[0] = out[1] = err[0] = err[1] = fail[0] = fail[1] = -1; childenv[0] = childenv[1] = -1; + // Reset errno to protect against bogus error messages + errno = 0; if ((c = NEW(ChildStuff, 1)) == NULL) return -1; c->argv = NULL; @@ -708,11 +727,9 @@ Java_java_lang_ProcessImpl_forkAndExec(JNIEnv *env, goto Catch; } case sizeof(errnum): - assert(errnum == CHILD_IS_ALIVE); if (errnum != CHILD_IS_ALIVE) { - /* Should never happen since the first thing the spawn - * helper should do is to send an alive ping to the parent, - * before doing any subsequent work. */ + /* This can happen if the spawn helper encounters an error + * before or during the handshake with the parent. */ throwIOException(env, 0, "Bad code from spawn helper " "(Failed to exec spawn helper)"); goto Catch; @@ -748,8 +765,12 @@ Java_java_lang_ProcessImpl_forkAndExec(JNIEnv *env, /* Always clean up fail and childEnv descriptors */ closeSafely(fail[0]); closeSafely(fail[1]); - closeSafely(childenv[0]); - closeSafely(childenv[1]); + /* We use 'c->childenv' here rather than 'childenv' because 'spawnChild()' might have + * already closed 'c->childenv[1]' and signaled this by setting 'c->childenv[1]' to '-1'. + * Otherwise 'c->childenv' and 'childenv' are the same because we just copied 'childenv' + * to 'c->childenv' (with 'copyPipe()') before calling 'startChild()'. */ + closeSafely(c->childenv[0]); + closeSafely(c->childenv[1]); releaseBytes(env, helperpath, phelperpath); releaseBytes(env, prog, pprog); diff --git a/src/java.base/unix/native/libjava/childproc.c b/src/java.base/unix/native/libjava/childproc.c index 1044b8ee226..c5dc6d31029 100644 --- a/src/java.base/unix/native/libjava/childproc.c +++ b/src/java.base/unix/native/libjava/childproc.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2013, 2020, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2013, 2023, 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 @@ -36,14 +36,6 @@ const char * const *parentPathv; -ssize_t -restartableWrite(int fd, const void *buf, size_t count) -{ - ssize_t result; - RESTARTABLE(write(fd, buf, count), result); - return result; -} - int restartableDup2(int fd_from, int fd_to) { @@ -163,6 +155,46 @@ readFully(int fd, void *buf, size_t nbyte) } } +/* + * Writes nbyte bytes from buf into file descriptor fd, + * The write operation is retried in case of EINTR or partial writes. + * + * Returns number of bytes written (normally nbyte). + * In case of write errors, returns -1 and sets errno. + */ +ssize_t +writeFully(int fd, const void *buf, size_t nbyte) +{ +#ifdef DEBUG +/* This code is only used in debug builds for testing truncated writes + * during the handshake with the spawn helper for MODE_POSIX_SPAWN. + * See: test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java + */ + const char* env = getenv("JTREG_JSPAWNHELPER_PROTOCOL_TEST"); + if (env != NULL && atoi(env) == 99 && nbyte == sizeof(ChildStuff)) { + printf("posix_spawn: truncating write of ChildStuff struct\n"); + fflush(stdout); + nbyte = nbyte / 2; + } +#endif + ssize_t remaining = nbyte; + for (;;) { + ssize_t n = write(fd, buf, remaining); + if (n > 0) { + remaining -= n; + if (remaining <= 0) + return nbyte; + /* We were interrupted in the middle of writing the bytes. + * Unlikely, but possible. */ + buf = (void *) (((char *)buf) + n); + } else if (n == -1 && errno == EINTR) { + /* Retry */ + } else { + return -1; + } + } +} + void initVectorFromBlock(const char**vector, const char* block, int count) { @@ -210,7 +242,7 @@ execve_with_shell_fallback(int mode, const char *file, const char *argv[], const char *const envp[]) { - if (mode == MODE_CLONE || mode == MODE_VFORK) { + if (mode == MODE_VFORK) { /* shared address space; be very careful. */ execve(file, (char **) argv, (char **) envp); if (errno == ENOEXEC) @@ -321,9 +353,14 @@ childProcess(void *arg) /* Child shall signal aliveness to parent at the very first * moment. */ int code = CHILD_IS_ALIVE; - restartableWrite(fail_pipe_fd, &code, sizeof(code)); + if (writeFully(fail_pipe_fd, &code, sizeof(code)) != sizeof(code)) { + goto WhyCantJohnnyExec; + } } +#ifdef DEBUG + jtregSimulateCrash(0, 6); +#endif /* Close the parent sides of the pipes. Closing pipe fds here is redundant, since closeDescriptors() would do it anyways, but a little paranoia is a good thing. */ @@ -390,9 +427,26 @@ childProcess(void *arg) */ { int errnum = errno; - restartableWrite(fail_pipe_fd, &errnum, sizeof(errnum)); + writeFully(fail_pipe_fd, &errnum, sizeof(errnum)); } close(fail_pipe_fd); _exit(-1); return 0; /* Suppress warning "no return value from function" */ } + +#ifdef DEBUG +/* This method is only used in debug builds for testing MODE_POSIX_SPAWN + * in the light of abnormal program termination of either the parent JVM + * or the newly created jspawnhelper child process during the execution of + * Java_java_lang_ProcessImpl_forkAndExec(). + * See: test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java + */ +void jtregSimulateCrash(pid_t child, int stage) { + const char* env = getenv("JTREG_JSPAWNHELPER_PROTOCOL_TEST"); + if (env != NULL && atoi(env) == stage) { + printf("posix_spawn:%d\n", child); + fflush(stdout); + _exit(stage); + } +} +#endif diff --git a/src/java.base/unix/native/libjava/childproc.h b/src/java.base/unix/native/libjava/childproc.h index 2190dd17923..e39126aa119 100644 --- a/src/java.base/unix/native/libjava/childproc.h +++ b/src/java.base/unix/native/libjava/childproc.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2013, 2020, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2013, 2023, 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 @@ -85,7 +85,6 @@ extern char **environ; #define MODE_FORK 1 #define MODE_POSIX_SPAWN 2 #define MODE_VFORK 3 -#define MODE_CLONE 4 typedef struct _ChildStuff { @@ -128,7 +127,7 @@ typedef struct _SpawnInfo { */ extern const char * const *parentPathv; -ssize_t restartableWrite(int fd, const void *buf, size_t count); +ssize_t writeFully(int fd, const void *buf, size_t count); int restartableDup2(int fd_from, int fd_to); int closeSafely(int fd); int isAsciiDigit(char c); @@ -149,4 +148,14 @@ void JDK_execvpe(int mode, const char *file, const char *const envp[]); int childProcess(void *arg); +#ifdef DEBUG +/* This method is only used in debug builds for testing MODE_POSIX_SPAWN + * in the light of abnormal program termination of either the parent JVM + * or the newly created jspawnhelper child process during the execution of + * Java_java_lang_ProcessImpl_forkAndExec(). + * See: test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java + */ +void jtregSimulateCrash(pid_t child, int stage); +#endif + #endif diff --git a/test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java b/test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java new file mode 100644 index 00000000000..d82a317148e --- /dev/null +++ b/test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java @@ -0,0 +1,237 @@ +/* + * Copyright Amazon.com Inc. 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 8307990 + * @requires (os.family == "linux") | (os.family == "aix") + * @requires vm.debug + * @library /test/lib + * @run main/othervm/timeout=300 JspawnhelperProtocol + */ + +import java.io.BufferedReader; +import java.io.IOException; +import java.nio.file.Path; +import java.util.Optional; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + +import jdk.test.lib.process.ProcessTools; + +public class JspawnhelperProtocol { + // Timout in seconds + private static final int TIMEOUT = 60; + // Base error code to communicate various error states from the parent process to the top-level test + private static final int ERROR = 10; + private static final String[] CMD = { "pwd" }; + private static final String ENV_KEY = "JTREG_JSPAWNHELPER_PROTOCOL_TEST"; + + private static void parentCode(String arg) throws IOException, InterruptedException { + System.out.println("Recursively executing 'JspawnhelperProtocol " + arg + "'"); + Process p = null; + try { + p = Runtime.getRuntime().exec(CMD); + } catch (Exception e) { + e.printStackTrace(System.out); + System.exit(ERROR); + } + if (!p.waitFor(TIMEOUT, TimeUnit.SECONDS)) { + System.out.println("Child process timed out"); + System.exit(ERROR + 1); + } + if (p.exitValue() == 0) { + String pwd = p.inputReader().readLine(); + String realPwd = Path.of("").toAbsolutePath().toString(); + if (!realPwd.equals(pwd)) { + System.out.println("Child process returned '" + pwd + "' (expected '" + realPwd + "')"); + System.exit(ERROR + 2); + } + System.out.println(" Successfully executed '" + CMD[0] + "'"); + System.exit(0); + } else { + System.out.println(" Failed to executed '" + CMD[0] + "' (exitValue=" + p.exitValue() + ")"); + System.exit(ERROR + 3); + } + } + + private static void normalExec() throws Exception { + ProcessBuilder pb; + pb = ProcessTools.createJavaProcessBuilder("-Djdk.lang.Process.launchMechanism=posix_spawn", + "JspawnhelperProtocol", + "normalExec"); + pb.inheritIO(); + Process p = pb.start(); + if (!p.waitFor(TIMEOUT, TimeUnit.SECONDS)) { + throw new Exception("Parent process timed out"); + } + if (p.exitValue() != 0) { + throw new Exception("Parent process exited with " + p.exitValue()); + } + } + + private static void simulateCrashInChild(int stage) throws Exception { + ProcessBuilder pb; + pb = ProcessTools.createJavaProcessBuilder("-Djdk.lang.Process.launchMechanism=posix_spawn", + "JspawnhelperProtocol", + "simulateCrashInChild" + stage); + pb.environment().put(ENV_KEY, Integer.toString(stage)); + Process p = pb.start(); + + boolean foundCrashInfo = false; + try (BufferedReader br = p.inputReader()) { + String line = br.readLine(); + while (line != null) { + System.out.println(line); + if (line.equals("posix_spawn:0")) { + foundCrashInfo = true; + } + line = br.readLine(); + } + } + if (!foundCrashInfo) { + throw new Exception("Wrong output from child process"); + } + if (!p.waitFor(TIMEOUT, TimeUnit.SECONDS)) { + throw new Exception("Parent process timed out"); + } + + int ret = p.exitValue(); + if (ret == 0) { + throw new Exception("Expected error during child execution"); + } + System.out.println("Parent exit code: " + ret); + } + + private static void simulateCrashInParent(int stage) throws Exception { + ProcessBuilder pb; + pb = ProcessTools.createJavaProcessBuilder("-Djdk.lang.Process.launchMechanism=posix_spawn", + "JspawnhelperProtocol", + "simulateCrashInParent" + stage); + pb.environment().put(ENV_KEY, Integer.toString(stage)); + Process p = pb.start(); + + String line = null; + try (BufferedReader br = p.inputReader()) { + line = br.readLine(); + while (line != null && !line.startsWith("posix_spawn:")) { + System.out.println(line); + line = br.readLine(); + } + } + if (line == null) { + throw new Exception("Wrong output from parent process"); + } + System.out.println(line); + long childPid = Integer.parseInt(line.substring(line.indexOf(':') + 1)); + + if (!p.waitFor(TIMEOUT, TimeUnit.SECONDS)) { + throw new Exception("Parent process timed out"); + } + + Optional oph = ProcessHandle.of(childPid); + if (!oph.isEmpty()) { + ProcessHandle ph = oph.get(); + try { + // Give jspawnhelper a chance to exit gracefully + ph.onExit().get(TIMEOUT, TimeUnit.SECONDS); + } catch (TimeoutException te) { + Optional cmd = ph.info().command(); + if (cmd.isPresent() && cmd.get().endsWith("jspawnhelper")) { + throw new Exception("jspawnhelper still alive after parent Java process terminated"); + } + } + } + int ret = p.exitValue(); + if (ret != stage) { + throw new Exception("Expected exit code " + stage + " but got " + ret); + } + System.out.println("Parent exit code: " + ret); + } + + private static void simulateTruncatedWriteInParent(int stage) throws Exception { + ProcessBuilder pb; + pb = ProcessTools.createJavaProcessBuilder("-Djdk.lang.Process.launchMechanism=posix_spawn", + "JspawnhelperProtocol", + "simulateTruncatedWriteInParent" + stage); + pb.environment().put(ENV_KEY, Integer.toString(stage)); + Process p = pb.start(); + + BufferedReader br = p.inputReader(); + String line = br.readLine(); + while (line != null && !line.startsWith("posix_spawn:")) { + System.out.println(line); + line = br.readLine(); + } + if (line == null) { + throw new Exception("Wrong output from parent process"); + } + System.out.println(line); + + if (!p.waitFor(TIMEOUT, TimeUnit.SECONDS)) { + throw new Exception("Parent process timed out"); + } + line = br.readLine(); + while (line != null) { + System.out.println(line); + line = br.readLine(); + } + + int ret = p.exitValue(); + if (ret != ERROR) { + throw new Exception("Expected exit code " + ERROR + " but got " + ret); + } + System.out.println("Parent exit code: " + ret); + } + + public static void main(String[] args) throws Exception { + // This test works as follows: + // - jtreg executes the test class `JspawnhelperProtocol` without arguments. + // This is the initial "grandparent" process. + // - For each sub-test (i.e. `normalExec()`, `simulateCrashInParent()` and + // `simulateCrashInChild()`), a new sub-process (called the "parent") will be + // forked which executes `JspawnhelperProtocol` recursively with a corresponding + // command line argument. + // - The forked `JspawnhelperProtocol` process (i.e. the "parent") runs + // `JspawnhelperProtocol::parentCode()` which forks off yet another sub-process + // (called the "child"). + // - The sub-tests in the "grandparent" check that various abnormal program + // terminations in the "parent" or the "child" process are handled gracefully and + // don't lead to deadlocks or zombie processes. + if (args.length > 0) { + // Entry point for recursive execution in the "parent" process + parentCode(args[0]); + } else { + // Main test entry for execution from jtreg + normalExec(); + simulateCrashInParent(1); + simulateCrashInParent(2); + simulateCrashInParent(3); + simulateCrashInChild(4); + simulateCrashInChild(5); + simulateCrashInChild(6); + simulateTruncatedWriteInParent(99); + } + } +}