8265465: jcmd VM.cds should keep already dumped archive when exception happens

Reviewed-by: iklam, ccheung

src/java.base/share/classes/jdk/internal/misc/CDS.java

@@ -257,21 +257,24 @@ public class CDS {
     */
     private static void dumpSharedArchive(boolean isStatic, String fileName) throws Exception {
         String currentPid = String.valueOf(ProcessHandle.current().pid());
-        String archiveFile =  fileName != null ? fileName :
+        String archiveFileName =  fileName != null ? fileName :
             "java_pid" + currentPid + (isStatic ? "_static.jsa" : "_dynamic.jsa");
 
-        // delete if archive file aready exists
+        String tempArchiveFileName = archiveFileName + ".temp";
-        File fileArchive = new File(archiveFile);
+        File tempArchiveFile = new File(tempArchiveFileName);
-        if (fileArchive.exists()) {
+        // The operation below may cause exception if the file or its dir is protected.
-            fileArchive.delete();
+        if (!tempArchiveFile.exists()) {
+            tempArchiveFile.createNewFile();
         }
+        tempArchiveFile.delete();
+
         if (isStatic) {
-            String listFile = archiveFile + ".classlist";
+            String listFileName = archiveFileName + ".classlist";
-            File fileList = new File(listFile);
+            File listFile = new File(listFileName);
-            if (fileList.exists()) {
+            if (listFile.exists()) {
-                fileList.delete();
+                listFile.delete();
             }
-            dumpClassList(listFile);
+            dumpClassList(listFileName);
             String jdkHome = System.getProperty("java.home");
             String classPath = System.getProperty("java.class.path");
             List<String> cmds = new ArrayList<String>();
@@ -280,8 +283,8 @@ public class CDS {
             cmds.add(classPath);
             cmds.add("-Xlog:cds");
             cmds.add("-Xshare:dump");
-            cmds.add("-XX:SharedClassListFile=" + listFile);
+            cmds.add("-XX:SharedClassListFile=" + listFileName);
-            cmds.add("-XX:SharedArchiveFile=" + archiveFile);
+            cmds.add("-XX:SharedArchiveFile=" + tempArchiveFileName);
 
             // All runtime args.
             String[] vmArgs = VM.getRuntimeArguments();
@@ -301,26 +304,33 @@ public class CDS {
 
             proc.waitFor();
             // done, delete classlist file.
-            if (fileList.exists()) {
+            listFile.delete();
-                fileList.delete();
+
-            }
             // Check if archive has been successfully dumped. We won't reach here if exception happens.
             // Throw exception if file is not created.
-            if (!fileArchive.exists()) {
+            if (!tempArchiveFile.exists()) {
-                throw new RuntimeException("Archive file " + archiveFile +
+                throw new RuntimeException("Archive file " + tempArchiveFileName +
                                            " is not created, please check stdout file " +
                                             stdOutFile + " or stderr file " +
                                             stdErrFile + " for more detail");
             }
         } else {
-            dumpDynamicArchive(archiveFile);
+            dumpDynamicArchive(tempArchiveFileName);
-            if (!fileArchive.exists()) {
+            if (!tempArchiveFile.exists()) {
-                throw new RuntimeException("Archive file " + archiveFile +
+                throw new RuntimeException("Archive file " + tempArchiveFileName +
                                            " is not created, please check process " +
                                            currentPid + " output for more detail");
             }
         }
+        // Override the existing archive file
+        File archiveFile = new File(archiveFileName);
+        if (archiveFile.exists()) {
+            archiveFile.delete();
+        }
+        if (!tempArchiveFile.renameTo(archiveFile)) {
+            throw new RuntimeException("Cannot rename temp file " + tempArchiveFileName + " to archive file" + archiveFileName);
+        }
         // Everyting goes well, print out the file name.
-        System.out.println((isStatic ? "Static" : " Dynamic") + " dump to file " + archiveFile);
+        System.out.println((isStatic ? "Static" : " Dynamic") + " dump to file " + archiveFileName);
     }
 }

test/hotspot/jtreg/TEST.groups

@@ -315,6 +315,7 @@ hotspot_appcds_dynamic = \
  -runtime/cds/appcds/javaldr/LockDuringDump.java \
  -runtime/cds/appcds/jcmd/JCmdTestStaticDump.java \
  -runtime/cds/appcds/jcmd/JCmdTestDynamicDump.java \
+ -runtime/cds/appcds/jcmd/JCmdTestFileSafety.java \
  -runtime/cds/appcds/methodHandles \
  -runtime/cds/appcds/sharedStrings \
  -runtime/cds/appcds/ArchiveRelocationTest.java \

test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTestDumpBase.java

@@ -28,6 +28,7 @@ import java.util.ArrayList;
 import java.util.List;
 
 import jdk.test.lib.apps.LingeredApp;
+import jdk.test.lib.dcmd.CommandExecutorException;
 import jdk.test.lib.dcmd.PidJcmdExecutor;
 import jdk.test.lib.process.OutputAnalyzer;
 import jtreg.SkippedException;
@@ -74,6 +75,18 @@ public abstract class JCmdTestDumpBase {
     public static final boolean EXPECT_PASS = true;
     public static final boolean EXPECT_FAIL = !EXPECT_PASS;
 
+    // If delete the created archive after test.
+    private static boolean keepArchive = false;
+    public static void setKeepArchive(boolean v) { keepArchive = v; }
+    public static void checkFileExistence(String fileName, boolean checkExist) throws Exception {
+        File file = new File(fileName);
+        boolean exist = file.exists();
+        boolean resultIsTrue = checkExist ? exist : !exist;
+        if (!resultIsTrue) {
+            throw new RuntimeException("File " + fileName +  " should " + (checkExist ?  "exist" : "not exist"));
+        }
+    }
+
     protected static void buildJars() throws Exception {
         testJar = JarBuilder.build("test", TEST_CLASSES);
         bootJar = JarBuilder.build("boot", BOOT_CLASSES);
@@ -147,7 +160,7 @@ public abstract class JCmdTestDumpBase {
         args.add("-Xlog:class+load");
 
         LingeredApp app = createLingeredApp(args.toArray(new String[0]));
-        app.setLogFileName("JCmdTestDynamicDump.log." + (logFileCount++));
+        app.setLogFileName("JCmdTest-Run-" + (isStatic ? "Static.log" : "Dynamic.log.") + (logFileCount++));
         app.stopApp();
         String output = app.getOutput().getStdout();
         if (messages != null) {
@@ -159,19 +172,17 @@ public abstract class JCmdTestDumpBase {
         }
     }
 
-    protected static void test(String archiveFile, long pid,
+    protected static void test(String fileName, long pid,
                              boolean useBoot, boolean expectOK, String... messages) throws Exception {
         System.out.println("Expected: " + (expectOK ? "SUCCESS" : "FAIL"));
-        String fileName = archiveFile != null ? archiveFile :
+        String archiveFileName = fileName != null ? fileName :
             ("java_pid" + pid + (isStatic ? "_static.jsa" : "_dynamic.jsa"));
-        File file = new File(fileName);
+
-        if (file.exists()) {
+        File archiveFile = new File(archiveFileName);
-            file.delete();
-        }
 
         String jcmd = "VM.cds " + (isStatic ? "static_dump" : "dynamic_dump");
-        if (archiveFile  != null) {
+        if (archiveFileName  != null) {
-          jcmd +=  " " + archiveFile;
+          jcmd +=  " " + archiveFileName;
         }
 
         PidJcmdExecutor cmdExecutor = new PidJcmdExecutor(String.valueOf(pid));
@@ -179,15 +190,14 @@ public abstract class JCmdTestDumpBase {
 
         if (expectOK) {
             output.shouldHaveExitValue(0);
-            if (!file.exists()) {
+            checkFileExistence(archiveFileName, true);
-                throw new RuntimeException("Could not create shared archive: " + fileName);
+            runWithArchiveFile(archiveFileName, useBoot, messages);
-            } else {
+            if (!keepArchive) {
-                runWithArchiveFile(fileName, useBoot, messages);
+                archiveFile.delete();
-                file.delete();
             }
         } else {
-            if (file.exists()) {
+            if (!keepArchive) {
-                throw new RuntimeException("Should not create shared archive " + fileName);
+                checkFileExistence(archiveFileName, false);
             }
         }
     }

test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTestFileSafety.java

@@ -0,0 +1,107 @@
+/*
+ * Copyright (c) 2021, 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
+ * @bug 8265465
+ * @summary Test jcmd to dump static shared archive.
+ * @requires vm.cds
+ * @library /test/lib /test/hotspot/jtreg/runtime/cds/appcds
+ * @modules jdk.jcmd/sun.tools.common:+open
+ * @compile ../test-classes/Hello.java JCmdTestDumpBase.java
+ * @build sun.hotspot.WhiteBox
+ * @build JCmdTestLingeredApp JCmdTestFileSafety
+ * @run driver jdk.test.lib.helpers.ClassFileInstaller sun.hotspot.WhiteBox
+ * @run main/othervm/timeout=480 -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI JCmdTestFileSafety
+ */
+
+import java.io.File;
+import java.io.IOException;
+import jdk.test.lib.cds.CDSTestUtils;
+import jdk.test.lib.apps.LingeredApp;
+import jdk.test.lib.Platform;
+
+public class JCmdTestFileSafety extends JCmdTestDumpBase {
+
+    static void test() throws Exception {
+        buildJars();
+
+        LingeredApp app = null;
+        long pid;
+
+        int  test_count = 1;
+        String subDir    = "subdir";
+        File outputDirFile = new File(subDir);
+        if (!outputDirFile.exists()) {
+            outputDirFile.mkdir();
+        }
+        outputDirFile.setWritable(true);
+        String localFileName = subDir + File.separator + "MyStaticDump.jsa";
+        setIsStatic(true/*static*/);
+        // Set target dir not writable, do static dump
+        print2ln(test_count++ + " Set target dir not writable, do static dump");
+        setKeepArchive(true);
+        app = createLingeredApp("-cp", allJars);
+        pid = app.getPid();
+        test(localFileName, pid, noBoot,  EXPECT_PASS);
+        outputDirFile.setWritable(false);
+        test(localFileName, pid, noBoot,  EXPECT_FAIL);
+        app.stopApp();
+        outputDirFile.setWritable(true);
+        checkFileExistence(localFileName, true/*exist*/);
+
+        setIsStatic(false/*dynamic*/);
+        //  Set target dir not writable, do dynamic dump
+        print2ln(test_count++ + " Set target dir not writable, do dynamic dump");
+        setKeepArchive(true);
+        outputDirFile.setWritable(true);
+        app = createLingeredApp("-cp", allJars, "-XX:+RecordDynamicDumpInfo");
+        pid = app.getPid();
+        localFileName = subDir + File.separator + "MyDynamicDump.jsa";
+        test(localFileName, pid, noBoot,  EXPECT_PASS);
+        app.stopApp();
+        // cannot dynamically dump twice, restart
+        app = createLingeredApp("-cp", allJars, "-XX:+RecordDynamicDumpInfo");
+        pid = app.getPid();
+        outputDirFile.setWritable(false);
+        test(localFileName, pid, noBoot,  EXPECT_FAIL);
+        outputDirFile.setWritable(true);
+        app.stopApp();
+        // MyDynamicDump.jsa should exist
+        checkFileExistence(localFileName, true);
+        File rmFile = new File(localFileName);
+        rmFile.delete();
+        outputDirFile.delete();
+    }
+
+    public static void main(String... args) throws Exception {
+        if (Platform.isWindows()) {
+            // ON windows, File operation resulted difference from other OS.
+            // Set dir to not accessible for write, we still can run the test
+            // to create archive successfully which is not expected.
+            throw new jtreg.SkippedException("Test skipped on Windows");
+        }
+        runTest(JCmdTestFileSafety::test);
+    }
+}