JDK-8271858: Handle to jimage file inherited into child processes (posix) #4991
Conversation
|
👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into |
|
@tstuefe To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command. Applicable Labels
|
| @@ -38,7 +38,7 @@ | |||
| * Return the file descriptor. | |||
| */ | |||
| jint osSupport::openReadOnly(const char *path) { | |||
| return ::open(path, 0); | |||
| return ::open(path, O_CLOEXEC); | |||
There was a problem hiding this comment.
This is okay but I think it would be useful to know why this one place needs O_CLOEXEC and not others.
There was a problem hiding this comment.
Probably others too, if we care about inheriting read handles to child.
The background is that I am playing with a new test which checks that the VM has no open inheritable file descriptors. It is supposed to replace some specialized tests which are maintenance-intensive. I am still tuning the test, since at the moment it turns out too many false positives.
Anyway, this is the very first descriptor the VM opens, therefore it triggered my test. I thought since there is a sibling fix for windows, a similar fix for posix would be symmetric.
If you think this is not worth fixing, or we should fix all occurrences in one swoop, that is fine too.
There was a problem hiding this comment.
On Unix systems, the JDK has always relied on the Runtime.exec implementation to close the file descriptors. On Windows the inheritance has to be disabled in the parent. So if the gtest launcher is forking directly then we may have a bit of whack-a-mole to change all the places that open file descriptors.
There was a problem hiding this comment.
My idea was born since we have jtreg tests that assert that certain VM internal fds, namely of log files, are not handed down to child processes. The motivation originally was Windows, since child processes would block that file from being moved. The test is done for both Unix and Windows, however. It is fragile and difficult to analyze when it fails. I wanted to throw away the Unix portion of that test and replace it with a simple gtest, either checking CLOEXEC for just that particular fd, or (my current approach) for all handles.
But if what you are saying is how we do things - we don't bother with CLOEXEC, just rely on Runtime.exec() to close all fds, and accept the fact that "foreign" forks will cause us problems - then I could just throw the Unix portion of that test away without replacing it with anything.
ATM the libs/module fd seems to be the only file descriptor tripping up my test though. Maybe it's not so bad. I am mainly afraid of situations where the gtestlauncher runs in some instrumented form, maybe with a virus scanner in its belly, with foreign code opening fds without our knowledge. I am still unsure about which direction to go.
There was a problem hiding this comment.
lib/modules is opened/mapped early in the startup so I assume this is why this one shows up quickly. Adding O_CLOEXEC everywhere is okay, up you if you want to do that or switch it to Runtime.exec.
|
/label add core-libs |
|
@AlanBateman |
|
I withdraw the PR and close the issue as won't fix since the issue is very unlikely to cause real problems (only in the event of someone raw-forking, bypassing Runtime.exec()). I'll find a narrower solution to simplify the test. |
tstuefe
deleted the
JDK-8271858-Handle-to-jimage-file-inherited-into-child-processes-posix
branch
We should not leak the handle to the jimage file (lib/modules) to childs.
JDK-8194734 did solve this for windows. We should use FD_CLOEXEC on Posix as well.
Note that this only poses a problem when a child process is spawned off not via
Runtime.execbut via another route since the spawnhelper closes all file descriptors.test:
manually verified that lib/modules now has the FD_CLOEXEC flag set.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4991/head:pull/4991$ git checkout pull/4991Update a local copy of the PR:
$ git checkout pull/4991$ git pull https://git.openjdk.java.net/jdk pull/4991/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4991View PR using the GUI difftool:
$ git pr show -t 4991Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4991.diff