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

Fri Jul 5 14:46:47 UTC 2024

On Mon, 1 Jul 2024 05:35:37 GMT, Chen Liang <liach 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
>
> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 441:
> 
>> 439:         }
>> 440: 
>> 441:         return SerializationBytecodeGenerator.defaultReadObjectForSerialization(cl);
> 
> How likely is it for multiple libraries to request readObject for the same class?
> 
> If it happens that many clients request factories for the same class, we can consider computing and leaving the MHs in a `ClassValue` instead (so the MHs can be GC'd when `cl` is unreachable, but are otherwise cached and shared)

My hope is that the method handle would be more easily inlined when each one is a separate constant. I'd feel less confident that this would be the case if I indirected through `List`. I think it would also increase the size of the generated method as well, though I'm not sure if there actually is any practical consequence to this.

Also, this was easier. ;-)

> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 464:
> 
>> 462:             }
>> 463:             field.setAccessible(true);
>> 464:             return (ObjectStreamField[]) field.get(null);
> 
> Should we clone the array to prevent user modification?

Sure, that's a good idea.

> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 488:
> 
>> 486:     }
>> 487: 
>> 488:     private static boolean isValidSerializable(Class<?> cl) {
> 
> Should we filter `Class`, `ObjectStreamClass`, and `String` (https://docs.oracle.com/en/java/javase/22/docs/specs/serialization/serial-arch.html#writing-to-an-object-stream) too?

Hmm, good idea.

> src/java.base/share/classes/jdk/internal/reflect/SerializationBytecodeGenerator.java line 53:
> 
>> 51: 
>> 52:     static MethodHandle defaultReadObjectForSerialization(Class<?> cl) {
>> 53:         // build an anonymous+hidden nestmate to perform the read operation
> 
> Maybe note that we generate erased types and use MH setters/getters because the generated bytecode is defined under the loader of SerializationBytecodeGenerator, which cannot access `cl`. Otherwise, the first thing coming to my mind is that we can just use direct field access and explicit `cl` type in the bytecode, or we are supporting hidden classes for `cl` so we have to stay erased (which is not the case here)

I added a more detailed explanation.

> src/java.base/share/classes/jdk/internal/reflect/SerializationBytecodeGenerator.java line 88:
> 
>> 86:                     }
>> 87:                     cb.ldc(DynamicConstantDesc.ofNamed(
>> 88:                         ConstantDescs.BSM_CLASS_DATA_AT,
> 
> One side remark: I think `classDataAt` is more useful for bootstrap method arguments, like `nullConstant`; in actual bytecode, `classData` + `List.get` invokeinterface should be more constant-pool friendly, as you just need one CP entry for the whole list as opposed to one CP entry for each list item.
> 
> But either way, this is correct, and we only care about correctness in legacy support code.

I accidentally replied in the wrong thread here: https://github.com/openjdk/jdk/pull/19702#discussion_r1661000439

> src/java.base/share/classes/jdk/internal/reflect/SerializationBytecodeGenerator.java line 100:
> 
>> 98:                     ClassDesc fieldDesc = fieldType.describeConstable().orElseThrow(InternalError::new);
>> 99: 
>> 100:                     switch (fieldDesc.descriptorString()) {
> 
> If you just switch over primitives + reference, consider something like `switch (TypeKind.from(fieldType)) {` for simplicity.

Ah, nice idea. This way we can be sure no case was missed.

> test/jdk/sun/reflect/ReflectionFactory/ReflectionFactoryTest.java line 732:
> 
>> 730:         final String final_str;
>> 731: 
>> 732:         Ser2(final byte final_byte, final short final_short, final char final_char, final int final_int,
> 
> I wonder if you can add a serializable user-defined class in the test; `String` is a reference but it's a primitive data type in serialization specification.

Sure. The test subclasses the `Object*Stream` classes so the special type handling never comes into play, but we can be clearer about it by using a different type.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1661000439
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1660971581
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1661000877
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1661014468
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1661020564
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1660996224
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1661006149