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

[Jaikiran Pai]

On Wed, 1 Dec 2021 18:19:05 GMT, Roger Riggs <rriggs at 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