8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close()

Reviewed-by: rriggs

Author: tstuefe

make/test/JtregNativeJdk.gmk

@@ -62,7 +62,7 @@ BUILD_JDK_JTREG_LIBRARIES_JDK_LIBS_libGetXSpace := java.base:libjava
 ifeq ($(call isTargetOs, windows), true)
   BUILD_JDK_JTREG_EXCLUDE += libDirectIO.c libInheritedChannel.c \
       libExplicitAttach.c libImplicitAttach.c \
-      exelauncher.c
+      exelauncher.c libFDLeaker.c exeFDLeakTester.c
 
   BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeNullCallerTest := $(LIBCXX)
   BUILD_JDK_JTREG_EXECUTABLES_LIBS_exerevokeall := advapi32.lib

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

@@ -52,6 +52,21 @@ closeSafely(int fd)
     return (fd == -1) ? 0 : close(fd);
 }
 
+int
+markCloseOnExec(int fd)
+{
+    const int flags = fcntl(fd, F_GETFD);
+    if (flags < 0) {
+        return -1;
+    }
+    if ((flags & FD_CLOEXEC) == 0) {
+        if (fcntl(fd, F_SETFD, flags | FD_CLOEXEC) < 0) {
+            return -1;
+        }
+    }
+    return 0;
+}
+
 static int
 isAsciiDigit(char c)
 {
@@ -68,21 +83,15 @@ isAsciiDigit(char c)
 #endif
 
 static int
-closeDescriptors(void)
+markDescriptorsCloseOnExec(void)
 {
     DIR *dp;
     struct dirent *dirp;
-    int from_fd = FAIL_FILENO + 1;
-
-    /* We're trying to close all file descriptors, but opendir() might
-     * itself be implemented using a file descriptor, and we certainly
-     * don't want to close that while it's in use.  We assume that if
-     * opendir() is implemented using a file descriptor, then it uses
-     * the lowest numbered file descriptor, just like open().  So we
-     * close a couple explicitly.  */
-
-    close(from_fd);          /* for possible use by opendir() */
-    close(from_fd + 1);      /* another one for good luck */
+    /* This function marks all file descriptors beyond stderr as CLOEXEC.
+     * That includes the file descriptor used for the fail pipe: we want that
+     * one to stay open up until the execve, but it should be closed with the
+     * execve. */
+    const int fd_from = STDERR_FILENO + 1;
 
 #if defined(_AIX)
     /* AIX does not understand '/proc/self' - it requires the real process ID */
@@ -91,18 +100,22 @@ closeDescriptors(void)
 #endif
 
     if ((dp = opendir(FD_DIR)) == NULL)
-        return 0;
+        return -1;
 
     while ((dirp = readdir(dp)) != NULL) {
         int fd;
         if (isAsciiDigit(dirp->d_name[0]) &&
-            (fd = strtol(dirp->d_name, NULL, 10)) >= from_fd + 2)
-            close(fd);
+            (fd = strtol(dirp->d_name, NULL, 10)) >= fd_from) {
+            if (markCloseOnExec(fd) == -1) {
+                closedir(dp);
+                return -1;
+            }
+        }
     }
 
     closedir(dp);
 
-    return 1;
+    return 0;
 }
 
 static int
@@ -394,11 +407,11 @@ childProcess(void *arg)
     fail_pipe_fd = FAIL_FILENO;
 
     /* close everything */
-    if (closeDescriptors() == 0) { /* failed,  close the old way */
+    if (markDescriptorsCloseOnExec() == -1) { /* failed,  close the old way */
         int max_fd = (int)sysconf(_SC_OPEN_MAX);
         int fd;
-        for (fd = FAIL_FILENO + 1; fd < max_fd; fd++)
-            if (close(fd) == -1 && errno != EBADF)
+        for (fd = STDERR_FILENO + 1; fd < max_fd; fd++)
+            if (markCloseOnExec(fd) == -1 && errno != EBADF)
                 goto WhyCantJohnnyExec;
     }
 
@@ -413,9 +426,6 @@ childProcess(void *arg)
         sigprocmask(SIG_SETMASK, &unblock_signals, NULL);
     }
 
-    if (fcntl(FAIL_FILENO, F_SETFD, FD_CLOEXEC) == -1)
-        goto WhyCantJohnnyExec;
-
     JDK_execvpe(p->mode, p->argv[0], p->argv, p->envv);
 
  WhyCantJohnnyExec:

test/jdk/java/lang/ProcessBuilder/FDLeakTest/FDLeakTest.java

@@ -0,0 +1,72 @@
+/*
+ * Copyright (c) 2025, 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 id=posix_spawn
+ * @summary Check that we don't leak FDs
+ * @requires os.family != "windows"
+ * @library /test/lib
+ * @run main/othervm/native -Djdk.lang.Process.launchMechanism=posix_spawn -agentlib:FDLeaker FDLeakTest
+ */
+
+/**
+ * @test id=fork
+ * @summary Check that we don't leak FDs
+ * @requires os.family != "windows"
+ * @library /test/lib
+ * @run main/othervm/native -Djdk.lang.Process.launchMechanism=fork -agentlib:FDLeaker FDLeakTest
+ */
+
+/**
+ * @test id=vfork
+ * @summary Check that we don't leak FDs
+ * @requires os.family == "linux"
+ * @library /test/lib
+ * @run main/othervm/native -Djdk.lang.Process.launchMechanism=vfork -agentlib:FDLeaker FDLeakTest
+ */
+
+import jdk.test.lib.process.ProcessTools;
+public class FDLeakTest {
+    // This test has two native parts:
+    // - a library invoked with -agentlib that ensures that, in the parent JVM, we open
+    //   a native fd without setting FD_CLOEXEC (libFDLeaker.c). This is necessary because
+    //   there is no way to do this from Java: if Java functions correctly, all files the
+    //   user could open via its APIs should be marked with FD_CLOEXEC.
+    // - a small native executable that tests - without using /proc - whether any file
+    //   descriptors other than stdin/out/err are open.
+    //
+    // What should happen: In the child process, between the initial fork and the exec of
+    // the target binary, we should close all filedescriptors that are not stdin/out/err.
+    // If that works, the child process should not see any other file descriptors save
+    // those three.
+    public static void main(String[] args) throws Exception {
+        ProcessBuilder pb = ProcessTools.createNativeTestProcessBuilder("FDLeakTester");
+        pb.inheritIO();
+        Process p = pb.start();
+        p.waitFor();
+        if (p.exitValue() != 0) {
+            throw new RuntimeException("Failed");
+        }
+    }
+}

test/jdk/java/lang/ProcessBuilder/FDLeakTest/exeFDLeakTester.c

@@ -0,0 +1,56 @@
+/*
+ * Copyright (c) 2021, 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.
+ */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+
+/* Check if any fd past stderr is valid; if true, print warning on stderr and return -1
+ *
+ * Note: check without accessing /proc since:
+ * - non-portable
+ * - may cause creation of temporary file descriptors
+ */
+int main(int argc, char** argv) {
+    int errors = 0;
+    int rc = 0;
+    char buf[128];
+    int max_fd = (int)sysconf(_SC_OPEN_MAX);
+    if (max_fd == -1) {
+        snprintf(buf, sizeof(buf), "*** sysconf(_SC_OPEN_MAX) failed? (%d) ***\n", errno);
+        rc = write(2, buf, strlen(buf));
+        max_fd = 10000;
+    }
+    // We start after stderr fd
+    for (int fd = 3; fd < max_fd; fd++) {
+        if (fcntl(fd, F_GETFD, 0) >= 0) {
+            // Error: found valid file descriptor
+            errors++;
+            snprintf(buf, sizeof(buf), "*** Parent leaked file descriptor %d ***\n", fd);
+            rc = write(2, buf, strlen(buf));
+        }
+    }
+    return errors > 0 ? -1 : 0;
+}

test/jdk/java/lang/ProcessBuilder/FDLeakTest/libFDLeaker.c

@@ -0,0 +1,36 @@
+/*
+ * Copyright (c) 2025, 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.
+ */
+
+#include <stdio.h>
+#include "jvmti.h"
+
+JNIEXPORT jint JNICALL
+Agent_OnLoad(JavaVM *jvm, char *options, void *reserved) {
+  const char* filename = "./testfile_FDLeaker.txt";
+  FILE* f = fopen(filename, "w");
+  if (f == NULL) {
+    return JNI_ERR;
+  }
+  printf("Opened and leaked %s (%d)", filename, fileno(f));
+  return JNI_OK;
+}