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

[Thomas Stüfe]

On Wed, Sep 12, 2018 at 2:30 PM, Martin Buchholz <martinrb at google.com> wrote:
> The reference
> from_fd + 2
> needs updating since it assumes two file descriptors have already been closed?

Good catch, I will fix that.

Btw, should I retry the readdir() on EINTR?

..Thomas

>
> 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