Re: 8194734 : Handle to jimage file inherited into child processes (win)

2018-04-17 Thread Alexander Miloslavskiy
Alan, thanks a lot for applying my patch! Now, do I need to try and remove the magic from Unix process creation code? Or you'd rather have it untouched?

Re: 8194734 : Handle to jimage file inherited into child processes (win)

2018-04-12 Thread Alexander Miloslavskiy
I'm still puzzled by the Unix change. I completely agree with Martin about CLOSEXEC but there is something fishy here that we need to get a handle on. When we fork then we close >= 3 in the child so there shouldn't be any magic with fd 3. The current code closes >= 4. FD 3 is a special one

Re: 8194734 : Handle to jimage file inherited into child processes (win)

2018-04-12 Thread Alexander Miloslavskiy
Please get Windows change applied. I'm mostly interested in this part, because bug is currently hidden on Unix. I'm still puzzled by the Unix change. I completely agree with Martin about CLOSEXEC but there is something fishy here that we need to get a handle on. When we fork then we close >=

Re: 8194734 : Handle to jimage file inherited into child processes (win)

2018-04-12 Thread Alan Bateman
On 12/04/2018 18:38, Alexander Miloslavskiy wrote: I still need someone to get this patch applied. Please? At this moment, it seems that patch is validated: > Martin Buchholz > But regardless of that, we should try to set the CLOEXEC bit on all > our file descriptor > Alan Bateman > The

Re: 8194734 : Handle to jimage file inherited into child processes (win)

2018-04-12 Thread Alexander Miloslavskiy
I still need someone to get this patch applied. Please? At this moment, it seems that patch is validated: > Martin Buchholz > But regardless of that, we should try to set the CLOEXEC bit on all > our file descriptor > Alan Bateman > The update to osSupport_Windows.cpp looks okay.

Re: 8194734 : Handle to jimage file inherited into child processes (win)

2018-04-08 Thread Alexander Miloslavskiy
> I'm hoping that JDK core libraries overall do a better job of reusing > the code in e.g. > src/hotspot/os/linux/os_linux:os::open > somehow, that actually gets the CLOEXEC bit right. os::open is exactly what was used in libjimage before the bug was introduced. I already described it in my

Re: 8194734 : Handle to jimage file inherited into child processes (win)

2018-04-08 Thread Martin Buchholz
I'm hoping that JDK core libraries overall do a better job of reusing the code in e.g. src/hotspot/os/linux/os_linux:os::open somehow, that actually gets the CLOEXEC bit right. On Sun, Apr 8, 2018 at 11:56 AM, Alexander Miloslavskiy < alexandr.miloslavs...@gmail.com> wrote: > I'm still only half

Re: 8194734 : Handle to jimage file inherited into child processes (win)

2018-04-08 Thread Alexander Miloslavskiy
I'm still only half paying attention, but I agree with Alexander that the low-level infrastructure in libjimage is fishy.  The rule should be that file descriptors created by the JDK should have the CLOEXEC bit on. But even if the CLOEXEC bit was forgotten, the subprocess code should still

Re: 8194734 : Handle to jimage file inherited into child processes (win)

2018-04-08 Thread Martin Buchholz
On Mon, Apr 2, 2018 at 2:50 PM, Alexander Miloslavskiy < alexandr.miloslavs...@gmail.com> wrote: > The problem in this bug is that jimage file is mistakenly opened with > "inherit by child processes" flag. In our case, the child process is > started with Runtime.exec() and serves as updater that

Re: 8194734 : Handle to jimage file inherited into child processes (win)

2018-04-08 Thread Alexander Miloslavskiy
> The only surprise is that it doesn't close as it shouldn't need the > file descriptor after it mmapped. Indeed, both on Windows and Unix file descriptor can be closed after mapping. On the other hand, ImageFileReader is not prepared for that. See

Re: 8194734 : Handle to jimage file inherited into child processes (win)

2018-04-08 Thread Alexander Miloslavskiy
> STDERR_FILENO is 2 so I'm curious which error reporting pipe is bring > discussed here. We're discussing line 365 of childproc.c http://hg.openjdk.java.net/jdk/jdk/file/8bdf2b5f472d/src/java.base/unix/native/libjava/childproc.c

Re: 8194734 : Handle to jimage file inherited into child processes (win)

2018-04-08 Thread Alan Bateman
On 06/04/2018 22:01, Martin Buchholz wrote: History: The design on Unix was that all file descriptors greater than 3 are closed on fork and before exec. But regardless of that, we should try to set the CLOEXEC bit on all our file descriptor (who knows, there might be native code that does

Re: 8194734 : Handle to jimage file inherited into child processes (win)

2018-04-06 Thread Martin Buchholz
History: The design on Unix was that all file descriptors greater than 3 are closed on fork and before exec. But regardless of that, we should try to set the CLOEXEC bit on all our file descriptor (who knows, there might be native code that does fork+exec). File descriptor 3 is indeed deep magic,

Re: 8194734 : Handle to jimage file inherited into child processes (win)

2018-04-06 Thread Alexander Miloslavskiy
> The update to osSupport_Windows.cpp looks okay. I think it would be > useful to discuss the osSupport_unix.cpp change further as the > childproc code is supported to close the file descriptors. childproc code uses some dark magic to figure which files to close. At the moment,

Re: 8194734 : Handle to jimage file inherited into child processes (win)

2018-04-06 Thread Alan Bateman
On 02/04/2018 22:50, Alexander Miloslavskiy wrote: The problem in this bug is that jimage file is mistakenly opened with "inherit by child processes" flag. In our case, the child process is started with Runtime.exec() and serves as updater that can also replace embedded JRE. However, due to

8194734 : Handle to jimage file inherited into child processes (win)

2018-04-05 Thread Alexander Miloslavskiy
The problem in this bug is that jimage file is mistakenly opened with "inherit by child processes" flag. In our case, the child process is started with Runtime.exec() and serves as updater that can also replace embedded JRE. However, due to jimage ("lib/modules") file handle being inherited,