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

Thomas Stüfe thomas.stuefe at gmail.com
Tue Sep 11 17:27:13 UTC 2018

Hi all,

May I please have reviews for this small fix:

Bug: https://bugs.openjdk.java.net/browse/JDK-8210549

patch (full): http://cr.openjdk.java.net/~stuefe/webrevs/8210549-Use-FD_CLOEXEC-instead-of-close/webrev.00/webrev/
patch (minimal alternative):

See also prior discussion:

Submit tests ran with full patch successfully through.



After fork()/vfork() and before exec(), the child process needs to
close all inherited file descriptors apart from the stdin/out/err pipe
ends. We do this by iterating thru all file descriptors in
/proc/<pid>/fd or whatever the equivalent is on that platform. This is
done using opendir(), readdir().

We then close all these file descriptors using close().

The problem with that technique is that we may accidentally close the
file descriptor opendir() is using internally to read the directory
content of /proc/<pid>/fd, thereby sawing off the branch we are
sitting on. The coding does some magic to attempt to prevent this:

86    /* We're trying to close all file descriptors, but opendir() might
87     * itself be implemented using a file descriptor, and we certainly
88     * don't want to close that while it's in use.  We assume that if
89     * opendir() is implemented using a file descriptor, then it uses
90     * the lowest numbered file descriptor, just like open().  So we
91     * close a couple explicitly.  */
93    close(from_fd);          /* for possible use by opendir() */
94    close(from_fd + 1);      /* another one for good luck */

108    while ((dirp = readdir64(dp)) != NULL) {
109        int fd;
110        if (isAsciiDigit(dirp->d_name[0]) &&
111            (fd = strtol(dirp->d_name, NULL, 10)) >= from_fd +
2)Improve "close all filedescriptors" coding.
112            close(fd);
113    }


This workaround can be removed if, instead of outright closing the
file descriptor in the loop, we were to set the file descriptor to
FD_CLOEXEC. Closing all descriptors would be defered to the actual
exec() call some milliseconds later.


The patch changes the close() call to fcntl(FD_CLOEXEC). This removes
the need for the workaround as described.

In addition to that, the full version of the patch also adds error
handling to the readdir() loop.

But note that this exposes us to a behavioral change should we run
into a readdir() error:

- currently, we would leave the remaining file descriptors quietly
unclosed. This usually would have no effect, but in rare cases may
lead to difficult-to-analyze errors when child processes accidentally
disturb parent process IO.

- With this patch, we will fail if readdir fails. However, we do not
just yet fail the Runtime.exec() call, but enter a fallback code
section here:

364     /* close everything */
365     if (closeDescriptors() == 0) { /* failed,  close the old way */
366         int max_fd = (int)sysconf(_SC_OPEN_MAX);
367         int fd;
368         for (fd = FAIL_FILENO + 1; fd < max_fd; fd++)
369             if (close(fd) == -1 && errno != EBADF)
370                 goto WhyCantJohnnyExec;
371     }

I am not convinced this is a good fallback. _SC_OPEN_MAX depends on
ulimit -Hn and may be high. On my Ubuntu 16.4 box, default value after
installation is 1048576. When we hit this fallback code, it takes
almost a full second to spawn a single child process. In fork
intensive scenarios this can get pretty bad.

What do you think?

Thanks & Best Regards,


More information about the core-libs-dev mailing list