> Hi, please consider the following patch. > > This patch replaces the existing close-file-descriptors-logic we follow > before exec'ing a target binary: instead of explicitly closing the file > descriptors, we mark them as CLOEXEC. That simplifies the logic: it gets rid > of the awkward tiptoeing around the fact that we need to keep alive a few > file descriptors: the fail pipe fd needs to be kept open right up to the > exec(), and we cause opening internal file descriptors during our iteration > of open file handles from /proc. > > This patch also makes future developments easier: I am working on improving > logging during child process spawning > (https://bugs.openjdk.org/browse/JDK-8357100), and there we have a similar > problem where we need to keep a logfile fd open right up to the point exec() > happens). > > Note: Using fcntl() with FD_CLOEXEC should work on all our POSIX platforms, > since we rely on it already, see unconditional use of that flag here: > https://github.com/openjdk/jdk/blob/3acfa9e4e7be2f37ac55f97348aad4f74ba802a0/src/java.base/unix/native/libjava/childproc.c#L408-L409 > > This patch also fixes two subtle bugs: > - we didn't check the return value of the close() inside > closeAllFileDescriptors > - the final fcntl for the fail pipe was subtly wrong (should have or'd the > FD_CLOEXEC flag with the existing state before setting it) > > ---- > > Testing: > > We already have the PipelineLeak test, but I also added a new test that > checks that we don't accidentally leak file descriptors even if those had > been opened outside the JVM and without FD_CLOEXEC. > > - in the parent JVM, the test opens a file in native code without FD_CLOEXEC > - test then spawns a child program that checks that no file descriptors > beyond the expected stdin/out/err are open > > I verified that the test correctly detects a broken implementation that leaks > file descriptors. > > I verified that with this patch, we close all file descriptors. I also > verified the fallback path (where we brute-force-iterate all descriptors up > to _SC_OPEN_MAX). > > I ran manually all tests from test/jdk/java/base/Process*, and verified that > these tests run as part of the GHAs, which are green.
Thomas Stuefe has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Merge branch 'openjdk:master' into JDK-8210549-Runtime-exec-in-closeDescriptors-use-FD_CLOEXEC-instead-of-close- - Merge branch 'openjdk:master' into JDK-8210549-Runtime-exec-in-closeDescriptors-use-FD_CLOEXEC-instead-of-close- - close dir fd on fcntl error - Mark fds with cloexec, plus test ------------- Changes: - all: https://git.openjdk.org/jdk/pull/25301/files - new: https://git.openjdk.org/jdk/pull/25301/files/8543a5ec..aef4e2bb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25301&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25301&range=01-02 Stats: 75 lines in 3 files changed: 4 ins; 69 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/25301.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25301/head:pull/25301 PR: https://git.openjdk.org/jdk/pull/25301
