On Fri, 13 Jun 2025 06:40:29 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> 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 pull request now contains five commits: > > - Merge branch '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- > - 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 Marked as reviewed by rriggs (Reviewer). test/jdk/java/lang/ProcessBuilder/FDLeakTest/FDLeakTest.java line 59: > 57: // > 58: // What should happen: In the child process, between the initial fork > and the exec of the target binary, we should > 59: // close all filedescriptors that are not stdin/out/err. If that > works, the child process should not see any other Can you shorten the long lines to < 100 chars. It makes side-by-side reviews easier. ------------- PR Review: https://git.openjdk.org/jdk/pull/25301#pullrequestreview-2933119140 PR Review Comment: https://git.openjdk.org/jdk/pull/25301#discussion_r2150613567
