On Mon, 19 May 2025 12:23:07 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. Looks good ------------- Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/25301#pullrequestreview-2858850942
