RFR: 8176717: GC log file handle leaked to child processes

[Thomas Stüfe]

Hi Leo,

This seems weird.

This would affect numerous open() calls, not just this GC log, I cannot
imagine the correct fix is to change all of them.

In fact, on Posix platforms we close all file descriptors except the Pipe
ones before between fork() and exec() - see unix/native/libjava/
childproc.c.

Such code is missing on Windows - see
windows/native/libjava/ProcessImpl_md.c  . There, we do not have fork/exec,
but CreateProcess(), and whether we inherit handles or not is controlled
via an argument to CreateProcess(). But that flag is TRUE, so child
processes inherit handles.

331                    if (!CreateProcessW(
332                        NULL,             /* executable name */
333                        (LPWSTR)pcmd,     /* command line */
334                        NULL,             /* process security attribute
*/
335                        NULL,             /* thread security attribute */
336                        TRUE,             /* inherits system handles */
         <<<<<<
337                        processFlag,      /* selected based on exe type
*/
338                        (LPVOID)penvBlock,/* environment block */
339                        (LPCWSTR)pdir,    /* change to the new current
directory */
340                        &si,              /* (in)  startup information */
341                        &pi))             /* (out) process information */
342                    {
343                        win32Error(env, L"CreateProcess");
344                    }

Maybe this is the real error we should fix? Make Windows Runtime.exec
behave like the Posix variant by closing all file descriptors upon
CreateProcessW?

(This seems more of a core-libs question.)

Kind Regards, Thomas


On Mon, Mar 12, 2018 at 2:20 PM, Leo Korinth <leo.korinth at oracle.com> wrote:

> Hi,
>
> This fix is for all operating systems though the problem only seams to
> appear on windows.
>
> I am creating a proxy function for fopen (os::fopen_retain) that appends
> the non-standard "e" mode for linux and bsds. For windows the "N" mode is
> used. For other operating systems, I assume that I can use fcntl F_SETFD
> FD_CLOEXEC. I think this will work for AIX, Solaris and other operating
> systems that do not support the "e" flag. Feedback otherwise please!
>
> The reason that I use the mode "e" and not only fcntl for linux and bsds
> is threefold. First, I still need to use mode flags on windows as it does
> not support fcntl. Second, I probably save a system call. Third, the change
> will be applied directly, and there will be no point in time (between
> system calls) when the process can leak the file descriptor, so it is safer.
>
> The test case forks three VMs in a row. By doing so we know that the
> second VM is opened with a specific log file. The third VM should have less
> open file descriptors (as it is does not use logging) which is checked
> using a UnixOperatingSystemMXBean. This is not possible on windows, so on
> windows I try to rename the file, which will not work if the file is opened
> (the actual reason the bug was opened).
>
> The added test case shows that the bug fix closes the log file on windows.
> The VM on other operating systems closed the log file even before the fix.
>
> Maybe the test case should be moved to a different path?
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8176717
> https://bugs.openjdk.java.net/browse/JDK-8176809
>
> Webrev:
> http://cr.openjdk.java.net/~lkorinth/8176717/00/
>
> Testing:
> hs-tier1, hs-tier2 and TestInheritFD.java
> (on 64-bit linux, solaris, windows and mac)
>
> Thanks,
> Leo
>