8361873: [GCC static analyzer] exec_md.c forkedChildProcess potential double 'close' of file descriptor '3'

[plummercj]

Fix gcc warning about double close. forkedChildProcess() calls closeDescriptors(), which always closes STDERR_FILENO + 1 and STDERR_FILENO + 2. However, if it fails at some point after that, then forkedChildProcess() will attempt to close these same two files again. Fixed by making forkedChildProcess() no longer attempt to close these two files.

I first verified the warning by configuring with --with-extra-cflags=-fanalyzer, and then verified the fix by doing the same.

Tested by running jdi nsk test. I'll add some more CI testing.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8361873: [GCC static analyzer] exec_md.c forkedChildProcess potential double 'close' of file descriptor '3' (Bug - P4)

Reviewers

• Jaikiran Pai (@jaikiran - Reviewer)
• Thomas Stuefe (@tstuefe - Reviewer) Review applies to 4095d6ce
• Matthias Baesken (@MBaesken - Reviewer) Review applies to 4095d6ce

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26487/head:pull/26487
$ git checkout pull/26487

Update a local copy of the PR:
$ git checkout pull/26487
$ git pull https://git.openjdk.org/jdk.git pull/26487/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26487

View PR using the GUI difftool:
$ git pr show -t 26487

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26487.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back cjplummer! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@plummercj This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8361873: [GCC static analyzer] exec_md.c forkedChildProcess potential double 'close' of file descriptor '3'

Reviewed-by: jpai, stuefe, mbaesken

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 16 new commits pushed to the master branch:

• 70ebb5e: 8364019: Add alignment precondition to Universe::reserve_heap
• b4028c9: 8362482: [TESTBUG] serviceability/HeapDump/UnmountedVThreadNativeMethodAtTop.java: System.gc() does not provide full GC
• 08b0b34: 8361715: (bf) Improve java/nio/Buffer/GetChars.java and migrate to JUnit
• ... and 13 more: https://git.openjdk.org/jdk/compare/d288ca28be7bfba3abe9f54cefbe53e73c25707e...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@plummercj The following label will be automatically applied to this pull request:

• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 02: Full - Incremental (49ca5abb)
• 01: Full - Incremental (4095d6ce)
• 00: Full (088932e9)

[jaikiran]

Hello Chris, would it be better to word this as "We are not allowed to return from this function until after ...."?

[plummercj]

The issue is returning with a failure, not simply returning early, although it should only ever return early if there is a failure. I can change "not allowed to fail" with "not allowed to return with a failure"

[jaikiran]

I can change "not allowed to fail" with "not allowed to return with a failure"

That sounds good to me.

[jaikiran]

This looks reasonable to me. I just have trivial comment about the code comment introduced in this change, which I've added inline.

[tstuefe]

Looks good to me.

Note that in the official Runtime.exec layer over in libjava, we switched to setting the filedescriptors to FD_CLOEXEC, which seems more elegant and circumvents this and some other problems.

[plummercj]

Note that in the official Runtime.exec layer over in libjava, we switched to setting the filedescriptors to FD_CLOEXEC, which seems more elegant and circumvents this and some other problems.

Are you talking about what is being done in os::open()? What about opens done that don't go through os::open() such as opens from app native code?

In any case, I think this is beyond the scope of this PR. Maybe an RFE could be filed for it if you think it is worth doing.

[jaikiran]

Thank you Chris for the update. This looks good to me.

[tstuefe]

Note that in the official Runtime.exec layer over in libjava, we switched to setting the filedescriptors to FD_CLOEXEC, which seems more elegant and circumvents this and some other problems.

Are you talking about what is being done in os::open()? What about opens done that don't go through os::open() such as opens from app native code?

No, I am talking about this:

jdk/src/java.base/unix/native/libjava/childproc.c

Line 109 in 4669005

if (markCloseOnExec(fd) == -1) {

where we close all file descriptors apart from the ones we need to communicate to the jspawnhelper.

In any case, I think this is beyond the scope of this PR. Maybe an RFE could be filed for it if you think it is worth doing.

Sure thing