RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close()
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... patch (minimal alternative): http://cr.openjdk.java.net/~stuefe/webrevs/8210549-Use-FD_CLOEXEC-instead-of... See also prior discussion: http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055173.h... 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
Hi Thomas, I like the full patch 😊 +1 Best regards Christoph
-----Original Message----- From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> On Behalf Of Thomas Stüfe Sent: Dienstag, 11. September 2018 19:27 To: Java Core Libs <core-libs-dev@openjdk.java.net> Subject: RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close()
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
Thank you Christoph :) On Wed, Sep 12, 2018 at 1:47 PM, Langer, Christoph <christoph.langer@sap.com> wrote:
Hi Thomas,
I like the full patch 😊 +1
Best regards Christoph
-----Original Message----- From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> On Behalf Of Thomas Stüfe Sent: Dienstag, 11. September 2018 19:27 To: Java Core Libs <core-libs-dev@openjdk.java.net> Subject: RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close()
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
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@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... patch (minimal alternative): http://cr.openjdk.java.net/~stuefe/webrevs/8210549-Use-FD_CLOEXEC-instead-of...
See also prior discussion: http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055173.h...
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
On Wed, Sep 12, 2018 at 2:30 PM, Martin Buchholz <martinrb@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@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... patch (minimal alternative): http://cr.openjdk.java.net/~stuefe/webrevs/8210549-Use-FD_CLOEXEC-instead-of...
See also prior discussion: http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055173.h...
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
On Wed, Sep 12, 2018 at 5:46 AM, Thomas Stüfe <thomas.stuefe@gmail.com> wrote:
On Wed, Sep 12, 2018 at 2:30 PM, Martin Buchholz <martinrb@google.com> wrote:
Btw, should I retry the readdir() on EINTR?
If I read http://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html correctly, then readdir should never fail with EINTR
In 2018, we can assume everyone has implemented FD_CLOEXEC. Back in 1970, 20 file descriptors was enough for any program, but today they're effectively unlimited, so futilely closing _SC_OPEN_MAX descriptors has become unreasonable (as you say). OTOH, there is still no standard way of ensuring all our file descriptors are closed. Do we need the fallback on your beloved HP-UX? Should we simply close the first 64k file descriptors and pray, as is traditional? freebsd offers closefrom(2) https://www.freebsd.org/cgi/man.cgi?query=closefrom&sektion=2&manpath=freebs... and today we can autoconfiscate the existence of closefrom and use that where available (BSD and Solaris?) closeDescriptors needs a better name now - maybe ensureDescriptorsAreClosed ? I'd give this function a javadoc style docstring. (Finding the FD_DIR directory has become rather ugly, especially the AIX-specific code)
More concurrent discussion over in libc-alpha """system and popen fail in case of big application""" http://sourceware-org.1504.n7.nabble.com/system-and-popen-fail-in-case-of-bi... On Wed, Sep 12, 2018 at 6:17 AM, Martin Buchholz <martinrb@google.com> wrote:
In 2018, we can assume everyone has implemented FD_CLOEXEC.
Back in 1970, 20 file descriptors was enough for any program, but today they're effectively unlimited, so futilely closing _SC_OPEN_MAX descriptors has become unreasonable (as you say). OTOH, there is still no standard way of ensuring all our file descriptors are closed. Do we need the fallback on your beloved HP-UX? Should we simply close the first 64k file descriptors and pray, as is traditional?
freebsd offers closefrom(2) https://www.freebsd.org/cgi/man.cgi?query=closefrom&sektion=2&manpath=freebs... and today we can autoconfiscate the existence of closefrom and use that where available (BSD and Solaris?)
closeDescriptors needs a better name now - maybe ensureDescriptorsAreClosed ?
I'd give this function a javadoc style docstring.
(Finding the FD_DIR directory has become rather ugly, especially the AIX-specific code)
Hi Martin, sorry for letting this rest for so long - I got sidetracked by other issues. On Wed, Sep 12, 2018 at 3:17 PM Martin Buchholz <martinrb@google.com> wrote:
In 2018, we can assume everyone has implemented FD_CLOEXEC.
Back in 1970, 20 file descriptors was enough for any program, but today they're effectively unlimited, so futilely closing _SC_OPEN_MAX descriptors has become unreasonable (as you say). OTOH, there is still no standard way of ensuring all our file descriptors are closed. Do we need the fallback on your beloved HP-UX? Should we simply close the first 64k file descriptors and pray, as is traditional?
This sounds like a reasonable limit. If I understand this correctly, this only affects stray JVM/JDK file descriptors which were not opened with FD_CLOEXEC - which is an error - and third party native code. In hotspot, os::open() sets FD_CLOEXEC, and java file descriptors are set to FD_CLOEXEC too. So the error surface is quiet small nowadays. So I think giving up after 64K is okay.
freebsd offers closefrom(2) https://www.freebsd.org/cgi/man.cgi?query=closefrom&sektion=2&manpath=freebs... and today we can autoconfiscate the existence of closefrom and use that where available (BSD and Solaris?)
Here is what I think is theoretically possible: AIX: has a fcntl command which closes all file descriptors above X which is fine BSD (but apparently not MacOS): has int closefrom(2) which is fine Solaris: has a useless closefrom(3) which returns no exit code. Unless one wants to ignore the exit code, this is no option. However, Solaris has fdwalk(3), which we use in our port to walk open file descriptors and close them. But I am not sure about the merit here, this may be actually more expensive than walking the proc file system itself. ----- In any case I would suggest to keep the fallback code, which walks the proc file system, for all Unices. And also to keep the fallback from that, which closes a range brute force - possibly with a reasonable max. Is that too strict? The question is now what is more important to you: code clarity (few platform ifdefs) or trying out every technique we can think of? A) If code clarity and simplicity is important, I would basically keep the coding we have now: just walk the proc file system - including the AIX ifdefs unfortunately - and as fallback close the range with brute force until reasonable max. B) If we want to try everything, we will have more ifdefs, not less, which would look something like this: #ifdef AIX try fcntl(F_CLOSEM) #endif #if defined(_ALL_BSD_SOURCE) && !defined(APPLE) try closefrom() #endif #ifdef SOLARIS try fdwalk (?) #endif fallback: walk proc file system fallback: close range until 64K or so. ------ Not sure this is worth it... In our port I did (B) but today I personally would go with (A). (B) has too many difficult-to-test-branches. And it may be actually slower, since you may do work twice (if e.g. fdwalk fails for the same reason that manually walking the proc fs fails). What do you think?
closeDescriptors needs a better name now - maybe ensureDescriptorsAreClosed ?
I'd give this function a javadoc style docstring.
Will fix both.
(Finding the FD_DIR directory has become rather ugly, especially the AIX-specific code)
Sorry. I keep hoping that sometime in the future IBM will actually take part in this AIX port and share some of the work. And blame :( Best Regards, Thomas
"Do we need the fallback on your beloved HP-UX?" We do not ship OpenJDK on HPUX, so I do not really care. And I really do not love it :) ..Thomas On Wed, Oct 24, 2018 at 8:16 PM Thomas Stüfe <thomas.stuefe@gmail.com> wrote:
Hi Martin,
sorry for letting this rest for so long - I got sidetracked by other issues.
On Wed, Sep 12, 2018 at 3:17 PM Martin Buchholz <martinrb@google.com> wrote:
In 2018, we can assume everyone has implemented FD_CLOEXEC.
Back in 1970, 20 file descriptors was enough for any program, but today they're effectively unlimited, so futilely closing _SC_OPEN_MAX descriptors has become unreasonable (as you say). OTOH, there is still no standard way of ensuring all our file descriptors are closed. Do we need the fallback on your beloved HP-UX? Should we simply close the first 64k file descriptors and pray, as is traditional?
This sounds like a reasonable limit.
If I understand this correctly, this only affects stray JVM/JDK file descriptors which were not opened with FD_CLOEXEC - which is an error - and third party native code. In hotspot, os::open() sets FD_CLOEXEC, and java file descriptors are set to FD_CLOEXEC too.
So the error surface is quiet small nowadays. So I think giving up after 64K is okay.
freebsd offers closefrom(2) https://www.freebsd.org/cgi/man.cgi?query=closefrom&sektion=2&manpath=freebs... and today we can autoconfiscate the existence of closefrom and use that where available (BSD and Solaris?)
Here is what I think is theoretically possible:
AIX: has a fcntl command which closes all file descriptors above X which is fine
BSD (but apparently not MacOS): has int closefrom(2) which is fine
Solaris: has a useless closefrom(3) which returns no exit code. Unless one wants to ignore the exit code, this is no option. However, Solaris has fdwalk(3), which we use in our port to walk open file descriptors and close them. But I am not sure about the merit here, this may be actually more expensive than walking the proc file system itself.
-----
In any case I would suggest to keep the fallback code, which walks the proc file system, for all Unices. And also to keep the fallback from that, which closes a range brute force - possibly with a reasonable max. Is that too strict?
The question is now what is more important to you: code clarity (few platform ifdefs) or trying out every technique we can think of?
A) If code clarity and simplicity is important, I would basically keep the coding we have now: just walk the proc file system - including the AIX ifdefs unfortunately - and as fallback close the range with brute force until reasonable max.
B) If we want to try everything, we will have more ifdefs, not less, which would look something like this:
#ifdef AIX try fcntl(F_CLOSEM) #endif
#if defined(_ALL_BSD_SOURCE) && !defined(APPLE) try closefrom() #endif
#ifdef SOLARIS try fdwalk (?) #endif
fallback: walk proc file system
fallback: close range until 64K or so.
------
Not sure this is worth it...
In our port I did (B) but today I personally would go with (A). (B) has too many difficult-to-test-branches. And it may be actually slower, since you may do work twice (if e.g. fdwalk fails for the same reason that manually walking the proc fs fails).
What do you think?
closeDescriptors needs a better name now - maybe ensureDescriptorsAreClosed ?
I'd give this function a javadoc style docstring.
Will fix both.
(Finding the FD_DIR directory has become rather ugly, especially the AIX-specific code)
Sorry.
I keep hoping that sometime in the future IBM will actually take part in this AIX port and share some of the work. And blame :(
Best Regards, Thomas
By the way, there is some related work on CLOEXEC for networking that I had done. It seems like this work is somewhat similar (although my work addresses a different use case - when JNI code calls exec on its own and doesn't go through and close its inherited FDs): http://mail.openjdk.java.net/pipermail/net-dev/2018-August/011730.html "> If I understand this correctly, this only affects stray JVM/JDK file
descriptors which were not opened with FD_CLOEXEC - which is an error - and third party native code. In hotspot, os::open() sets FD_CLOEXEC, and java file descriptors are set to FD_CLOEXEC too."
Unfortunately, that is only for sockets... sockets in Linux are file descriptors as well, but OpenJDK is (currently) not opening them with CLOEXEC. Thanks, -Andrew -----Original Message----- From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> On Behalf Of Thomas Stüfe Sent: Wednesday, October 24, 2018 11:18 AM To: Martin Buchholz <martinrb@google.com> Cc: Java Core Libs <core-libs-dev@openjdk.java.net> Subject: Re: RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close() "Do we need the fallback on your beloved HP-UX?" We do not ship OpenJDK on HPUX, so I do not really care. And I really do not love it :) ..Thomas On Wed, Oct 24, 2018 at 8:16 PM Thomas Stüfe <thomas.stuefe@gmail.com> wrote:
Hi Martin,
sorry for letting this rest for so long - I got sidetracked by other issues.
On Wed, Sep 12, 2018 at 3:17 PM Martin Buchholz <martinrb@google.com> wrote:
In 2018, we can assume everyone has implemented FD_CLOEXEC.
Back in 1970, 20 file descriptors was enough for any program, but today they're effectively unlimited, so futilely closing _SC_OPEN_MAX descriptors has become unreasonable (as you say). OTOH, there is still no standard way of ensuring all our file descriptors are closed. Do we need the fallback on your beloved HP-UX? Should we simply close the first 64k file descriptors and pray, as is traditional?
This sounds like a reasonable limit.
If I understand this correctly, this only affects stray JVM/JDK file descriptors which were not opened with FD_CLOEXEC - which is an error - and third party native code. In hotspot, os::open() sets FD_CLOEXEC, and java file descriptors are set to FD_CLOEXEC too.
So the error surface is quiet small nowadays. So I think giving up after 64K is okay.
freebsd offers closefrom(2) https://www.freebsd.org/cgi/man.cgi?query=closefrom&sektion=2&manpat h=freebsd-release-ports and today we can autoconfiscate the existence of closefrom and use that where available (BSD and Solaris?)
Here is what I think is theoretically possible:
AIX: has a fcntl command which closes all file descriptors above X which is fine
BSD (but apparently not MacOS): has int closefrom(2) which is fine
Solaris: has a useless closefrom(3) which returns no exit code. Unless one wants to ignore the exit code, this is no option. However, Solaris has fdwalk(3), which we use in our port to walk open file descriptors and close them. But I am not sure about the merit here, this may be actually more expensive than walking the proc file system itself.
-----
In any case I would suggest to keep the fallback code, which walks the proc file system, for all Unices. And also to keep the fallback from that, which closes a range brute force - possibly with a reasonable max. Is that too strict?
The question is now what is more important to you: code clarity (few platform ifdefs) or trying out every technique we can think of?
A) If code clarity and simplicity is important, I would basically keep the coding we have now: just walk the proc file system - including the AIX ifdefs unfortunately - and as fallback close the range with brute force until reasonable max.
B) If we want to try everything, we will have more ifdefs, not less, which would look something like this:
#ifdef AIX try fcntl(F_CLOSEM) #endif
#if defined(_ALL_BSD_SOURCE) && !defined(APPLE) try closefrom() #endif
#ifdef SOLARIS try fdwalk (?) #endif
fallback: walk proc file system
fallback: close range until 64K or so.
------
Not sure this is worth it...
In our port I did (B) but today I personally would go with (A). (B) has too many difficult-to-test-branches. And it may be actually slower, since you may do work twice (if e.g. fdwalk fails for the same reason that manually walking the proc fs fails).
What do you think?
closeDescriptors needs a better name now - maybe ensureDescriptorsAreClosed ?
I'd give this function a javadoc style docstring.
Will fix both.
(Finding the FD_DIR directory has become rather ugly, especially the AIX-specific code)
Sorry.
I keep hoping that sometime in the future IBM will actually take part in this AIX port and share some of the work. And blame :(
Best Regards, Thomas
Sorry, I meant to say: "Unfortunately, that is only for files... sockets in Linux are file descriptors as well, but OpenJDK is (currently) not opening them with CLOEXEC." -----Original Message----- From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> On Behalf Of Andrew Luo Sent: Wednesday, October 24, 2018 1:18 PM To: Thomas Stüfe <thomas.stuefe@gmail.com>; Martin Buchholz <martinrb@google.com> Cc: Java Core Libs <core-libs-dev@openjdk.java.net> Subject: RE: RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close() By the way, there is some related work on CLOEXEC for networking that I had done. It seems like this work is somewhat similar (although my work addresses a different use case - when JNI code calls exec on its own and doesn't go through and close its inherited FDs): http://mail.openjdk.java.net/pipermail/net-dev/2018-August/011730.html "> If I understand this correctly, this only affects stray JVM/JDK file
descriptors which were not opened with FD_CLOEXEC - which is an error - and third party native code. In hotspot, os::open() sets FD_CLOEXEC, and java file descriptors are set to FD_CLOEXEC too."
Unfortunately, that is only for sockets... sockets in Linux are file descriptors as well, but OpenJDK is (currently) not opening them with CLOEXEC. Thanks, -Andrew -----Original Message----- From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> On Behalf Of Thomas Stüfe Sent: Wednesday, October 24, 2018 11:18 AM To: Martin Buchholz <martinrb@google.com> Cc: Java Core Libs <core-libs-dev@openjdk.java.net> Subject: Re: RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close() "Do we need the fallback on your beloved HP-UX?" We do not ship OpenJDK on HPUX, so I do not really care. And I really do not love it :) ..Thomas On Wed, Oct 24, 2018 at 8:16 PM Thomas Stüfe <thomas.stuefe@gmail.com> wrote:
Hi Martin,
sorry for letting this rest for so long - I got sidetracked by other issues.
On Wed, Sep 12, 2018 at 3:17 PM Martin Buchholz <martinrb@google.com> wrote:
In 2018, we can assume everyone has implemented FD_CLOEXEC.
Back in 1970, 20 file descriptors was enough for any program, but today they're effectively unlimited, so futilely closing _SC_OPEN_MAX descriptors has become unreasonable (as you say). OTOH, there is still no standard way of ensuring all our file descriptors are closed. Do we need the fallback on your beloved HP-UX? Should we simply close the first 64k file descriptors and pray, as is traditional?
This sounds like a reasonable limit.
If I understand this correctly, this only affects stray JVM/JDK file descriptors which were not opened with FD_CLOEXEC - which is an error - and third party native code. In hotspot, os::open() sets FD_CLOEXEC, and java file descriptors are set to FD_CLOEXEC too.
So the error surface is quiet small nowadays. So I think giving up after 64K is okay.
freebsd offers closefrom(2) https://www.freebsd.org/cgi/man.cgi?query=closefrom&sektion=2&manpat h=freebsd-release-ports and today we can autoconfiscate the existence of closefrom and use that where available (BSD and Solaris?)
Here is what I think is theoretically possible:
AIX: has a fcntl command which closes all file descriptors above X which is fine
BSD (but apparently not MacOS): has int closefrom(2) which is fine
Solaris: has a useless closefrom(3) which returns no exit code. Unless one wants to ignore the exit code, this is no option. However, Solaris has fdwalk(3), which we use in our port to walk open file descriptors and close them. But I am not sure about the merit here, this may be actually more expensive than walking the proc file system itself.
-----
In any case I would suggest to keep the fallback code, which walks the proc file system, for all Unices. And also to keep the fallback from that, which closes a range brute force - possibly with a reasonable max. Is that too strict?
The question is now what is more important to you: code clarity (few platform ifdefs) or trying out every technique we can think of?
A) If code clarity and simplicity is important, I would basically keep the coding we have now: just walk the proc file system - including the AIX ifdefs unfortunately - and as fallback close the range with brute force until reasonable max.
B) If we want to try everything, we will have more ifdefs, not less, which would look something like this:
#ifdef AIX try fcntl(F_CLOSEM) #endif
#if defined(_ALL_BSD_SOURCE) && !defined(APPLE) try closefrom() #endif
#ifdef SOLARIS try fdwalk (?) #endif
fallback: walk proc file system
fallback: close range until 64K or so.
------
Not sure this is worth it...
In our port I did (B) but today I personally would go with (A). (B) has too many difficult-to-test-branches. And it may be actually slower, since you may do work twice (if e.g. fdwalk fails for the same reason that manually walking the proc fs fails).
What do you think?
closeDescriptors needs a better name now - maybe ensureDescriptorsAreClosed ?
I'd give this function a javadoc style docstring.
Will fix both.
(Finding the FD_DIR directory has become rather ugly, especially the AIX-specific code)
Sorry.
I keep hoping that sometime in the future IBM will actually take part in this AIX port and share some of the work. And blame :(
Best Regards, Thomas
participants (4)
-
Andrew Luo
-
Langer, Christoph
-
Martin Buchholz
-
Thomas Stüfe