On Wed, 1 Dec 2021 18:19:05 GMT, Roger Riggs <rri...@openjdk.org> wrote:
> The effects of invalid values of `jdk.serialFilter` and > `jdk.serialFilterFactory` properties are > incompletely specified. The behavior for invalid values of the properties is > different and > use an unconventional exception type, `ExceptionInInitializerError` and leave > the `OIF.Config` class > uninitialized. > > The exceptions in the `ObjectInputFilter.Config` class initialization caused > by invalid values of the two properties, > either by system properties supplied on the command line or security > properties are logged. > The `Config` class marks either or both the filter and filter factory values > as unusable > and remembers the exception message. > > Subsequent calls to the methods that get or set the filter or filter factory > or create > an `ObjectInputStream` throw `java.lang.IllegalStateException` with the > remembered exception message. > Constructing an `ObjectInputStream` calls both `Config.getSerialFilter` and > `Config.getSerialFilterFactory`. > The nature of the invalid property is reported as an `IllegalStateException` > on first use. > > This PR supercedes https://github.com/openjdk/jdk/pull/6508 Document that > setting an invalid property jdk.serialFilter disables deserialization src/java.base/share/classes/java/io/ObjectInputFilter.java line 526: > 524: * {@code jdk.serialFilter} is defined then it is used to configure > the filter. > 525: * The filter is created as if {@link #createFilter(String) > createFilter} is called, > 526: * if the filter string is invalid the initialization fails and > subsequent attempts to Hello Roger, > if the filter string is invalid the initialization fails Should this instead be "if the filter string is invalid the initialization of {@code Config} class fails ..." src/java.base/share/classes/java/io/ObjectInputFilter.java line 527: > 525: * The filter is created as if {@link #createFilter(String) > createFilter} is called, > 526: * if the filter string is invalid the initialization fails and > subsequent attempts to > 527: * {@linkplain Config#getSerialFilter() get the filter}, {@link > Config#setSerialFilter set a filter}, > {@link Config#setSerialFilter set a filter} Should this instead be "{@link Config#setSerialFilter(ObjectInputFilter) set a filter}" i.e. include the param as part of the `@link`, like in other places? src/java.base/share/classes/java/io/ObjectInputFilter.java line 532: > 530: * invalid serial filter. > 531: * If the system property {@code jdk.serialFilter} or the {@link > java.security.Security} > 532: * property is not set the filter can be set with > or the {@link java.security.Security} property is not set the filter can be > set ... Is it intentional that the name of security property is left out here? Perhaps this should be: `or the {@link java.security.Security} property {@code jdk.serialFilter} is not set the filter ....` or even: `or the {@link java.security.Security} property of the same name is not set the filter....` src/java.base/share/classes/java/io/ObjectInputFilter.java line 751: > 749: if (serialFilter != null) { > 750: throw new IllegalStateException("Serial filter can > only be set once"); > 751: } else if (invalidFilterMessage != null) { The `invalidFilterMessage` is a `static` `final` `String` which gets initialized during the static initialization of the class and as such doesn't get changed after that. Do you think this lock on `serialFilterLock` is necessary for this check or it can be moved outside this `synchronized` block? src/java.base/share/classes/java/io/ObjectInputFilter.java line 859: > 857: /* > 858: * Return message for an invalid filter factory configuration > saved from the static init. > 859: * It can be called before the static initializer is complete > and has set the message/null. More of a curiosity than a review comment - Is that because the `Config.getSerialFilterFactory()/setSerialFilterFactory(...)` could be called from the constructor of the user configured factory class, which gets invoked when static initialization is in progress for the `Config` class? test/jdk/java/io/Serializable/serialFilter/InvalidGlobalFilterTest.java line 38: > 36: /* > 37: * @test > 38: * @bug 8269336 Would this need an update to include the new bug id of this PR? ------------- PR: https://git.openjdk.java.net/jdk/pull/6645
