8170291: Unpredictable results of j.i.ObjectInputFilter::createFilter
authorrriggs
Wed, 07 Dec 2016 15:32:31 -0500
changeset 42446 397681315009
parent 42445 3786c0c57a66
child 42447 36db92901ae9
8170291: Unpredictable results of j.i.ObjectInputFilter::createFilter Reviewed-by: dfuchs
jdk/src/java.base/share/classes/java/io/ObjectInputFilter.java
jdk/src/java.base/share/classes/java/io/ObjectInputStream.java
jdk/test/java/io/Serializable/serialFilter/SerialFilterTest.java
--- a/jdk/src/java.base/share/classes/java/io/ObjectInputFilter.java	Wed Dec 07 12:47:19 2016 -0800
+++ b/jdk/src/java.base/share/classes/java/io/ObjectInputFilter.java	Wed Dec 07 15:32:31 2016 -0500
@@ -350,16 +350,24 @@
          * The first pattern that matches, working from left to right, determines
          * the {@link Status#ALLOWED Status.ALLOWED}
          * or {@link Status#REJECTED Status.REJECTED} result.
-         * If nothing matches, the result is {@link Status#UNDECIDED Status.UNDECIDED}.
+         * If the limits are not exceeded and no pattern matches the class,
+         * the result is {@link Status#UNDECIDED Status.UNDECIDED}.
          *
          * @param pattern the pattern string to parse; not null
-         * @return a filter to check a class being deserialized; may be null;
+         * @return a filter to check a class being deserialized;
          *          {@code null} if no patterns
-         * @throws IllegalArgumentException
-         *                if a limit is missing the name, or the long value
-         *                is not a number or is negative,
-         *                or the module name is missing if the pattern contains "/"
-         *                or if the package is missing for ".*" and ".**"
+         * @throws IllegalArgumentException if the pattern string is illegal or
+         *         malformed and cannot be parsed.
+         *         In particular, if any of the following is true:
+         * <ul>
+         * <li>   if a limit is missing the name or the name is not one of
+         *        "maxdepth", "maxrefs", "maxbytes", or "maxarray"
+         * <li>   if the value of the limit can not be parsed by
+         *        {@link Long#parseLong Long.parseLong} or is negative
+         * <li>   if the pattern contains "/" and the module name is missing
+         *        or the remaining pattern is empty
+         * <li>   if the package is missing for ".*" and ".**"
+         * </ul>
          */
         public static ObjectInputFilter createFilter(String pattern) {
             Objects.requireNonNull(pattern, "pattern");
@@ -402,14 +410,19 @@
              * Returns an ObjectInputFilter from a string of patterns.
              *
              * @param pattern the pattern string to parse
-             * @return a filter to check a class being deserialized; not null
+             * @return a filter to check a class being deserialized;
+             *          {@code null} if no patterns
              * @throws IllegalArgumentException if the parameter is malformed
              *                if the pattern is missing the name, the long value
              *                is not a number or is negative.
              */
             static ObjectInputFilter createFilter(String pattern) {
-                Global filter = new Global(pattern);
-                return filter.isEmpty() ? null : filter;
+                try {
+                    return new Global(pattern);
+                } catch (UnsupportedOperationException uoe) {
+                    // no non-empty patterns
+                    return null;
+                }
             }
 
             /**
@@ -417,8 +430,10 @@
              *
              * @param pattern a pattern string of filters
              * @throws IllegalArgumentException if the pattern is malformed
+             * @throws UnsupportedOperationException if there are no non-empty patterns
              */
             private Global(String pattern) {
+                boolean hasLimits = false;
                 this.pattern = pattern;
 
                 maxArrayLength = Long.MAX_VALUE; // Default values are unlimited
@@ -436,6 +451,7 @@
                     }
                     if (parseLimit(p)) {
                         // If the pattern contained a limit setting, i.e. type=value
+                        hasLimits = true;
                         continue;
                     }
                     boolean negate = p.charAt(0) == '!';
@@ -510,18 +526,9 @@
                         filters.add(c -> moduleName.equals(c.getModule().getName()) ? patternFilter.apply(c) : Status.UNDECIDED);
                     }
                 }
-            }
-
-            /**
-             * Returns if this filter has any checks.
-             * @return {@code true} if the filter has any checks, {@code false} otherwise
-             */
-            private boolean isEmpty() {
-                return filters.isEmpty() &&
-                        maxArrayLength == Long.MAX_VALUE &&
-                        maxDepth == Long.MAX_VALUE &&
-                        maxReferences == Long.MAX_VALUE &&
-                        maxStreamBytes == Long.MAX_VALUE;
+                if (filters.isEmpty() && !hasLimits) {
+                    throw new UnsupportedOperationException("no non-empty patterns");
+                }
             }
 
             /**
--- a/jdk/src/java.base/share/classes/java/io/ObjectInputStream.java	Wed Dec 07 12:47:19 2016 -0800
+++ b/jdk/src/java.base/share/classes/java/io/ObjectInputStream.java	Wed Dec 07 15:32:31 2016 -0500
@@ -1164,6 +1164,13 @@
      * for each class and reference in the stream.
      * The filter can check any or all of the class, the array length, the number
      * of references, the depth of the graph, and the size of the input stream.
+     * The depth is the number of nested {@linkplain #readObject readObject}
+     * calls starting with the reading of the root of the graph being deserialized
+     * and the current object being deserialized.
+     * The number of references is the cumulative number of objects and references
+     * to objects already read from the stream including the current object being read.
+     * The filter is invoked only when reading objects from the stream and for
+     * not primitives.
      * <p>
      * If the filter returns {@link ObjectInputFilter.Status#REJECTED Status.REJECTED},
      * {@code null} or throws a {@link RuntimeException},
@@ -1178,8 +1185,9 @@
      *
      * @implSpec
      * The filter, when not {@code null}, is invoked during {@link #readObject readObject}
-     * and {@link #readUnshared readUnshared} for each object
-     * (regular or class) in the stream including the following:
+     * and {@link #readUnshared readUnshared} for each object (regular or class) in the stream.
+     * Strings are treated as primitives and do not invoke the filter.
+     * The filter is called for:
      * <ul>
      *     <li>each object reference previously deserialized from the stream
      *     (class is {@code null}, arrayLength is -1),
--- a/jdk/test/java/io/Serializable/serialFilter/SerialFilterTest.java	Wed Dec 07 12:47:19 2016 -0800
+++ b/jdk/test/java/io/Serializable/serialFilter/SerialFilterTest.java	Wed Dec 07 15:32:31 2016 -0500
@@ -99,16 +99,6 @@
     @DataProvider(name="InvalidPatterns")
     static Object[][] invalidPatterns() {
         return new Object [][] {
-                {"maxrefs=-1"},
-                {"maxdepth=-1"},
-                {"maxbytes=-1"},
-                {"maxarray=-1"},
-                {"xyz=0"},
-                {"xyz=-1"},
-                {"maxrefs=0xabc"},
-                {"maxrefs=abc"},
-                {"maxrefs="},
-                {"maxrefs=+"},
                 {".*"},
                 {".**"},
                 {"!"},
@@ -121,11 +111,32 @@
     @DataProvider(name="Limits")
     static Object[][] limits() {
         // The numbers are arbitrary > 1
-        return new Object[][]{
+        return new Object[][] {
+                {"maxrefs", 1},     // 0 is tested as n-1
                 {"maxrefs", 10},
                 {"maxdepth", 5},
                 {"maxbytes", 100},
                 {"maxarray", 16},
+                {"maxbytes", Long.MAX_VALUE},
+        };
+    }
+
+    @DataProvider(name="InvalidLimits")
+    static Object[][] invalidLimits() {
+        return new Object[][] {
+                {"maxrefs=-1"},
+                {"maxdepth=-1"},
+                {"maxbytes=-1"},
+                {"maxarray=-1"},
+                {"xyz=0"},
+                {"xyz=-1"},
+                {"maxrefs=0xabc"},
+                {"maxrefs=abc"},
+                {"maxrefs="},
+                {"maxrefs=+"},
+                {"maxbytes=-1"},
+                {"maxbytes=9223372036854775808"},
+                {"maxbytes=-9223372036854775807"},
         };
     }
 
@@ -305,7 +316,7 @@
      * @param value a test value
      */
     @Test(dataProvider="Limits")
-    static void testLimits(String name, int value) {
+    static void testLimits(String name, long value) {
         Class<?> arrayClass = new int[0].getClass();
         String pattern = String.format("%s=%d;%s=%d", name, value, name, value - 1);
         ObjectInputFilter filter = ObjectInputFilter.Config.createFilter(pattern);
@@ -320,6 +331,21 @@
     }
 
     /**
+     * Test invalid limits.
+     * Construct a filter with the limit, it should throw IllegalArgumentException.
+     * @param pattern a pattern to test
+     */
+    @Test(dataProvider="InvalidLimits", expectedExceptions=java.lang.IllegalArgumentException.class)
+    static void testInvalidLimits(String pattern) {
+        try {
+            ObjectInputFilter filter = ObjectInputFilter.Config.createFilter(pattern);
+        } catch (IllegalArgumentException iae) {
+            System.out.printf("    success exception: %s%n", iae);
+            throw iae;
+        }
+    }
+
+    /**
      * Test that returning null from a filter causes deserialization to fail.
      */
     @Test(expectedExceptions=InvalidClassException.class)
@@ -332,7 +358,7 @@
                 }
             });
         } catch (InvalidClassException ice) {
-            System.out.printf("Success exception: %s%n", ice);
+            System.out.printf("    success exception: %s%n", ice);
             throw ice;
         }
     }
@@ -606,7 +632,7 @@
             }
             return array;
         } else if (pattern.startsWith("maxarray=")) {
-            return allowed ? new int[(int)value] : new int[(int)value+1];
+                return allowed ? new int[(int)value] : new int[(int)value+1];
         }
         Assert.fail("Object could not be generated for pattern: "
                 + pattern