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

[Martin Buchholz]

The reference
from_fd + 2
needs updating since it assumes two file descriptors have already been closed?

On Tue, Sep 11, 2018 at 10:27 AM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> 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):
> http://cr.openjdk.java.net/~stuefe/webrevs/8210549-Use-FD_CLOEXEC-instead-of-close/webrev.minimal/webrev/
>
> See also prior discussion:
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055173.html
>
> Submit tests ran with full patch successfully through.
>
> ---
>
> Background:
>
> 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:
>
> <quote>
> 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.  */
> 92
> 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    }
>
> </quote>
>
> 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,
>
> Thomas