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

[Thomas Stuefe]

On Wed, 21 May 2025 19:10:49 GMT, Roger Riggs <rriggs 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.
>
> Looks good

@RogerRiggs Thank you. I hold this up a few days until JDK26 since its not super critical. Would one review be sufficient or do I need more?

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

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