Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vector should have SerialVersionUID, and other problems with SI-8549 discovered by the Xcheckinit build. #8576

Closed
scabug opened this issue May 10, 2014 · 10 comments
Assignees

Comments

@scabug
Copy link

scabug commented May 10, 2014

As noticed in the instability of the new test for #8549 under -Xcheckinit:

https://scala-webapps.epfl.ch/jenkins/view/2.12.x/job/scala-nightly-checkinit-2.12.x/

Marking as a blocker for 2.11.1 as the release is intended to fix serialization issues.

@scabug
Copy link
Author

scabug commented May 10, 2014

Imported From: https://issues.scala-lang.org/browse/SI-8576?orig=1
Reporter: @retronym
See #7838, #8549

@scabug
Copy link
Author

scabug commented May 10, 2014

@retronym said:
I've identified the parts of the test that are unstable under the checkinit build. For now, we'll disable them: scala/scala#3739

@scabug
Copy link
Author

scabug commented May 19, 2014

@retronym said:
Pushing this down to 2.12

@scabug
Copy link
Author

scabug commented Jul 26, 2016

@szeiger said:
I did some digging in the test failures, class files and specs. SerialVersionUID is only the tip of the iceberg.

I think we first need to be clear what kind of stability to expect. t8549 currently checks that a serialized object representation is the same as a previously computed reference. This is inevitably bound to fail under Xcheckinit if any of the involved classes requires the addition of bitmap fields. The additional fields are serialized, which changes the representation. The only classes that are stable in this way between builds with and without Xcheckinit are the ones that either define their own serialization format or are not affected by Xcheckinit (this is the reason why List is stable but Vector is not)

How useful is this serialization of the bitmaps anyway? t8549 also checks that deserializing a representation without bitmap fields (the reference) is equal to a freshly constructed object, so we cannot rely on the presence of these bitmaps in the serialized representation. If they are missing, the Xcheckinit code will fail because it sees all fields in the deserialized object as uninitialized. Let's assume we could rely on the presence of the bitmap fields. How do you interpret them in a serialized representation? They would have to adhere to the usual versioning compatibility rules (https://docs.oracle.com/javase/7/docs/platform/serialization/spec/version.html#6678), which they don't.

One way out would be to forego the safety provided by Xcheckinit for deserialized objects: Under Xcheckinit, make the bitmap fields transient and add a private readResolve method (or modify an existing one) to set all bits. This gives us serialization compatibility between builds with and without Xcheckinit, doesn't break the versioning rules, and prevents false positive unitialized checks in deserialized objects. There's just one caveat: We cannot add readResolve if some other class in the hierarchy (including unknowable subclasses) has a readResolve method.

For final classes and sealed hierarchies in the standard library, we can solve the problem on a case-by-case basis by implementing Externalizable instead of relying on Java's standard serialization.

I don't see any good solution for the general case. Objects serialized with Xcheckinit should be readable without Xcheckinit, but the opposite is not true. The serialized representation will not be stable, either. Only deserialized objects can be resonably compared.

@scabug
Copy link
Author

scabug commented Jul 27, 2016

@szeiger said:
PR for 2.12: scala/scala#5306

Rescheduling to 2.13. We should consider using a non-standard serialization scheme via "Externalizable" for the new Scala collection to solve this at least for the collection classes.

@scabug
Copy link
Author

scabug commented Aug 12, 2016

@SethTisue said (edited on Aug 12, 2016 2:36:31 PM UTC):
making the fields volatile transient seems appealing to me. the lost “safety” seems to me like nothing anyone would have expected or counted on anyway. I would actually have expected the fields to be volatile transient in the first place.

I'm not sure I've understood it fully, but perhaps the issue with readResolve is something we could simply document?

@scabug
Copy link
Author

scabug commented Aug 12, 2016

@SethTisue said:
what about reversing the current meaning of the bits, so that the bits start at 1, and get set to 0 later? that would remove the need to set the bits at deserialization time.

@scabug
Copy link
Author

scabug commented Aug 12, 2016

@szeiger said:
I assume you mean transient, not volatile. The confusion was probably caused by looking at my attempted fix for #7838 (where I made the same mistake) just before commenting on this ticket.

Reversing the meaning of the bits might work. Field initializers are not run before deserialization. You could use a field initializer to set the bits upon proper construction of the object and end up with unset bits after deserialization.

@szeiger
Copy link
Member

szeiger commented May 30, 2018

Removing the "collections" label because this is not really about Vector or collections. In light of https://www.bleepingcomputer.com/news/security/oracle-plans-to-drop-java-serialization-support-the-source-of-most-security-bugs/ and with Java serialization being mostly abandoned in favor of newer protocols anyway I'm not sure we should invest time into this corner case (serialization compatibility between checkinit and non-checkinit builds) at all.

@SethTisue
Copy link
Member

SethTisue commented Jan 14, 2021

I'm not sure we should invest time into this corner case (serialization compatibility between checkinit and non-checkinit builds) at all.

agree, in fact, I think we should just close this ticket — but I think @dwijnand may want to double check with Lukas first, in case I'm missing something

@lrytz lrytz closed this as completed Jan 18, 2021
@SethTisue SethTisue removed this from the Backlog milestone Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants