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

[Roger Riggs]

On Fri, 13 Jun 2025 06:40:29 GMT, Thomas Stuefe <stuefe at 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