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