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

[Thomas Stuefe]

On Mon, 19 May 2025 12:23:07 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.

Ping @RogerRiggs ? And @simonis, perhaps?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/25301#issuecomment-2896570218