8278087: Deserialization filter and filter factory property error reporting under specified

Reviewed-by: lancea, bpb

src/java.base/share/classes/java/io/ObjectInputFilter.java

@@ -27,7 +27,6 @@ package java.io;
 
 import jdk.internal.access.SharedSecrets;
 import jdk.internal.util.StaticProperty;
-import sun.security.action.GetBooleanAction;
 
 import java.lang.reflect.InvocationTargetException;
 import java.security.AccessController;
@@ -523,10 +522,15 @@ public interface ObjectInputFilter {
      * {@systemProperty jdk.serialFilter}, its value is used to configure the filter.
      * If the system property is not defined, and the {@link java.security.Security} property
      * {@code jdk.serialFilter} is defined then it is used to configure the filter.
-     * The filter is created as if {@link #createFilter(String) createFilter} is called;
+     * The filter is created as if {@link #createFilter(String) createFilter} is called,
-     * if the filter string is invalid, an {@link ExceptionInInitializerError} is thrown.
+     * if the filter string is invalid the initialization fails and subsequent attempts to
-     * Otherwise, the filter is not configured during initialization and
+     * {@linkplain Config#getSerialFilter() get the filter}, {@linkplain Config#setSerialFilter set a filter},
-     * can be set with {@link #setSerialFilter(ObjectInputFilter) Config.setSerialFilter}.
+     * or create an {@linkplain ObjectInputStream#ObjectInputStream(InputStream) ObjectInputStream}
+     * throw {@link IllegalStateException}. Deserialization is not possible with an
+     * invalid serial filter.
+     * If the system property {@code jdk.serialFilter} or the {@link java.security.Security}
+     * property {@code jdk.serialFilter} is not set the filter can be set with
+     * {@link #setSerialFilter(ObjectInputFilter) Config.setSerialFilter}.
      * Setting the {@code jdk.serialFilter} with {@link System#setProperty(String, String)
      * System.setProperty} <em>does not set the filter</em>.
      * The syntax for the property value is the same as for the
@@ -545,9 +549,12 @@ public interface ObjectInputFilter {
      * <p>The class must be public, must have a public zero-argument constructor, implement the
      * {@link BinaryOperator {@literal BinaryOperator<ObjectInputFilter>}} interface, provide its implementation and
      * be accessible via the {@linkplain ClassLoader#getSystemClassLoader() application class loader}.
-     * If the filter factory constructor is not invoked successfully, an {@link ExceptionInInitializerError}
+     * If the filter factory constructor is not invoked successfully subsequent attempts to
-     * is thrown and subsequent use of the filter factory for deserialization fails with
+     * {@linkplain Config#getSerialFilterFactory() get the factory},
-     * {@link IllegalStateException}.
+     * {@linkplain Config#setSerialFilterFactory(BinaryOperator) set the factory}, or create an
+     * {@link ObjectInputStream#ObjectInputStream(InputStream) ObjectInputStream}
+     * throw {@link IllegalStateException}. Deserialization is not possible with an
+     * invalid serial filter factory.
      * The filter factory configured using the system or security property during initialization
      * can NOT be replaced with {@link #setSerialFilterFactory(BinaryOperator) Config.setSerialFilterFactory}.
      * This ensures that a filter factory set on the command line is not overridden accidentally
@@ -582,12 +589,22 @@ public interface ObjectInputFilter {
          */
         private static volatile ObjectInputFilter serialFilter;
 
+        /**
+         * Saved message if the jdk.serialFilter property is invalid.
+         */
+        private static final String invalidFilterMessage;
+
         /**
          * Current serial filter factory.
          * @see Config#setSerialFilterFactory(BinaryOperator)
          */
         private static volatile BinaryOperator<ObjectInputFilter> serialFilterFactory;
 
+        /**
+         * Saved message if the jdk.serialFilterFactory property is invalid.
+         */
+        private static final String invalidFactoryMessage;
+
         /**
          * Boolean to indicate that the filter factory can not be set or replaced.
          * - an ObjectInputStream has already been created using the current filter factory
@@ -630,23 +647,24 @@ public interface ObjectInputFilter {
                         Security.getProperty(SERIAL_FILTER_PROPNAME));
 
             // Initialize the static filter if the jdk.serialFilter is present
-            ObjectInputFilter filter = null;
+            String filterMessage = null;
             if (filterString != null) {
                 configLog.log(DEBUG,
                         "Creating deserialization filter from {0}", filterString);
                 try {
-                    filter = createFilter(filterString);
+                    serialFilter = createFilter(filterString);
                 } catch (RuntimeException re) {
                     configLog.log(ERROR,
                             "Error configuring filter: {0}", (Object) re);
-                    // Do not continue if configuration not initialized
+                    // serialFilter remains null
-                    throw re;
+                    filterMessage = "Invalid jdk.serialFilter: " +  re.getMessage();
                 }
             }
-            serialFilter = filter;
+            invalidFilterMessage = filterMessage;
 
             // Initialize the filter factory if the jdk.serialFilterFactory is defined
             // otherwise use the builtin filter factory.
+            String factoryMessage = null;
             if (factoryClassName == null) {
                 serialFilterFactory = new BuiltinFilterFactory();
             } else {
@@ -671,10 +689,13 @@ public interface ObjectInputFilter {
                     Throwable th = (ex instanceof InvocationTargetException ite) ? ite.getCause() : ex;
                     configLog.log(ERROR,
                             "Error configuring filter factory: {0}", (Object)th);
-                    // Do not continue if configuration not initialized
+                    // Configuration not initialized
-                    throw new ExceptionInInitializerError(th);
+                    // serialFilterFactory remains null and filterFactoryNoReplace == true;
+                    factoryMessage = "invalid jdk.serialFilterFactory: " +
+                            factoryClassName + ": " + th.getClass().getName() + ": " + th.getMessage();
                 }
             }
+            invalidFactoryMessage = factoryMessage;
             // Setup shared secrets for RegistryImpl to use.
             SharedSecrets.setJavaObjectInputFilterAccess(Config::createFilter2);
         }
@@ -696,8 +717,14 @@ public interface ObjectInputFilter {
          * Returns the static JVM-wide deserialization filter or {@code null} if not configured.
          *
          * @return the static JVM-wide deserialization filter or {@code null} if not configured
+         * @throws IllegalStateException if the initialization of the filter from the
+         *      system property {@code jdk.serialFilter} or
+         *      the security property {@code jdk.serialFilter} fails.
          */
         public static ObjectInputFilter getSerialFilter() {
+            if (invalidFilterMessage != null) {
+                throw new IllegalStateException(invalidFilterMessage);
+            }
             return serialFilter;
         }
 
@@ -707,7 +734,9 @@ public interface ObjectInputFilter {
          * @param filter the deserialization filter to set as the JVM-wide filter; not null
          * @throws SecurityException if there is security manager and the
          *       {@code SerializablePermission("serialFilter")} is not granted
-         * @throws IllegalStateException if the filter has already been set
+         * @throws IllegalStateException if the filter has already been set or the initialization
+         *       of the filter from the system property {@code jdk.serialFilter} or
+         *       the security property {@code jdk.serialFilter} fails.
          */
         public static void setSerialFilter(ObjectInputFilter filter) {
             Objects.requireNonNull(filter, "filter");
@@ -716,6 +745,9 @@ public interface ObjectInputFilter {
             if (sm != null) {
                 sm.checkPermission(ObjectStreamConstants.SERIAL_FILTER_PERMISSION);
             }
+            if (invalidFilterMessage != null) {
+                throw new IllegalStateException(invalidFilterMessage);
+            }
             synchronized (serialFilterLock) {
                 if (serialFilter != null) {
                     throw new IllegalStateException("Serial filter can only be set once");
@@ -749,8 +781,10 @@ public interface ObjectInputFilter {
          * @since 17
          */
         public static BinaryOperator<ObjectInputFilter> getSerialFilterFactory() {
-            if (serialFilterFactory == null)
+            if (serialFilterFactory == null) {
-                throw new IllegalStateException("Serial filter factory initialization incomplete");
+                // If initializing the factory failed or not yet complete, throw with the message
+                throw new IllegalStateException(invalidFilterFactoryMessage());
+            }
             return serialFilterFactory;
         }
 
@@ -812,15 +846,26 @@ public interface ObjectInputFilter {
             }
             if (filterFactoryNoReplace.getAndSet(true)) {
                 final String msg = serialFilterFactory != null
-                        ? serialFilterFactory.getClass().getName()
+                        ? "Cannot replace filter factory: " + serialFilterFactory.getClass().getName()
-                        : "initialization incomplete";
+                        : invalidFilterFactoryMessage();
-                throw new IllegalStateException("Cannot replace filter factory: " + msg);
+                throw new IllegalStateException(msg);
             }
             configLog.log(DEBUG,
                     "Setting deserialization filter factory to {0}", filterFactory.getClass().getName());
             serialFilterFactory = filterFactory;
         }
 
+        /*
+         * Return message for an invalid filter factory configuration saved from the static init.
+         * It can be called before the static initializer is complete and has set the message/null.
+         */
+        private static String invalidFilterFactoryMessage() {
+            assert serialFilterFactory == null;     // undefined if a filter factory has been set
+            return (invalidFactoryMessage != null)
+                ? invalidFactoryMessage
+                : "Serial filter factory initialization incomplete";
+        }
+
         /**
          * Returns an ObjectInputFilter from a string of patterns.
          * <p>

src/java.base/share/classes/java/io/ObjectInputStream.java

@@ -384,6 +384,8 @@ public class ObjectInputStream
      * <p>The constructor initializes the deserialization filter to the filter returned
      * by invoking the {@link Config#getSerialFilterFactory()} with {@code null} for the current filter
      * and the {@linkplain Config#getSerialFilter() static JVM-wide filter} for the requested filter.
+     * If the serial filter or serial filter factory properties are invalid
+     * an {@link IllegalStateException} is thrown.
      *
      * <p>If a security manager is installed, this constructor will check for
      * the "enableSubclassImplementation" SerializablePermission when invoked
@@ -396,6 +398,8 @@ public class ObjectInputStream
      * @throws  IOException if an I/O error occurs while reading stream header
      * @throws  SecurityException if untrusted subclass illegally overrides
      *          security-sensitive methods
+     * @throws  IllegalStateException if the initialization of {@link ObjectInputFilter.Config}
+     *          fails due to invalid serial filter or serial filter factory properties.
      * @throws  NullPointerException if {@code in} is {@code null}
      * @see     ObjectInputStream#ObjectInputStream()
      * @see     ObjectInputStream#readFields()
@@ -421,6 +425,8 @@ public class ObjectInputStream
      * <p>The constructor initializes the deserialization filter to the filter returned
      * by invoking the {@link Config#getSerialFilterFactory()} with {@code null} for the current filter
      * and the {@linkplain Config#getSerialFilter() static JVM-wide filter} for the requested filter.
+     * If the serial filter or serial filter factory properties are invalid
+     * an {@link IllegalStateException} is thrown.
      *
      * <p>If there is a security manager installed, this method first calls the
      * security manager's {@code checkPermission} method with the
@@ -431,6 +437,8 @@ public class ObjectInputStream
      *          {@code checkPermission} method denies enabling
      *          subclassing.
      * @throws  IOException if an I/O error occurs while creating this stream
+     * @throws  IllegalStateException if the initialization of {@link ObjectInputFilter.Config}
+     *      fails due to invalid serial filter or serial filter factory properties.
      * @see SecurityManager#checkPermission
      * @see java.io.SerializablePermission
      */

test/jdk/java/io/Serializable/serialFilter/InvalidGlobalFilterTest.java

@@ -21,67 +21,100 @@
  * questions.
  */
 
-import jdk.test.lib.process.OutputAnalyzer;
+import org.testng.Assert;
-import jdk.test.lib.process.ProcessTools;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
 
-import java.io.File;
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
 import java.io.ObjectInputFilter;
+import java.io.ObjectInputStream;
+import java.util.Map;
 
 /*
  * @test
- * @bug 8269336
+ * @bug 8278087
  * @summary Test that an invalid pattern value for the jdk.serialFilter system property causes an
- * exception to be thrown in the class initialization of java.io.ObjectInputFilter.Config class
+ * exception to be thrown when an attempt is made to use the filter or deserialize.
- * @library /test/lib
+ * A subset of invalid filter patterns is tested.
- * @run driver InvalidGlobalFilterTest
+ * @run testng/othervm -Djdk.serialFilter=.* InvalidGlobalFilterTest
+ * @run testng/othervm -Djdk.serialFilter=! InvalidGlobalFilterTest
+ * @run testng/othervm -Djdk.serialFilter=/ InvalidGlobalFilterTest
+ *
  */
+@Test
 public class InvalidGlobalFilterTest {
     private static final String serialPropName = "jdk.serialFilter";
+    private static final String serialFilter = System.getProperty(serialPropName);
+
+    static {
+        // Enable logging
+        System.setProperty("java.util.logging.config.file",
+                System.getProperty("test.src", ".") + "/logging.properties");
+    }
 
     /**
-     * Launches multiple instances of a Java program by passing each instance an invalid value
+     * Map of invalid patterns to the expected exception message.
-     * for the {@code jdk.serialFilter} system property. The launched program then triggers the
-     * class initialization of {@code ObjectInputFilter.Config} class to have it parse the (invalid)
-     * value of the system property. The launched program is expected to propagate the exception
-     * raised by the {@code ObjectInputFilter.Config} initialization and the test asserts that the
-     * launched program did indeed fail with this expected exception.
      */
-    public static void main(final String[] args) throws Exception {
+    private static final Map<String, String> invalidMessages =
-        final String[] invalidPatterns = {".*", ".**", "!", "/java.util.Hashtable", "java.base/", "/"};
+            Map.of(".*", "Invalid jdk.serialFilter: package missing in: \".*\"",
-        for (final String invalidPattern : invalidPatterns) {
+                    ".**", "Invalid jdk.serialFilter: package missing in: \".**\"",
-            final ProcessBuilder processBuilder = ProcessTools.createJavaProcessBuilder(
+                    "!", "Invalid jdk.serialFilter: class or package missing in: \"!\"",
-                    "-D" + serialPropName + "=" + invalidPattern,
+                    "/java.util.Hashtable", "Invalid jdk.serialFilter: module name is missing in: \"/java.util.Hashtable\"",
-                    "-Djava.util.logging.config.file=" + System.getProperty("test.src")
+                    "java.base/", "Invalid jdk.serialFilter: class or package missing in: \"java.base/\"",
-                            + File.separator + "logging.properties",
+                    "/", "Invalid jdk.serialFilter: module name is missing in: \"/\"");
-                    ObjectInputFilterConfigLoader.class.getName());
+
-            // launch a process by passing it an invalid value for -Djdk.serialFilter
+    @DataProvider(name = "MethodsToCall")
-            final OutputAnalyzer outputAnalyzer = ProcessTools.executeProcess(processBuilder);
+    private Object[][] cases() {
-            try {
+        return new Object[][] {
-                // we expect the JVM launch to fail
+                {serialFilter, "getSerialFilter", (Assert.ThrowingRunnable) () -> ObjectInputFilter.Config.getSerialFilter()},
-                outputAnalyzer.shouldNotHaveExitValue(0);
+                {serialFilter, "setSerialFilter", (Assert.ThrowingRunnable) () -> ObjectInputFilter.Config.setSerialFilter(new NoopFilter())},
-                // do an additional check to be sure it failed for the right reason
+                {serialFilter, "new ObjectInputStream(is)", (Assert.ThrowingRunnable) () -> new ObjectInputStream(new ByteArrayInputStream(new byte[0]))},
-                outputAnalyzer.stderrShouldContain("java.lang.ExceptionInInitializerError");
+                {serialFilter, "new OISSubclass()", (Assert.ThrowingRunnable) () -> new OISSubclass()},
-            } finally {
+        };
-                // fail or pass, we print out the generated output from the launched program
-                // for any debugging
-                System.err.println("Diagnostics from process " + outputAnalyzer.pid() + ":");
-                // print out any stdout/err that was generated in the launched program
-                outputAnalyzer.reportDiagnosticSummary();
     }
+
+    /**
+     * Test each method that should throw IllegalStateException based on
+     * the invalid arguments it was launched with.
+     */
+    @Test(dataProvider = "MethodsToCall")
+    public void initFaultTest(String pattern, String method, Assert.ThrowingRunnable runnable) {
+
+        IllegalStateException ex = Assert.expectThrows(IllegalStateException.class,
+                runnable);
+
+        String expected = invalidMessages.get(serialFilter);
+        if (expected == null) {
+            Assert.fail("No expected message for filter: " + serialFilter);
+        }
+        System.out.println(ex.getMessage());
+        Assert.assertEquals(ex.getMessage(), expected, "wrong message");
+    }
+
+    private static class NoopFilter implements ObjectInputFilter {
+        /**
+         * Returns UNDECIDED.
+         *
+         * @param filter the FilterInfo
+         * @return Status.UNDECIDED
+         */
+        public ObjectInputFilter.Status checkInput(FilterInfo filter) {
+             return ObjectInputFilter.Status.UNDECIDED;
+        }
+
+        public String toString() {
+            return "NoopFilter";
         }
     }
 
-    // A main() class which just triggers the class initialization of ObjectInputFilter.Config
+    /**
-    private static final class ObjectInputFilterConfigLoader {
+     * Subclass of ObjectInputStream to test subclassing constructor.
+     */
+    private static class OISSubclass extends ObjectInputStream {
 
-        public static void main(final String[] args) throws Exception {
+        protected OISSubclass() throws IOException {
-            System.out.println("JVM was launched with " + serialPropName
-                    + " system property set to " + System.getProperty(serialPropName));
-            // this call is expected to fail and we aren't interested in the result.
-            // we just let the exception propagate out of this call and fail the
-            // launched program. The test which launched this main, then asserts
-            // that the exception was indeed thrown.
-            ObjectInputFilter.Config.getSerialFilter();
         }
     }
+
 }

test/jdk/java/io/Serializable/serialFilter/SerialFactoryFaults.java

@@ -22,10 +22,14 @@
  */
 
 import org.testng.Assert;
+import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
 import java.io.ObjectInputFilter;
 import java.io.ObjectInputFilter.Config;
+import java.io.ObjectInputStream;
 import java.util.function.BinaryOperator;
 
 /* @test
@@ -39,33 +43,47 @@ import java.util.function.BinaryOperator;
 @Test
 public class SerialFactoryFaults {
 
+    // Sample the serial factory class name
+    private static final String factoryName = System.getProperty("jdk.serialFilterFactory");
+
     static {
         // Enable logging
         System.setProperty("java.util.logging.config.file",
                 System.getProperty("test.src", ".") + "/logging.properties");
     }
 
-    public void initFaultTest() {
+    @DataProvider(name = "MethodsToCall")
-        String factoryName = System.getProperty("jdk.serialFilterFactory");
+    private Object[][] cases() {
-        ExceptionInInitializerError ex = Assert.expectThrows(ExceptionInInitializerError.class,
+        return new Object[][] {
-                () -> Config.getSerialFilterFactory());
+                {"getSerialFilterFactory", (Assert.ThrowingRunnable) () -> Config.getSerialFilterFactory()},
-        Throwable cause = ex.getCause();
+                {"setSerialFilterFactory", (Assert.ThrowingRunnable) () -> Config.setSerialFilterFactory(new NoopFactory())},
+                {"new ObjectInputStream(is)", (Assert.ThrowingRunnable) () -> new ObjectInputStream(new ByteArrayInputStream(new byte[0]))},
+                {"new OISSubclass()", (Assert.ThrowingRunnable) () -> new OISSubclass()},
+        };
+    }
+
+    /**
+     * Test each method that should throw IllegalStateException based on
+     * the invalid arguments it was launched with.
+     */
+    @Test(dataProvider = "MethodsToCall")
+    public void initFaultTest(String name, Assert.ThrowingRunnable runnable) {
+        IllegalStateException ex = Assert.expectThrows(IllegalStateException.class,
+                runnable);
+        final String msg = ex.getMessage();
 
         if (factoryName.equals("ForcedError_NoSuchClass")) {
-            Assert.assertEquals(cause.getClass(),
+            Assert.assertEquals(msg,
-                    ClassNotFoundException.class, "wrong exception");
+                    "invalid jdk.serialFilterFactory: ForcedError_NoSuchClass: java.lang.ClassNotFoundException: ForcedError_NoSuchClass", "wrong exception");
         } else if (factoryName.equals("SerialFactoryFaults$NoPublicConstructor")) {
-            Assert.assertEquals(cause.getClass(),
+            Assert.assertEquals(msg,
-                    NoSuchMethodException.class, "wrong exception");
+                    "invalid jdk.serialFilterFactory: SerialFactoryFaults$NoPublicConstructor: java.lang.NoSuchMethodException: SerialFactoryFaults$NoPublicConstructor.<init>()", "wrong exception");
         } else if (factoryName.equals("SerialFactoryFaults$ConstructorThrows")) {
-            Assert.assertEquals(cause.getClass(),
+            Assert.assertEquals(msg,
-                    IllegalStateException.class, "wrong exception");
+                    "invalid jdk.serialFilterFactory: SerialFactoryFaults$ConstructorThrows: java.lang.RuntimeException: constructor throwing a runtime exception", "wrong exception");
         } else if (factoryName.equals("SerialFactoryFaults$FactorySetsFactory")) {
-            Assert.assertEquals(cause.getClass(),
+            Assert.assertEquals(msg,
-                    IllegalStateException.class, "wrong exception");
+                    "invalid jdk.serialFilterFactory: SerialFactoryFaults$FactorySetsFactory: java.lang.IllegalStateException: Serial filter factory initialization incomplete", "wrong exception");
-            Assert.assertEquals(cause.getMessage(),
-                    "Cannot replace filter factory: initialization incomplete",
-                    "wrong message");
         } else {
             Assert.fail("No test for filter factory: " + factoryName);
         }
@@ -90,7 +108,7 @@ public class SerialFactoryFaults {
     public static final class ConstructorThrows
             implements BinaryOperator<ObjectInputFilter> {
         public ConstructorThrows() {
-            throw new IllegalStateException("SerialFactoryFaults$ConstructorThrows");
+            throw new RuntimeException("constructor throwing a runtime exception");
         }
 
         public ObjectInputFilter apply(ObjectInputFilter curr, ObjectInputFilter next) {
@@ -112,4 +130,21 @@ public class SerialFactoryFaults {
         }
     }
 
+    public static final class NoopFactory implements BinaryOperator<ObjectInputFilter> {
+        public NoopFactory() {}
+
+        public ObjectInputFilter apply(ObjectInputFilter curr, ObjectInputFilter next) {
+            throw new RuntimeException("NYI");
+        }
+    }
+
+    /**
+     * Subclass of ObjectInputStream to test subclassing constructor.
+     */
+    private static class OISSubclass extends ObjectInputStream {
+
+        protected OISSubclass() throws IOException {
+        }
+    }
+
 }