On 25 Jul 2016, at 19:55, Roger Riggs <roger.ri...@oracle.com> wrote:
Hi Chris,
Thanks for the review and comments,
Updates in place:
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-serial-filter-jdk9-8155760/
SpecDiff:
http://cr.openjdk.java.net/~rriggs/filter-diffs/overview-summary.html
Javadoc (subset)
http://cr.openjdk.java.net/~rriggs/filter-javadoc/java/io/ObjectInputStream.html
http://cr.openjdk.java.net/~rriggs/filter-javadoc/java/io/ObjectInputFilter.html
On 7/25/2016 10:54 AM, Chris Hegarty wrote:
Roger,
Mainly looks good. Some comments on the spec:
- Use the Present Simple Tense consistently, e.g.
"Return*S* an ObjectInputFilter computed from a string of patterns."
ok
- ObjectInputFilter. Was there a comment already on the use of links?
For example the following is showing in the javadoc:
"ObjectInputStream.setObjectInputFilter(java.io.ObjectInputFilter)"
when "setObjectInputFilter" would be better.
ok, will fix (It would be nice if javadoc had a @linksimple that didn't supply
the whole signature).
Same comment applies to the links to ObjectInputFilter.Status, and
other places.
- ObjectInputFilter class description. "If set on an
ObjectInputStream, the method*(s)* are called ..."
ok
- Looking at the example in the ObjectInputFilter class description
makes me think that maybe the default process-wide filter should
be a filter that simply returns UNDECIDED, rather than being null.
Is it important to discern whether, or not, it has been set?
When writing a customized filter, it is useful to know whether the process-wide
filter is has been configured.
Usually it is not and there will be a (slight) performance improvement in not
calling it and checking the return.
- ObjectInputFilter.Config
The initial sentence in the class description should describe the
class itself, so maybe " A utility class for ..."
ok
- "process-wide" is this an agreed upon term? I'm just curious where
it came from. Is there a more common term for this?
It is useful to distinguish between the filter applied by default to all
ObjectInputStreams from
one set for a particular stream. I initially used 'global' but that seemed
overly broad.
The description is used sparingly but I'm open to suggestions.
- Config.setSerialFilter: SecurityException - if there is security
manager and the SerializablePermission("serialFilter") is not
granted or if there is no securityManager set and the process-wide
filter has already been set non-null
It is a little odd to throw a SE if there is no SM, no ?
True, that would be better as IllegalStateException; updated
- Is there a class/package level statement covering null, or should
it be covered for each applicable method?
Added one.
- ObjectInputStream
"... the serialization filter for the stream." ->
"... the serialization filter for THIS stream."
ok
- setObjectInputFilter: "The checkInput method is called for each
class and reference in the stream". Does this apply to back
references too?
yes, a reference in the stream, as opposed to a new instance in the stream,
refers to back references.
- setObjectInputFilter: "... when the ObjectInputStream is constructed
and CANNOT be re-set until an object has been deserialized."
The intent was to prevent it from being modified during deserialization.
But I think it will be clearer if it can only be set non-null once and only if
the previous
value was the pre-configured process-wide filter.
I've also had the recommendation that ObjectInputStream.setObjectInputFilter
should be protected by the same permission as configuring the process-wide
filter.
(SerializablePermission("serialFilter"))
Roger
-Chris.
On 19/07/16 15:02, Roger Riggs wrote:
Please review the design, implementation, and tests of JEP 290: Filter
Incoming Serialization Data[1]
It allows incoming streams of object-serialization data to be filtered
in order to improve both security and robustness.
The JEP[1] has more detail on the background and scope.
The core mechanism is a filter interface implemented by serialization
clients and set on an |ObjectInputStream|. The filter is called during
the deserialization process to validate the classes being deserialized,
the sizes of arrays being created, and metrics describing stream length,
stream depth, and number of references as the stream is being decoded.
A process-wide filter can be configured that is applied to every
ObjectInputStream.
The API of ObjectInputStream can be used to set a custom filter to
supersede or augment the process-wide filter.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-serial-filter-jdk9-8155760/
SpecDiff:
http://cr.openjdk.java.net/~rriggs/filter-diffs/overview-summary.html
Javadoc (subset)
http://cr.openjdk.java.net/~rriggs/filter-javadoc/java/io/ObjectInputStream.html
http://cr.openjdk.java.net/~rriggs/filter-javadoc/java/io/ObjectInputFilter.html
Comments appreciated, Roger
[1] JEP 290: https://bugs.openjdk.java.net/browse/JDK-8154961
