EnumSet.class serialization broken - twice
Peter Levart
peter.levart at gmail.com
Sun Jul 7 14:31:55 UTC 2019
Hi Stuart,
I managed to find some time to tackle this bug.
What I found from inspecting the JDK code and experimenting is that it
doesn't matter how the private static final long serialVersionUID field
is initialized. Whether it is a compile-time constant or its value is
computed in <clinit>, the serialization/deserialization infrastructure
acts the same. It is using reflection to obtain the field's value and so
it can't distinguish between compile-time constant and computed value.
So ypur plan to eliminate serialVersionUID value from the stream of
"patched" EnumSet.class object falls apart.
I filed a bug [1] and prepared a patch [2] which just adds
serialVersionUID to EnumSet and initializes it to the same value that is
automatically computed in JDK 8 and before.
I had to revert the existing BogusEnumSet test to basically a version
that was actual at the time of JDK 8 release, undoing the edits of
byte[] value representing serialized stream that have been performed by
the same patches that broke the serialization of EnumSet.class objects.
I also added a test that verifies the de-serialization of a stream
containing EnumSet.class object, produced on JDK 8.
Although the patched EnumSet now contains explicit serialVersionUID,
serializing EnumSet.class with patched JDK 14 does not procude the same
stream, because the stream also contains descriptors of EnumSet fields
which changed from version in JDK 8. When deserializing EnumSet.class
object, only serialVersionUID from stream is checked against local
version though, so this doesn't matter.
Considering that this patch breaks serialization between already
released 9,10,11,12 versions of JDK and yet to be released versions that
would contain the patch, I think the patch should be backported to:
- 13
- 11u (11.0.4?)
(and possibly to other 9+ update versions yet to be released).
This would provide consistent serialization among latest releases of
actual versions including JDK 8 and before.
Do you think it is necessary to provide a fall-back system property or
just advise people to upgrade to latest update release in case they
experience trouble? Upgrading to latest update releases is pretty safe
these days and should not be a problem for anyone.
Regards,
Peter
[1] https://bugs.openjdk.java.net/browse/JDK-8227368
[2]
http://cr.openjdk.java.net/~plevart/jdk-dev/8227368_EnumSet.class_serialization/
On 6/29/19 2:00 AM, Stuart Marks wrote:
> Daniel Fuchs pointed me to a weird thing they had to do with the
> serialVersionUID in one of the JMX classes:
>
> https://hg.openjdk.java.net/jdk/jdk/file/c59f36ed7b52/src/java.management/share/classes/javax/management/MBeanAttributeInfo.java#l46
>
>
> Essentially the svuid for this class initialized in a static block,
> and its value is conditional based on the value of some system
> property. I don't think using a property is necessary for the EnumSet
> case. However, it does point out something interesting, which is that
> if the svuid is not initialized with a compile-time constant, and
> instead via a static block, the value doesn't appear in
> serialized-form.html.
>
> Thus, we can backport a change to JDK 11 that changes EnumSet's svuid,
> without changing the Java SE 11 specification! (This is analogous to
> changing a method's implementation to behave the way we want to,
> without changing the method's specification to specify that it behaves
> that way. On the other hand, this is a really sleazy hack.)
>
> Here's an outline of what we can do:
>
> 1) Add EnumSet.serialVersionUID to JDK 13 in the usual way, specifying
> a constant that's the same as the JDK 8 value. We are after RDP1 but I
> think this change is well-justified. This change should automatically
> be propagated to JDK 14.
>
> 2) "Backport" the fix to JDK 11, but assign the value in a static
> initializer block instead of as a constant in a field initializer.
> Something like this:
>
>
> diff -r 27d4b8acbe07 src/java.base/share/classes/java/util/EnumSet.java
> --- a/src/java.base/share/classes/java/util/EnumSet.java Thu Jun 13
> 17:46:57 2019 -0700
> +++ b/src/java.base/share/classes/java/util/EnumSet.java Fri Jun 28
> 16:42:03 2019 -0700
> @@ -76,11 +76,18 @@
> * @since 1.5
> * @see EnumMap
> */
> - at SuppressWarnings("serial") // No serialVersionUID due to usage of
> - // serial proxy pattern
> + at SuppressWarnings("serial") // serialVersionUID is not a compile-time
> constant
> public abstract class EnumSet<E extends Enum<E>> extends AbstractSet<E>
> implements Cloneable, java.io.Serializable
> {
> + // Initialize the serialVersionUID to be compatible with JDK 8.
> + // Do this from a static block to avoid having the value appear
> + // in serialized-form.html. See JDK-xxxxxxx.
> + private static final long serialVersionUID;
> + static {
> + serialVersionUID = 1009687484059888093L;
> + }
> +
> /**
> * The class of all the elements of this set.
> */
>
> 3) Backport the fix to JDK 12. I'm not sure this is absolutely
> necessary, since JDK 12 likely has a short lifetime, but if we're
> doing 11 and 13 it makes sense to do 12 as well (though mainly for
> completeness).
>
> 4) It's unclear whether a similar patch as above needs to be added to
> JDK 8. Since it already has the right svuid, we could get away without
> doing anything. However, with backports continuing in the 8u release
> family, it might be prudent to apply this patch in order to prevent
> future backports to 8u from inadvertently changing the svuid -- which
> as you point out did happen in 9 and 10.
>
> 5) I don't think we need to patch JDK 1.6, 7, 9, or 10 but the
> maintainers of those releases can certainly decide to do so.
>
> =====
>
> In any case, I think doing the above will result in consistent
> EnumSet.serialVersionUID values for JDK 8 LTS, JDK 11 LTS, and current
> JDK releases, without having to worry about spec changes for any of
> the past releases.
>
> Let me know how you'd like to proceed. I'm happy to help out with
> reviewing, filing bugs, CSRs, etc.
>
> s'marks
>
>
>
>
>
>
> On 6/27/19 2:57 PM, Stuart Marks wrote:
>> Arrrrrggggh. Yet another serialization bug.
>>
>> But yes, this is a bug. Thanks for finding and diagnosing it.
>>
>> Like Martin, I've often forgotten that classes themselves can be
>> included in a serial stream, as well as instances of those classes.
>> In fact I seem to recall arguing that because EnumSet uses the
>> serialization proxy pattern, instances of it should never appear in a
>> legitimate serial stream. I think that's true. However, I sent on to
>> say that because of this, there is no issue with serialization
>> compatibility, and thus EnumSet didn't need a serialVersionUID.
>> That's incorrect.
>>
>> I'm uncomfortable with relaxing the serialization spec and mechanism
>> to allow a class in the serial stream to have a different svuid from
>> the one loaded in the running JVM. Offhand I don't know what problems
>> it could cause, but it seems like a fundamental change that would
>> lead to problems at some point.
>>
>> Also, this is a problem with one class (so far...) and I don't think
>> we should change the whole serialization mechanism to support it.
>>
>> I'm thus leaning toward your first suggestion of adding a
>> serialVersionUID declaration to EnumSet that matches the value from
>> JDK 8. This would go into the current repo (JDK 14) and likely be
>> backported to JDK 13.
>>
>> It seems like a no-brainer to backport this to JDK 11 as well; this
>> would provide broad compatibility across JDK 8 LTS, JDK 11 LTS, and
>> current JDK releases. However, changing the svuid is a specification
>> change. More investigation is necessary to figure out what would be
>> involved in doing this.
>>
>> Meanwhile, it would seem sensible to file a bug and start on a fix
>> for the current release(s). Would you be able to do that?
>>
>> Again, thanks for finding this.
>>
>> s'marks
>>
>> On 6/18/19 7:32 AM, Peter Levart wrote:
>>>
>>>
>>> On 6/18/19 4:00 PM, Martin Buchholz wrote:
>>>> Java Historian says:
>>>> I was a reviewer for Effective Java 3rd Edition and EnumSet is the
>>>> canonical example of the Serialization Proxy pattern,
>>>> so I tried to make sure the pattern was implemented as perfectly as
>>>> possible.
>>>> 8192935: Fix EnumSet's SerializationProxy javadoc
>>>> All of us who try to make java serialization work right have a
>>>> mental model of the many things that might go wrong.
>>>> Serialization of Class objects has never been part of my own mental
>>>> model - I've only ever considered instances.
>>>
>>> Perhaps the necessity for Class objects representing Serializable
>>> classes to agree in sertialVersionUID is a bug in Java serialization
>>> implementation? There's no such requirement for Class objects
>>> representing non-Serializable classes and I don't see why this
>>> requirement is there for Serializable classes. Could this
>>> requirement simply be relaxed with no ill consequences?
>>>
>>> Regards, Peter
>>>
>>>>
>>>>
>>>> On Tue, Jun 18, 2019 at 5:32 AM Peter Levart
>>>> <peter.levart at gmail.com <mailto:peter.levart at gmail.com>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> I recently stumbled on an exception thrown when deserializing
>>>> stream
>>>> produced on JDK 8 and read with JDK 11. I narrowed the problem
>>>> down to
>>>> serialization/deserialization of a public EnumSet.class object.
>>>> There
>>>> were several changes made to EnumSet in the Mercurial history
>>>> of jdk
>>>> repo, but I think the following two broke the serialization:
>>>>
>>>> http://hg.openjdk.java.net/jdk/jdk/rev/d0e8542ef650
>>>> http://hg.openjdk.java.net/jdk/jdk/rev/a7e13065a7a0
>>>>
>>>> It is interesting to note that before those two changes were made,
>>>> there
>>>> was a chance to fix the problem reported by newly added serial
>>>> lint
>>>> warnings. Unfortunately they were just silenced:
>>>>
>>>> http://hg.openjdk.java.net/jdk/jdk/rev/501d8479f798
>>>>
>>>> + at SuppressWarnings("serial") // No serialVersionUID due to
>>>> usage of
>>>> + // serial proxy pattern
>>>>
>>>> It is true that serialization of instances of Serializable
>>>> classes is
>>>> not broken by changes to them when they implement serial proxy
>>>> pattern
>>>> (i.e. writeReplace() method) even if they don't itself declare a
>>>> private
>>>> static final long serialVersionUID field, but this is not true of
>>>> Class
>>>> objects representing those Serializable classes. It is even more
>>>> controversial that serialization of Class objects representing
>>>> non-Serializable classes is never broken (which is
>>>> understandable as
>>>> they don't have a habit of declaring serialVersionUID fields).
>>>>
>>>> Both of the above braking changes were made post JDK 8 release, so
>>>> deserialization of JDK 8 (and older) streams is affected in all
>>>> JDK 9 +
>>>> releases or vice versa.
>>>>
>>>> So, what shall be done. I suggest adding serialVersionUID field to
>>>> EnumSet vith a value that corresponds to JDK 8 serialization
>>>> format and
>>>> later backport this change to JDK 11.
>>>>
>>>> What do you think?
>>>>
>>>>
>>>> Regards, Peter
>>>>
>>>>
>>>> PS: ImmutableCollections nested classes also implement serial
>>>> proxy
>>>> pattern and don't declare serialVersionUID fields, but they are
>>>> not
>>>> public, so it is less chance that Class objects representing them
>>>> could
>>>> be used in serial streams, although it is not impossible. For
>>>> example:
>>>>
>>>> objectOutputStream.writeObject(Set.of().getClass());
>>>>
>>>
More information about the core-libs-dev
mailing list