Draft: 8387350

[jaikiran]

WIP

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (1 review required, with at least 1 Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/2594/head:pull/2594
$ git checkout pull/2594

Update a local copy of the PR:
$ git checkout pull/2594
$ git pull https://git.openjdk.org/valhalla.git pull/2594/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2594

View PR using the GUI difftool:
$ git pr show -t 2594

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2594.diff

[bridgekeeper]

👋 Welcome back jpai! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[liach]

This call sounds a bit risky - I doubt this might change preexisting serialization behavior. Also I don't think this should come before hasWriteObjectMethod in particular, might break writeReplace delegation too.

[viktorklang-ora]

Correct me if I'm wrong, but this check would run on-each-write? Seems like a check like that should be cached, as I'm concerned about perf regressions as well.

[liach]

The checkDeserialize for the original class is already run on each write.

[viktorklang-ora]

A compromise might be opening up an issue to improve this (both from a regression-perspective, and in the case for deserialize it would be a potential avenue for improving performance).

[liach]

This is the patch I am considering right now:

diff --git a/src/java.base/share/classes/java/io/ObjectStreamClass.java b/src/java.base/share/classes/java/io/ObjectStreamClass.java
index e4d21792667..7d841540a1a 100644
--- a/src/java.base/share/classes/java/io/ObjectStreamClass.java
+++ b/src/java.base/share/classes/java/io/ObjectStreamClass.java
@@ -123,7 +123,9 @@ protected Map<FieldReflectorKey, FieldReflector> computeValue(Class<?> type) {
     /** true if represents record type */
     private boolean isRecord;
     /** true if represented class cannot use allocate-and-fill deserialization,
-     * due to value class or strict field initialization restrictions. */
+     * due to value class or strict field initialization restrictions.
+     * Such a class either has a deserializer or has both serialize/deserialize
+     * exceptions once initialized. */
     private boolean requiresDeserializer;
     /** true if represented class implements Serializable */
     private boolean serializable;
@@ -350,14 +352,21 @@ private ObjectStreamClass(final Class<?> cl) {
         isProxy = Proxy.isProxyClass(cl);
         isEnum = Enum.class.isAssignableFrom(cl);
         isRecord = cl.isRecord();
-        requiresDeserializer = cl.isValue() || ValueClass.hasStrictInstanceField(cl);
         serializable = Serializable.class.isAssignableFrom(cl);
         externalizable = Externalizable.class.isAssignableFrom(cl);
 
+        // Note: AVC with no strict instance field like java.lang.Number is allowed;
+        // non-serializable superclasses are allowed because their constructors are called
+        requiresDeserializer = serializable && (ValueClass.isConcreteValueClass(cl) || ValueClass.hasStrictInstanceField(cl));
+
         Class<?> superCl = cl.getSuperclass();
         superDesc = (superCl != null) ? lookup(superCl, false) : null;
         localDesc = this;
 
+        if (superDesc != null) {
+            requiresDeserializer |= superDesc.requiresDeserializer;
+        }
+
         if (serializable) {
             if (isEnum) {
                 suid = 0L;
@@ -379,14 +388,15 @@ private ObjectStreamClass(final Class<?> cl) {
                     canonicalCtr = canonicalRecordCtr(cl);
                     cachedRecordConstructors = new RecordConstructorsCache();
                 } else if (requiresDeserializer) {
-                    if (!Modifier.isAbstract(cl.getModifiers())) {
-                        // Serializable value classes that appear in streams should
-                        // have a factory annotated with @Deserializer
-                        deserializer = findDeserializer(cl, fields);
-                    }
+                    // Concrete value classes and classes with strict instance
+                    // fields must not breach their integrity with the serializable
+                    // constructor. Make sure they fail also upon serialization
+                    // in addition to deserialization if they don't have a
+                    // correct internal @Deserializer
+                    deserializer = findDeserializer(cl, fields);
                     if (deserializer == null) {
                         serializeEx = deserializeEx = new ExceptionInfo(cl.getName(),
-                                                                        "cannot serialize value class");
+                                "cannot serialize due to concrete value class or strict instance fields");
                     }
                 } else if (externalizable) {
                     cons = getExternalizableConstructor(cl);
@@ -422,7 +432,7 @@ private ObjectStreamClass(final Class<?> cl) {
         if (deserializeEx == null) {
             if (isEnum) {
                 deserializeEx = new ExceptionInfo(name, "enum type");
-            } else if (cons == null && !(isRecord || requiresDeserializer)) {
+            } else if (cons == null && !isRecord && deserializer == null) {
                 deserializeEx = new ExceptionInfo(name, "no valid constructor");
             }
         }
diff --git a/test/jdk/java/io/Serializable/valueObjects/SimpleValueGraphs.java b/test/jdk/java/io/Serializable/valueObjects/SimpleValueGraphs.java
index cc11d7e95b3..3b4b633e8af 100644
--- a/test/jdk/java/io/Serializable/valueObjects/SimpleValueGraphs.java
+++ b/test/jdk/java/io/Serializable/valueObjects/SimpleValueGraphs.java
@@ -360,7 +360,7 @@ public String toString(int depth) {
     void testExternalizableNotSer() {
         var obj = new ValueExt();
         var ex = Assertions.assertThrows(InvalidClassException.class, () -> serialize(obj));
-        Assertions.assertEquals("SimpleValueGraphs$ValueExt; cannot serialize value class", ex.getMessage());
+        Assertions.assertEquals("SimpleValueGraphs$ValueExt; cannot serialize due to concrete value class or strict instance fields", ex.getMessage());
     }
 
     @Test
@@ -369,7 +369,7 @@ void testExternalizableNotDeser() throws IOException {
         byte[] bytes = serialize(obj);
         byte[] newBytes = patchBytes(bytes, "IdentExt", "ValueExt");
         var ex = Assertions.assertThrows(InvalidClassException.class, () -> deserialize(newBytes));
-        Assertions.assertTrue(ex.getMessage().contains("cannot serialize value class"));
+        Assertions.assertTrue(ex.getMessage().contains("cannot serialize due to concrete value class or strict instance fields"));
     }
 
     // Exception trying to serialize

[jaikiran]

This code is very new to me and (understandably) complex. So my proposed change will need many more eyes for me to be confident with it.

Chen, if you are comfortable with the change you are suggesting, I'll go ahead and assign the issue to you and it's OK with me if you take it forward in a new PR. I can close this draft.

[liach]

Thanks Jaikiran. I will wait for your serialization test improvements to be ready first, as that sets the ground for the new tests we need to introduce here.