RFR: 8333796: Add missing serialization functionality to sun.reflect.ReflectionFactory [v3]

[Roger Riggs]

On Thu, 29 Aug 2024 14:33:08 GMT, David M. Lloyd <duke at openjdk.org> wrote:

>> Issue [JDK-8164908](https://bugs.openjdk.org/browse/JDK-8164908) added support for functionality required to continue to support IIOP and custom serializers in light of additional module-based restrictions on reflection. It was expected that these libraries would use `sun.misc.Unsafe` in order to access fields of serializable classes. However, with JEP 471, the methods necessary to do this are being removed.
>> 
>> To allow these libraries to continue to function, it is proposed to add two methods to `sun.reflect.ReflectionFactory` which will allow serialization libraries to acquire a method handle to generated `readObject`/`writeObject` methods which set or get the fields of the serializable class using the serialization `GetField`/`PutField` mechanism. These generated methods should be used by serialization libraries to serialize and deserialize classes which do not have a `readObject`/`writeObject` method or which use `ObjectInputStream.defaultReadObject`/`ObjectOutputStream.defaultWriteObject` to supplement default serialization.
>> 
>> It is also proposed to add methods which allow for the reading of serialization-specific private static final fields from classes which have them.
>> 
>> With the addition of these methods, serialization libraries no longer need to rely on `Unsafe` for serialization/deserialization activities.
>> cc: @AlanBateman
>
> David M. Lloyd has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Address review comment

src/java.base/share/classes/java/io/ObjectStreamDefaultSupport.java line 39:

> 37:  *
> 38:  */
> 39: final class ObjectStreamDefaultSupport {

The class and method names are getting a bit unwieldy including "DefaultSupport".
How about "reflection" instead of "DefaultSupport";
i.e.  ObjectStreamReflection.java?

src/java.base/share/classes/java/io/ObjectStreamDefaultSupport.java line 55:

> 53:             throw new NoSuchMethodError(e.getMessage());
> 54:         } catch (IllegalAccessException e) {
> 55:             throw new IllegalAccessError(e.getMessage());

InternalError might be more obviously a "can't happen" case.

src/java.base/share/classes/java/io/ObjectStreamDefaultSupport.java line 59:

> 57:     }
> 58: 
> 59:     private static void defaultReadObject(ObjectStreamClass streamClass, Object obj, ObjectInputStream ois) throws IOException, ClassNotFoundException {

For future maintainers, it would be helpful to describe what's going here.
In particular, note that `ObjectInputStream.readFields` may have been overridden by a subclass of OIS to provide a set of field values from a different source or stream encoding.
As well as a description of pulling the field values out of the alternate GetFields and stuffing them into arrays that are then used to set the fields of the object.

src/java.base/share/classes/java/io/ObjectStreamDefaultSupport.java line 78:

> 76:                 default -> throw new IllegalStateException();
> 77:             }
> 78:         }

Before setting any of the object fields check types of objects.
`streamClass.checkObjFieldValueTypes(obj, objs)`
It avoids partially initialized objects.

src/java.base/share/classes/java/io/ObjectStreamDefaultSupport.java line 83:

> 81:     }
> 82: 
> 83:     private static void defaultWriteObject(ObjectStreamClass streamClass, Object obj, ObjectOutputStream oos) throws IOException {

Ditto here, describing a subclasses OOS can override putFields() to return a PutField instance that extracts the field values from the object and copies them into the PutField instance making the value available to be put into the stream in a custom format.

src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 470:

> 468:                 return null;
> 469:             }
> 470:             field.setAccessible(true);

setAccessible() might need a doPriv to be successful in all cases.

src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 491:

> 489:             return 0;
> 490:         }
> 491:     }

How is this different than `ObjectStreamClass.lookup(clazz).getSerialVersionUID()` ?

And BTW, this misses the computation of the default SVUID and the SVUID of Records.

src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 505:

> 503:     );
> 504: 
> 505:     private static boolean isValidSerializable(Class<?> cl) {

"Valid" is too broad to clearly identify the purpose. 
You might flip it to return true for all classes that use the default serialized object format.

You might find that using the pattern language feature and "instanceof" would be effective and not need the small set.

test/jdk/sun/reflect/ReflectionFactory/ReflectionFactoryTest.java line 379:

> 377:         @SuppressWarnings("removal")
> 378:         ObjectOutputStream oos = AccessController.doPrivileged((PrivilegedExceptionAction<ObjectOutputStream>) () -> new ObjectOutputStream() {
> 379:             protected void writeObjectOverride(final Object obj) throws IOException {

The alternate invocation of the replacement defaultWriteObject should be hooked in here, to see how the whole mechanism works smoothly. Especially for nested serializations.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1769226371
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1767069044
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1769194000
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1767593821
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1769197407
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1769217751
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1679849130
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1679899170
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1714206229