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

[Thomas Stuefe]

> 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 incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:

 - 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

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

Changes:
  - all: https://git.openjdk.org/jdk/pull/25301/files
  - new: https://git.openjdk.org/jdk/pull/25301/files/8543a5ec..aef4e2bb

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=25301&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25301&range=01-02

  Stats: 75 lines in 3 files changed: 4 ins; 69 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/25301.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/25301/head:pull/25301

PR: https://git.openjdk.org/jdk/pull/25301