8292297: Fix up loading of override java.security properties file

Reviewed-by: xuelei

src/java.base/share/classes/java/security/Security.java

@@ -83,112 +83,83 @@ public final class Security {
 
     private static void initialize() {
         props = new Properties();
-        boolean loadedProps = false;
         boolean overrideAll = false;
 
         // first load the system properties file
         // to determine the value of security.overridePropertiesFile
         File propFile = securityPropFile("java.security");
-        if (propFile.exists()) {
+        boolean success = loadProps(propFile, null, false);
-            InputStream is = null;
+        if (!success) {
-            try {
+            throw new InternalError("Error loading java.security file");
-                is = new FileInputStream(propFile);
-                props.load(is);
-                loadedProps = true;
-
-                if (sdebug != null) {
-                    sdebug.println("reading security properties file: " +
-                                propFile);
-                }
-            } catch (IOException e) {
-                if (sdebug != null) {
-                    sdebug.println("unable to load security properties from " +
-                                propFile);
-                    e.printStackTrace();
-                }
-            } finally {
-                if (is != null) {
-                    try {
-                        is.close();
-                    } catch (IOException ioe) {
-                        if (sdebug != null) {
-                            sdebug.println("unable to close input stream");
-                        }
-                    }
-                }
-            }
         }
 
         if ("true".equalsIgnoreCase(props.getProperty
                 ("security.overridePropertiesFile"))) {
 
             String extraPropFile = System.getProperty
-                                        ("java.security.properties");
+                    ("java.security.properties");
             if (extraPropFile != null && extraPropFile.startsWith("=")) {
                 overrideAll = true;
                 extraPropFile = extraPropFile.substring(1);
             }
+            loadProps(null, extraPropFile, overrideAll);
+        }
+    }
 
-            if (overrideAll) {
+    private static boolean loadProps(File masterFile, String extraPropFile, boolean overrideAll) {
-                props = new Properties();
+        InputStream is = null;
-                if (sdebug != null) {
+        try {
-                    sdebug.println
+            if (masterFile != null && masterFile.exists()) {
-                        ("overriding other security properties files!");
+                is = new FileInputStream(masterFile);
+            } else if (extraPropFile != null) {
+                extraPropFile = PropertyExpander.expand(extraPropFile);
+                File propFile = new File(extraPropFile);
+                URL propURL;
+                if (propFile.exists()) {
+                    propURL = new URL
+                            ("file:" + propFile.getCanonicalPath());
+                } else {
+                    propURL = new URL(extraPropFile);
                 }
-            }
 
-            // now load the user-specified file so its values
+                is = propURL.openStream();
-            // will win if they conflict with the earlier values
+                if (overrideAll) {
-            if (extraPropFile != null) {
+                    props = new Properties();
-                InputStream is = null;
-                try {
-                    URL propURL;
-
-                    extraPropFile = PropertyExpander.expand(extraPropFile);
-                    propFile = new File(extraPropFile);
-                    if (propFile.exists()) {
-                        propURL = new URL
-                                ("file:" + propFile.getCanonicalPath());
-                    } else {
-                        propURL = new URL(extraPropFile);
-                    }
-                    is = propURL.openStream();
-                    props.load(is);
-                    loadedProps = true;
-
-                    if (sdebug != null) {
-                        sdebug.println("reading security properties file: " +
-                                        propURL);
-                        if (overrideAll) {
-                            sdebug.println
-                                ("overriding other security properties files!");
-                        }
-                    }
-                } catch (Exception e) {
                     if (sdebug != null) {
                         sdebug.println
-                                ("unable to load security properties from " +
+                                ("overriding other security properties files!");
-                                extraPropFile);
-                        e.printStackTrace();
                     }
-                } finally {
+                }
-                    if (is != null) {
+            } else {
-                        try {
+                // unexpected
-                            is.close();
+                return false;
-                        } catch (IOException ioe) {
+            }
-                            if (sdebug != null) {
+            props.load(is);
-                                sdebug.println("unable to close input stream");
+            if (sdebug != null) {
-                            }
+                // ExceptionInInitializerError if masterFile.getName() is
-                        }
+                // called here (NPE!). Leave as is (and few lines down)
+                sdebug.println("reading security properties file: " +
+                        masterFile == null ? extraPropFile : "java.security");
+            }
+            return true;
+        } catch (IOException | PropertyExpander.ExpandException e) {
+            if (sdebug != null) {
+                sdebug.println("unable to load security properties from " +
+                        masterFile == null ? extraPropFile : "java.security");
+                e.printStackTrace();
+            }
+            return false;
+        } finally {
+            if (is != null) {
+                try {
+                    is.close();
+                } catch (IOException ioe) {
+                    if (sdebug != null) {
+                        sdebug.println("unable to close input stream");
                     }
                 }
             }
         }
-
-        if (!loadedProps) {
-            throw new InternalError("java.security file missing");
-        }
-
     }
 
     /**

test/jdk/java/security/Security/ConfigFileTest.java

@@ -28,6 +28,7 @@ import java.io.IOException;
 import java.io.UncheckedIOException;
 import java.nio.file.*;
 
+import java.security.Provider;
 import java.security.Security;
 import java.util.Arrays;
 import java.util.Optional;
@@ -35,7 +36,7 @@ import java.util.Optional;
 /*
  * @test
  * @summary Throw error if default java.security file is missing
- * @bug 8155246
+ * @bug 8155246 8292297
  * @library /test/lib
  * @run main ConfigFileTest
  */
@@ -50,39 +51,66 @@ public class ConfigFileTest {
 
         if (args.length == 1) {
             // set up is complete. Run code to exercise loading of java.security
-            System.out.println(Arrays.toString(Security.getProviders()));
+            Provider[] provs = Security.getProviders();
+            System.out.println(Arrays.toString(provs) + "NumProviders: " + provs.length);
         } else {
             Files.createDirectory(copyJdkDir);
             Path jdkTestDir = Path.of(Optional.of(System.getProperty("test.jdk"))
                             .orElseThrow(() -> new RuntimeException("Couldn't load JDK Test Dir"))
             );
 
-            copyJDKMinusJavaSecurity(jdkTestDir, copyJdkDir);
+            copyJDK(jdkTestDir, copyJdkDir);
             String extraPropsFile = Path.of(System.getProperty("test.src"), "override.props").toString();
 
             // exercise some debug flags while we're here
-            // launch JDK without java.security file being present or specified
+            // regular JDK install - should expect success
-            exerciseSecurity(copiedJava.toString(), "-cp", System.getProperty("test.classes"),
+            exerciseSecurity(0, "java",
+                    copiedJava.toString(), "-cp", System.getProperty("test.classes"),
                     "-Djava.security.debug=all", "-Djavax.net.debug=all", "ConfigFileTest", "runner");
 
+            // given an overriding security conf file that doesn't exist, we shouldn't
+            // overwrite the properties from original/master security conf file
+            exerciseSecurity(0, "SUN version",
+                    copiedJava.toString(), "-cp", System.getProperty("test.classes"),
+                    "-Djava.security.debug=all", "-Djavax.net.debug=all",
+                    "-Djava.security.properties==file:///" + extraPropsFile + "badFileName",
+                    "ConfigFileTest", "runner");
+
+            // test JDK launch with customized properties file
+            exerciseSecurity(0, "NumProviders: 6",
+                    copiedJava.toString(), "-cp", System.getProperty("test.classes"),
+                    "-Djava.security.debug=all", "-Djavax.net.debug=all",
+                    "-Djava.security.properties==file:///" + extraPropsFile,
+                    "ConfigFileTest", "runner");
+
+            // delete the master conf file
+            Files.delete(Path.of(copyJdkDir.toString(), "conf",
+                    "security","java.security"));
+
+            // launch JDK without java.security file being present or specified
+            exerciseSecurity(1, "Error loading java.security file",
+                    copiedJava.toString(), "-cp", System.getProperty("test.classes"),
+                    "-Djava.security.debug=all", "-Djavax.net.debug=all",
+                    "ConfigFileTest", "runner");
+
             // test the override functionality also. Should not be allowed since
             // "security.overridePropertiesFile=true" Security property is missing.
-            exerciseSecurity(copiedJava.toString(), "-cp", System.getProperty("test.classes"),
+            exerciseSecurity(1, "Error loading java.security file",
+                    copiedJava.toString(), "-cp", System.getProperty("test.classes"),
                     "-Djava.security.debug=all", "-Djavax.net.debug=all",
-                    "-Djava.security.properties==file://" + extraPropsFile, "ConfigFileTest", "runner");
+                    "-Djava.security.properties==file:///" + extraPropsFile, "ConfigFileTest", "runner");
         }
     }
 
-    private static void exerciseSecurity(String... args) throws Exception {
+    private static void exerciseSecurity(int exitCode, String output, String... args) throws Exception {
         ProcessBuilder process = new ProcessBuilder(args);
         OutputAnalyzer oa = ProcessTools.executeProcess(process);
-        oa.shouldHaveExitValue(1).shouldContain("java.security file missing");
+        oa.shouldHaveExitValue(exitCode).shouldContain(output);
     }
 
-    private static void copyJDKMinusJavaSecurity(Path src, Path dst) throws Exception {
+    private static void copyJDK(Path src, Path dst) throws Exception {
         Files.walk(src)
             .skip(1)
-            .filter(p -> !p.toString().endsWith("java.security"))
             .forEach(file -> {
                 try {
                     Files.copy(file, dst.resolve(src.relativize(file)), StandardCopyOption.COPY_ATTRIBUTES);
@@ -91,4 +119,4 @@ public class ConfigFileTest {
                 }
             });
     }
-}
+}

test/jdk/java/security/Security/override.props

@@ -1,7 +1,7 @@
+# exercise ServiceLoader and legacy (class load) approach
 security.provider.1=sun.security.provider.Sun
-security.provider.2=sun.security.rsa.SunRsaSign
+security.provider.2=SunRsaSign
 security.provider.3=sun.security.ssl.SunJSSE
 security.provider.4=com.sun.crypto.provider.SunJCE
-security.provider.5=sun.security.jgss.SunProvider
+security.provider.5=SunJGSS
-security.provider.6=com.sun.security.sasl.Provider
+security.provider.6=SunSASL
-