Another final thought that just occurred to me… java.io.SerializablePermission will need its class-level javadoc updated to include the new permission target name.
-Chris. > 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 >>> >
