8379967: (process) Improve ProcessBuilder error reporting

make/modules/java.base/Launcher.gmk

@@ -95,7 +95,8 @@ ifeq ($(call isTargetOsType, unix), true)
       CFLAGS := $(VERSION_CFLAGS), \
       EXTRA_HEADER_DIRS := libjava, \
       EXTRA_OBJECT_FILES := \
-          $(SUPPORT_OUTPUTDIR)/native/$(MODULE)/libjava/childproc$(OBJ_SUFFIX), \
+          $(SUPPORT_OUTPUTDIR)/native/$(MODULE)/libjava/childproc$(OBJ_SUFFIX) \
+          $(SUPPORT_OUTPUTDIR)/native/$(MODULE)/libjava/childproc_errorcodes$(OBJ_SUFFIX), \
       LD_SET_ORIGIN := false, \
       OUTPUT_DIR := $(SUPPORT_OUTPUTDIR)/modules_libs/$(MODULE), \
   ))

src/java.base/unix/native/jspawnhelper/jspawnhelper.c

@@ -34,43 +34,42 @@
 #include <sys/stat.h>
 
 #include "childproc.h"
+#include "childproc_errorcodes.h"
 
 extern int errno;
 
 #define ALLOC(X,Y) { \
     void *mptr; \
     mptr = malloc (Y); \
     if (mptr == 0) { \
-        error (fdout, ERR_MALLOC); \
+        sendErrorCodeAndExit (fdout, ESTEP_JSPAWN_ALLOC_FAILED, (int)Y, errno); \
     } \
     X = mptr; \
 }
 
-#define ERR_MALLOC 1
-#define ERR_PIPE 2
-#define ERR_ARGS 3
-
 #ifndef VERSION_STRING
 #error VERSION_STRING must be defined
 #endif
 
-void error (int fd, int err) {
-    if (write (fd, &err, sizeof(err)) != sizeof(err)) {
-        /* Not sure what to do here. I have no one to speak to. */
-        exit(0x80 + err);
+/* Attempts to send an error code to the parent (which may or may not
+ * work depending on whether the fail pipe exists); then exits with an
+ * error code corresponding to the fail step. */
+void sendErrorCodeAndExit(int failpipe_fd, int step, int hint, int errno_) {
+    errcode_t errcode;
+    buildErrorCode(&errcode, step, hint, errno_);
+    if (failpipe_fd == -1 || !sendErrorCode(failpipe_fd, errcode)) {
+        /* Write error code to stdout, in the hope someone reads this. */
+        printf("jspawnhelper fail: " ERRCODE_FORMAT "\n", ERRCODE_FORMAT_ARGS(errcode));
     }
-    exit (1);
+    exit(exitCodeFromErrorCode(errcode));
 }
 
-void shutItDown() {
-    fprintf(stdout, "jspawnhelper version %s\n", VERSION_STRING);
-    fprintf(stdout, "This command is not for general use and should ");
-    fprintf(stdout, "only be run as the result of a call to\n");
-    fprintf(stdout, "ProcessBuilder.start() or Runtime.exec() in a java ");
-    fprintf(stdout, "application\n");
-    fflush(stdout);
-    _exit(1);
-}
+static const char* usageErrorText =
+    "jspawnhelper version " VERSION_STRING "\n"
+    "This command is not for general use and should "
+    "only be run as the result of a call to\n"
+    "ProcessBuilder.start() or Runtime.exec() in a java "
+    "application\n";
 
 /*
  * read the following off the pipefd
@@ -84,31 +83,41 @@ void initChildStuff (int fdin, int fdout, ChildStuff *c) {
     int bufsize, offset=0;
     int magic;
     int res;
+    const int step = ESTEP_JSPAWN_RCV_CHILDSTUFF_COMM_FAIL;
+    int substep = 0;
 
     res = readFully (fdin, &magic, sizeof(magic));
-    if (res != 4 || magic != magicNumber()) {
-        error (fdout, ERR_PIPE);
+    if (res != 4) {
+        sendErrorCodeAndExit(fdout, step, substep, errno);
+    }
+
+    substep ++;
+    if (magic != magicNumber()) {
+        sendErrorCodeAndExit(fdout, step, substep, errno);
     }
 
 #ifdef DEBUG
     jtregSimulateCrash(0, 5);
 #endif
 
+    substep ++;
     if (readFully (fdin, c, sizeof(*c)) != sizeof(*c)) {
-        error (fdout, ERR_PIPE);
+        sendErrorCodeAndExit(fdout, step, substep, errno);
     }
 
+    substep ++;
     if (readFully (fdin, &sp, sizeof(sp)) != sizeof(sp)) {
-        error (fdout, ERR_PIPE);
+        sendErrorCodeAndExit(fdout, step, substep, errno);
     }
 
     bufsize = sp.argvBytes + sp.envvBytes +
               sp.dirlen + sp.parentPathvBytes;
 
     ALLOC(buf, bufsize);
 
+    substep++;
     if (readFully (fdin, buf, bufsize) != bufsize) {
-        error (fdout, ERR_PIPE);
+        sendErrorCodeAndExit(fdout, step, substep, errno);
     }
 
     /* Initialize argv[] */
@@ -150,25 +159,29 @@ int main(int argc, char *argv[]) {
 #endif
 
     if (argc != 3) {
-        fprintf(stdout, "Incorrect number of arguments: %d\n", argc);
-        shutItDown();
+        printf("Incorrect number of arguments: %d\n", argc);
+        puts(usageErrorText);
+        sendErrorCodeAndExit(-1, ESTEP_JSPAWN_ARG_ERROR, 0, 0);
     }
 
     if (strcmp(argv[1], VERSION_STRING) != 0) {
-        fprintf(stdout, "Incorrect Java version: %s\n", argv[1]);
-        shutItDown();
+        printf("Incorrect Java version: %s\n", argv[1]);
+        puts(usageErrorText);
+        sendErrorCodeAndExit(-1, ESTEP_JSPAWN_VERSION_ERROR, 0, 0);
     }
 
     r = sscanf (argv[2], "%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)) {
-            fprintf(stdout, "Incorrect input pipe\n");
-            shutItDown();
+            printf("Incorrect input pipe\n");
+            puts(usageErrorText);
+            sendErrorCodeAndExit(-1, ESTEP_JSPAWN_NOT_A_PIPE, fdinr, errno);
         }
     } else {
-        fprintf(stdout, "Incorrect FD array data: %s\n", argv[2]);
-        shutItDown();
+        printf("Incorrect FD array data: %s\n", argv[2]);
+        puts(usageErrorText);
+        sendErrorCodeAndExit(-1, ESTEP_JSPAWN_NOT_A_PIPE, fdinr, errno);
     }
 
     // Close the writing end of the pipe we use for reading from the parent.

src/java.base/unix/native/libjava/ProcessImpl_md.c

@@ -47,6 +47,7 @@
 #include <spawn.h>
 
 #include "childproc.h"
+#include "childproc_errorcodes.h"
 
 /*
  *
@@ -678,7 +679,6 @@ Java_java_lang_ProcessImpl_forkAndExec(JNIEnv *env,
                                        jintArray std_fds,
                                        jboolean redirectErrorStream)
 {
-    int errnum;
     int resultPid = -1;
     int in[2], out[2], err[2], fail[2], childenv[2];
     jint *fds = NULL;
@@ -779,23 +779,27 @@ Java_java_lang_ProcessImpl_forkAndExec(JNIEnv *env,
     }
     close(fail[1]); fail[1] = -1; /* See: WhyCantJohnnyExec  (childproc.c)  */
 
+    errcode_t errcode;
+
     /* If we expect the child to ping aliveness, wait for it. */
     if (c->sendAlivePing) {
-        switch(readFully(fail[0], &errnum, sizeof(errnum))) {
+        switch(readFully(fail[0], &errcode, sizeof(errcode))) {
         case 0: /* First exec failed; */
             {
                 int tmpStatus = 0;
                 int p = waitpid(resultPid, &tmpStatus, 0);
                 throwExitCause(env, p, tmpStatus, c->mode);
                 goto Catch;
             }
-        case sizeof(errnum):
-            if (errnum != CHILD_IS_ALIVE) {
-                /* This can happen if the spawn helper encounters an error
-                 * before or during the handshake with the parent. */
-                throwInternalIOException(env, 0,
-                                         "Bad code from spawn helper (Failed to exec spawn helper)",
-                                         c->mode);
+        case sizeof(errcode):
+            if (errcode.step != ESTEP_CHILD_ALIVE) {
+                /* This can happen if the child process encounters an error
+                 * before or during initial handshake with the parent. */
+                char msg[256];
+                snprintf(msg, sizeof(msg),
+                         "Bad early code from spawn helper " ERRCODE_FORMAT " (Failed to exec spawn helper)",
+                         ERRCODE_FORMAT_ARGS(errcode));
+                throwInternalIOException(env, 0, msg, c->mode);
                 goto Catch;
             }
             break;
@@ -805,11 +809,29 @@ Java_java_lang_ProcessImpl_forkAndExec(JNIEnv *env,
         }
     }
 
-    switch (readFully(fail[0], &errnum, sizeof(errnum))) {
+    switch (readFully(fail[0], &errcode, sizeof(errcode))) {
     case 0: break; /* Exec succeeded */
-    case sizeof(errnum):
+    case sizeof(errcode):
+        /* Always reap first! */
         waitpid(resultPid, NULL, 0);
-        throwIOException(env, errnum, "Exec failed");
+        /* Most of these errors are implementation errors and should result in an internal IOE, but
+         * a few can be caused by bad user input and need to be communicated to the end user. */
+        switch(errcode.step) {
+        case ESTEP_CHDIR_FAIL:
+            throwIOException(env, errcode.errno_, "Failed to access working directory");
+            break;
+        case ESTEP_EXEC_FAIL:
+            throwIOException(env, errcode.errno_, "Exec failed");
+            break;
+        default: {
+            /* Probably implementation error */
+            char msg[256];
+            snprintf(msg, sizeof(msg),
+                     "Bad code from spawn helper " ERRCODE_FORMAT " (Failed to exec spawn helper)",
+                     ERRCODE_FORMAT_ARGS(errcode));
+            throwInternalIOException(env, 0, msg, c->mode);
+        }
+        };
         goto Catch;
     default:
         throwInternalIOException(env, errno, "Read failed", c->mode);