On Thu, 13 Jun 2024 14:31:06 GMT, David M. Lloyd <d...@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
