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

Fri Jul 5 14:46:46 UTC 2024

On Thu, 13 Jun 2024 14:31:06 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

Oops, I forgot that `cc` is a Skara command. Sorry, bot.

I've updated the PR with a new access control solution, but it's not ideal since it involves internal reflection. To solve this issue, unless I'm misunderstanding something, I think we would need to have access to `Lookup#IMPL_LOOKUP` from the method generators. The only way I can think to do this (without exposing the lookup from `JavaLangInvokeAccess`, which I assume would not be desirable) would be to move the generators into the `java.lang.invoke` package, adding methods to `JavaLangInvokeAccess` for that purpose. Would this be the recommended approach, or is there some other better approach that I do not see?

I added a commit which moves the bytecode gen to the JLI package and accesses it via SharedSecrets, which is reasonably similar to what other methods in ReflectionFactory are already doing.

Edit: Per Alan's feedback this was moved back again. See [the resolved discussion](https://github.com/openjdk/jdk/pull/19702#discussion_r1658837413) for details.

> I skimmed through the latest update ([d137f43](https://github.com/openjdk/jdk/commit/d137f43b00e935e9fbcaeaa80fdff93a0a2df44b)) and I think you've got this to a good place and a sensible/workable proposal. I've asked from help from others on the security review of this.

Great. There are a couple more tests I want to add but once that's done I'll mark it "Ready for review".

> Right now, I'm still wonder if defaultReadObjectForSerialization/defaultWriteObjectForSerialization should return the same as readObjectForSerialization/writeObjectForSerialization when the readObject/writeObject methods are defined. This is more of a concern for a class with a readObject of course as that readObject will likely check invariants that would be bypassed if the serialization library always uses the defaultXXX methods.

By spec, the `readObject` method must call either `OIS#defaultReadObject` or `OIS#readFields`, but they _are_ allowed to choose which one they call. If they call `defaultReadObject`, then the serialization library would still need `defaultReadObjectForSerialization` in order to implement it. If `defaultReadObjectForSerialization` falls back to calling the user `readObject`, we then would (at best) have a stack overflow situation, because the user's `readObject` would call `defaultReadObject`, which in turn would call back into the user's `readObject`, etc.

So, in some cases, the (de)serializer can use only `readObjectForSerialization` (specifically, when there is a user `readObject` method which uses `readFields`); in other cases it can use only `defaultReadObjectForSerialization` (when the user didn't provide a `readObject`); and, in the final case, it would need to use both (when the user `readObject` calls `OIS#defaultReadObject`).

The same thing applies on the write side.

> I'd probably drop the "Of" suffix from serialPersistentFieldsOf and serialVersionUIDOf but naming isn't important right now.

I'll do so.

> /csr
> 
> Since this is an API addition, this will require a CSR; Valhalla folks can check if this API is future-proof in the CSR.

According to https://wiki.openjdk.org/display/csr/CSR+FAQs, only changes to public and exported APIs in the `jdk.*`, `java.*`, and `javax.*` packages need a CSR. This PR changes public and exported APIs in the `sun.misc` package, and changes public but non-exported APIs in the `jdk.internal.reflect` package, so I don't think it qualifies, does it?

The CSR is [JDK-8335438](https://bugs.openjdk.org/browse/JDK-8335438).

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

PR Comment: https://git.openjdk.org/jdk/pull/19702#issuecomment-2165859586
PR Comment: https://git.openjdk.org/jdk/pull/19702#issuecomment-2195182295
PR Comment: https://git.openjdk.org/jdk/pull/19702#issuecomment-2195575503
PR Comment: https://git.openjdk.org/jdk/pull/19702#issuecomment-2200005883
PR Comment: https://git.openjdk.org/jdk/pull/19702#issuecomment-2200116881
PR Comment: https://git.openjdk.org/jdk/pull/19702#issuecomment-2200604899