RFR: JDK-8068721 - RMI-IIOP communication fails when ConcurrentHashMap is passed to remote method

[Martin Buchholz]

jsr166 CVS now contains this small improvement (I verified the emitted
bytes are unchanged):

--- src/main/java/util/concurrent/ConcurrentHashMap.java 23 Mar 2015
18:48:19 -0000 1.269
+++ src/main/java/util/concurrent/ConcurrentHashMap.java 24 Mar 2015
22:28:58 -0000
@@ -1373,9 +1373,10 @@
             new Segment<?,?>[DEFAULT_CONCURRENCY_LEVEL];
         for (int i = 0; i < segments.length; ++i)
             segments[i] = new Segment<K,V>(LOAD_FACTOR);
-        s.putFields().put("segments", segments);
-        s.putFields().put("segmentShift", segmentShift);
-        s.putFields().put("segmentMask", segmentMask);
+        java.io.ObjectOutputStream.PutField streamFields = s.putFields();
+        streamFields.put("segments", segments);
+        streamFields.put("segmentShift", segmentShift);
+        streamFields.put("segmentMask", segmentMask);
         s.writeFields();

         Node<K,V>[] t;


On Tue, Mar 24, 2015 at 2:38 AM, Chris Hegarty <chris.hegarty at oracle.com>
wrote:

> Martin,
>
> On 23 Mar 2015, at 22:15, Martin Buchholz <martinrb at google.com> wrote:
>
> > Let us know if the serialization code of the collection classes can be
> > improved.
>
> I think there are a few minor cleanups that would be beneficial in CHM:
>
>  1) Add @serialField so that the serializable stream fields show up in the
>      javadoc ( serial form ), since they are still part of the serial
> form, even
>      though not used in the implementation. This is just documenting
>      existing behaviour/form.
>
>  2) Mark correctly identified a small hole in the putFields() spec, which
>      should be fixed. A minor, benign, change in CHM writeObject
>      can avoid this ( spec ambiguity ).
>
> Please consider the following change:
>
> diff --git
> a/src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java
> b/src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java
> ---
> a/src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java
> +++
> b/src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java
> @@ -35,6 +35,7 @@
>
>  package java.util.concurrent;
>
> +import java.io.ObjectOutputStream;
>  import java.io.ObjectStreamField;
>  import java.io.Serializable;
>  import java.lang.reflect.ParameterizedType;
> @@ -599,7 +600,12 @@
>      /** Number of CPUS, to place bounds on some sizings */
>      static final int NCPU = Runtime.getRuntime().availableProcessors();
>
> -    /** For serialization compatibility. */
> +    /**
> +     * For serialization compatibility.
> +     * @serialField segments Segment[]
> +     * @serialField segmentMask int
> +     * @serialField segmentShift int
> +     */
>      private static final ObjectStreamField[] serialPersistentFields = {
>          new ObjectStreamField("segments", Segment[].class),
>          new ObjectStreamField("segmentMask", Integer.TYPE),
> @@ -1400,9 +1406,10 @@
>              new Segment<?,?>[DEFAULT_CONCURRENCY_LEVEL];
>          for (int i = 0; i < segments.length; ++i)
>              segments[i] = new Segment<K,V>(LOAD_FACTOR);
> -        s.putFields().put("segments", segments);
> -        s.putFields().put("segmentShift", segmentShift);
> -        s.putFields().put("segmentMask", segmentMask);
> +        ObjectOutputStream.PutField streamFields = s.putFields();
> +        streamFields.put("segments", segments);
> +        streamFields.put("segmentShift", segmentShift);
> +        streamFields.put("segmentMask", segmentMask);
>          s.writeFields();
>
>          Node<K,V>[] t;
>
> -Chris.
>
> > On Mon, Mar 23, 2015 at 2:25 PM, Mark Sheppard <mark.sheppard at oracle.com
> >
> > wrote:
> >
> >> Hi
> >> please oblige and review the following fix
> >>
> >> http://cr.openjdk.java.net/~msheppar/8068721/jdk9/corba/webrev/
> >> http://cr.openjdk.java.net/~msheppar/8068721/jdk9/jdk/webrev/
> >>
> >>
> >> which addresses the issue in
> >> https://bugs.openjdk.java.net/browse/JDK-8068721
> >>
> >> This relates to RMI-IIOP and the interplay between custom marshalling of
> >> ValueTypes and
> >> the corresponding serialization of a Java class, in this case
> >> ConcurrentHashMap.
> >>
> >> ConcurrentHashMap changed its structure in jdk8 from that used in jdk7.
> >> This resulted in modification to the readObject and writeObject methods,
> >> and in particular, former serial fields were removed, resulting in
> >> writeObject using PutField and readObject using defaultReadObject.
> >> The writeObject invokes the putFields method of an ObjectOutputStream
> >> multiple times, and assumes
> >> that it will receive the same PutField object instance for each
> >> invocation. The spec
> >> doesn't endorse this behaviour - but that's what the implementation of
> >> ObjectOutputStream
> >> provides. However in the case of RMI-IIOP, the OutputStreamHook, a
> >> subclass of ObjectOutputStream, returns a new instance for each
> >> putFields invocation. Thus, the ConcurrentHashMap writeObject results in
> >> improper serialization in the context
> >> of RMI-IIOP.
> >> In the unmarshalling flow of ConcurrentHashMap, the readObject now uses
> >> defaultReadObject rather than GetField
> >> In this call flow the IIOPInputStream attempts to ignore any primitive
> >> field, in the serialized data, that doesn't
> >> correspond with a reflective field of the object. However, it leaves the
> >> primitive in the stream.
> >> Thus, in the case of ConcurrentHashMap, which has serialized two
> integers
> >> and
> >> an array of Segments (subclass of ReentrantLock), this results in
> erroneous
> >> deserialization, with a misinterpretation of a value tag in the stream
> as
> >> an array length
> >> and an attempt to allocate a very large array ensues, with an exception
> >> being thrown.
> >>
> >> The OutputStreamHook now returns the same instance of PutField allocated
> >> for each separate call of putFields method.
> >> This highlights a need to tighten up and disambiguate the
> >> OutputObjectStream spec for putFields.
> >>
> >> IIOPInputStream now removes the primitive values from the stream.
> >>
> >> regards
> >> Mark
> >>
>
>