8176717: GC log file handle leaked to child processes

Reviewed-by: stuefe, rehn

src/hotspot/share/logging/logFileOutput.cpp

@@ -245,7 +245,7 @@ bool LogFileOutput::initialize(const char* options, outputStream* errstream) {
     increment_file_count();
   }
 
-  _stream = fopen(_file_name, FileOpenMode);
+  _stream = os::fopen(_file_name, FileOpenMode);
   if (_stream == NULL) {
     errstream->print_cr("Error opening log file '%s': %s",
                         _file_name, strerror(errno));
@@ -334,7 +334,7 @@ void LogFileOutput::rotate() {
   archive();
 
   // Open the active log file using the same stream as before
-  _stream = fopen(_file_name, FileOpenMode);
+  _stream = os::fopen(_file_name, FileOpenMode);
   if (_stream == NULL) {
     jio_fprintf(defaultStream::error_stream(), "Could not reopen file '%s' during log rotation (%s).\n",
                 _file_name, os::strerror(errno));

src/hotspot/share/runtime/os.cpp

@@ -1251,6 +1251,33 @@ char* os::format_boot_path(const char* format_string,
     return formatted_path;
 }
 
+// This function is a proxy to fopen, it tries to add a non standard flag ('e' or 'N')
+// that ensures automatic closing of the file on exec. If it can not find support in
+// the underlying c library, it will make an extra system call (fcntl) to ensure automatic
+// closing of the file on exec.
+FILE* os::fopen(const char* path, const char* mode) {
+  char modified_mode[20];
+  assert(strlen(mode) + 1 < sizeof(modified_mode), "mode chars plus one extra must fit in buffer");
+  sprintf(modified_mode, "%s" LINUX_ONLY("e") BSD_ONLY("e") WINDOWS_ONLY("N"), mode);
+  FILE* file = ::fopen(path, modified_mode);
+
+#if !(defined LINUX || defined BSD || defined _WINDOWS)
+  // assume fcntl FD_CLOEXEC support as a backup solution when 'e' or 'N'
+  // is not supported as mode in fopen
+  if (file != NULL) {
+    int fd = fileno(file);
+    if (fd != -1) {
+      int fd_flags = fcntl(fd, F_GETFD);
+      if (fd_flags != -1) {
+        fcntl(fd, F_SETFD, fd_flags | FD_CLOEXEC);
+      }
+    }
+  }
+#endif
+
+  return file;
+}
+
 bool os::set_boot_path(char fileSep, char pathSep) {
   const char* home = Arguments::get_java_home();
   int home_len = (int)strlen(home);

src/hotspot/share/runtime/os.hpp

@@ -551,6 +551,7 @@ class os: AllStatic {
   static const int default_file_open_flags();
   static int open(const char *path, int oflag, int mode);
   static FILE* open(int fd, const char* mode);
+  static FILE* fopen(const char* path, const char* mode);
   static int close(int fd);
   static jlong lseek(int fd, jlong offset, int whence);
   static char* native_path(char *path);

test/hotspot/jtreg/runtime/8176717/TestInheritFD.java

@@ -0,0 +1,148 @@
+import static java.io.File.createTempFile;
+import static java.lang.Long.parseLong;
+import static java.lang.System.getProperty;
+import static java.lang.management.ManagementFactory.getOperatingSystemMXBean;
+import static java.nio.file.Files.readAllBytes;
+import static jdk.test.lib.process.ProcessTools.createJavaProcessBuilder;
+
+import java.io.File;
+import java.io.IOException;
+
+import com.sun.management.UnixOperatingSystemMXBean;
+
+/*
+ * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test TestInheritFD
+ * @bug 8176717 8176809
+ * @summary a new process should not inherit open file descriptors
+ * @library /test/lib
+ * @modules java.base/jdk.internal.misc
+ *          java.management
+ */
+
+/**
+ * Test that HotSpot does not leak logging file descriptors.
+ *
+ * This test is performed in three steps. The first VM starts a second VM with
+ * gc logging enabled. The second VM starts a third VM and redirects the third
+ * VMs output to the first VM, it then exits and hopefully closes its log file.
+ *
+ * The third VM waits for the second to exit and close its log file. After that,
+ * the third VM tries to rename the log file of the second VM. If it succeeds in
+ * doing so it means that the third VM did not inherit the open log file
+ * (windows can not rename opened files easily)
+ *
+ * The third VM communicates the success to rename the file by printing "CLOSED
+ * FD". The first VM checks that the string was printed by the third VM.
+ *
+ * On unix like systems, UnixOperatingSystemMXBean is used to check open file
+ * descriptors.
+ */
+
+public class TestInheritFD {
+
+    public static final String LEAKS_FD = "VM RESULT => LEAKS FD";
+    public static final String RETAINS_FD = "VM RESULT => RETAINS FD";
+    public static final String EXIT = "VM RESULT => VM EXIT";
+
+    // first VM
+    public static void main(String[] args) throws Exception {
+        String logPath = createTempFile("logging", ".log").getName();
+        File commFile = createTempFile("communication", ".txt");
+
+        ProcessBuilder pb = createJavaProcessBuilder(
+            "-Xlog:gc:\"" + logPath + "\"",
+            "-Dtest.jdk=" + getProperty("test.jdk"),
+            VMStartedWithLogging.class.getName(),
+            logPath);
+
+        pb.redirectOutput(commFile); // use temp file to communicate between processes
+        pb.start();
+
+        String out = "";
+        do {
+            out = new String(readAllBytes(commFile.toPath()));
+            Thread.sleep(100);
+            System.out.println("SLEEP 100 millis");
+        } while (!out.contains(EXIT));
+
+        System.out.println(out);
+        if (out.contains(RETAINS_FD)) {
+            System.out.println("Log file was not inherited by third VM");
+        } else {
+            throw new RuntimeException("could not match: " + RETAINS_FD);
+        }
+    }
+
+    static class VMStartedWithLogging {
+        // second VM
+        public static void main(String[] args) throws IOException, InterruptedException {
+            ProcessBuilder pb = createJavaProcessBuilder(
+                "-Dtest.jdk=" + getProperty("test.jdk"),
+                VMShouldNotInheritFileDescriptors.class.getName(),
+                args[0],
+                "" + ProcessHandle.current().pid(),
+                "" + (supportsUnixMXBean()?+unixNrFD():-1));
+            pb.inheritIO(); // in future, redirect information from third VM to first VM
+            pb.start();
+        }
+    }
+
+    static class VMShouldNotInheritFileDescriptors {
+        // third VM
+        public static void main(String[] args) throws InterruptedException {
+            File logFile = new File(args[0]);
+            long parentPid = parseLong(args[1]);
+            long parentFDCount = parseLong(args[2]);
+
+            if(supportsUnixMXBean()){
+                long thisFDCount = unixNrFD();
+                System.out.println("This VM FD-count (" + thisFDCount + ") should be strictly less than parent VM FD-count (" + parentFDCount + ") as log file should have been closed");
+                System.out.println(thisFDCount<parentFDCount?RETAINS_FD:LEAKS_FD);
+            } else if (getProperty("os.name").toLowerCase().contains("win")) {
+                windows(logFile, parentPid);
+            } else {
+                System.out.println(LEAKS_FD); // default fail on unknown configuration
+            }
+            System.out.println(EXIT);
+        }
+    }
+
+    static boolean supportsUnixMXBean() {
+        return getOperatingSystemMXBean() instanceof UnixOperatingSystemMXBean;
+    }
+
+    static long unixNrFD() {
+        UnixOperatingSystemMXBean osBean = (UnixOperatingSystemMXBean) getOperatingSystemMXBean();
+        return osBean.getOpenFileDescriptorCount();
+    }
+
+    static void windows(File f, long parentPid) throws InterruptedException {
+        System.out.println("waiting for pid: " + parentPid);
+        ProcessHandle.of(parentPid).ifPresent(handle -> handle.onExit().join());
+        System.out.println("trying to rename file to the same name: " + f);
+        System.out.println(f.renameTo(f)?RETAINS_FD:LEAKS_FD); // this parts communicates a closed file descriptor by printing "CLOSED FD"
+    }
+}