Re: RFR: JDK-8271858: Handle to jimage file inherited into child processes (posix)
On Wed, 4 Aug 2021 12:22:24 GMT, Thomas Stuefe wrote: > 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.exec` but via another route since the spawnhelper closes all > file descriptors. > > --- > test: > > manually verified that lib/modules now has the FD_CLOEXEC flag set. 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. - PR: https://git.openjdk.java.net/jdk/pull/4991
Re: RFR: JDK-8271858: Handle to jimage file inherited into child processes (posix)
On Thu, 5 Aug 2021 14:06:17 GMT, Thomas Stuefe wrote: >> 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. > > 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. 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 open, up you if you want to do that or switch it to Runtime.exec. - PR: https://git.openjdk.java.net/jdk/pull/4991
Re: RFR: JDK-8271858: Handle to jimage file inherited into child processes (posix)
On Thu, 5 Aug 2021 13:13:27 GMT, Alan Bateman wrote: >> 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. > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/4991
Re: RFR: JDK-8271858: Handle to jimage file inherited into child processes (posix)
On Wed, 4 Aug 2021 17:04:38 GMT, Thomas Stuefe wrote:
>> src/java.base/unix/native/libjimage/osSupport_unix.cpp line 41:
>>
>>> 39: */
>>> 40: jint osSupport::openReadOnly(const char *path) {
>>> 41: return ::open(path, O_CLOEXEC);
>>
>> This is okay but I think it would be useful to know why this one place needs
>> O_CLOEXEC and not others.
>
> 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.
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.
-
PR: https://git.openjdk.java.net/jdk/pull/4991
Re: RFR: JDK-8271858: Handle to jimage file inherited into child processes (posix)
On Wed, 4 Aug 2021 13:35:59 GMT, Alan Bateman wrote:
>> 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.exec` but via another route since the spawnhelper closes all
>> file descriptors.
>>
>> ---
>> test:
>>
>> manually verified that lib/modules now has the FD_CLOEXEC flag set.
>
> src/java.base/unix/native/libjimage/osSupport_unix.cpp line 41:
>
>> 39: */
>> 40: jint osSupport::openReadOnly(const char *path) {
>> 41: return ::open(path, O_CLOEXEC);
>
> This is okay but I think it would be useful to know why this one place needs
> O_CLOEXEC and not others.
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.
-
PR: https://git.openjdk.java.net/jdk/pull/4991
RFR: JDK-8271858: Handle to jimage file inherited into child processes (posix)
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.exec` but via another route since the spawnhelper closes all file descriptors. --- test: manually verified that lib/modules now has the FD_CLOEXEC flag set. - Commit messages: - open image file with O_CLOEXEC Changes: https://git.openjdk.java.net/jdk/pull/4991/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4991=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8271858 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4991.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4991/head:pull/4991 PR: https://git.openjdk.java.net/jdk/pull/4991
Re: RFR: JDK-8271858: Handle to jimage file inherited into child processes (posix)
On Wed, 4 Aug 2021 12:22:24 GMT, Thomas Stuefe wrote:
> 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.exec` but via another route since the spawnhelper closes all
> file descriptors.
>
> ---
> test:
>
> manually verified that lib/modules now has the FD_CLOEXEC flag set.
src/java.base/unix/native/libjimage/osSupport_unix.cpp line 41:
> 39: */
> 40: jint osSupport::openReadOnly(const char *path) {
> 41: return ::open(path, O_CLOEXEC);
This is okay but I think it would be useful to know why this one place needs
O_CLOEXEC and not others.
-
PR: https://git.openjdk.java.net/jdk/pull/4991
