(15) RFR: JDK-8247444: Trust final fields in records

[Mandy Chung]

On 6/17/20 8:11 AM, Aleksey Shipilev wrote:
> On 6/15/20 11:58 PM, Mandy Chung wrote:
>> This patch also proposes `sun.misc.Unsafe::objectFieldOffset` and
>> `sun.misc.Unsafe::staticField{Offset/Base}` not to support records.
> Note this would break otherwise innocuous introspection for records, for example dumping the VM
> layout with JOL:
>    http://openjdk.java.net/projects/code-tools/jol/

To me, JOL needs a supported API in the serviceability area to 
introspect a field layout.  JOL will not work for inline types without 
adding new APIs.

sun.misc.Unsafe is an unsupported API.   My proposal for sun.misc.Unsafe 
(in fact jdk.unsupported module) is to keep the support for legacy use 
but not necessarily for new features.

>
> Amusingly, with the rest of Unsafe available the barn is still full open. Here's the sketch for the
> workaround: get the field list with getDeclaredFields, optionally guess the VM layout (the rules are
> not that hard, and JOL does it routinely), instantiate a Record with whatever arguments, poke the
> test patterns with Unsafe.put*, read them back from record components to verify. It would get me the
> same objectFieldOffset in a round-about way.

I'm aware of these workarounds in the wild.   As sun.misc.Unsafe is JDK 
unsupported API, this patch does not attempt to implement a complete 
solution but adds an obvious notification informing the frameworks that 
`sun.misc.Unsafe::objectFieldOffset` and 
`sun.misc.Unsafe::staticField{Offset/Base}` not to support records.

Unsafe is unsafe!   Use it at your own risk.

> And speaking from JOL maintainer standpoint, that one would be very tempting to do, because it would
> not depend on whatever protection shenanigans a particular JDK release tries to enforce.
>
>> This change impacts 3rd-party frameworks including 3rd-party
>> serialization framework that rely on core reflection `setAccessible` or
>> `sun.misc.Unsafe::allocateInstance` and `objectFieldOffset` etc to
>> construct records but not using the canonical constructor.
>> These frameworks would need to be updated to construct records via its
>> canonical constructor as done by the Java serialization.
> Are we absolutely sure that what ObjectInputStream.readRecord() does fits the 3rd party
> serialization libraries? Does it work for them performance-wise?

There should be performance improvement opportunity (see Peter Levart's 
good work - JDK-8247532 [1])

> (Are we even sure about that for JDK itself?)

Java serialization support for records use its canonical constructor.  
(I'm not sure what you tried to point out by this).

>   Because if it is not, 3rd party lib maintainers would proceed to hacking in the
> "objectFieldOffset workaround" and we would get the cobra-effect-like strengthening of dependency on
> Unsafe quirks.

This is exactly why we request this for 15 so that 3rd-party lib 
maintainers can prototype and send feedback.  We love to understand any 
stumbling block and work together to resolve any issue before records 
exit preview.

>
>> No change is made in JNI.  JNI could be considered to disallow
>> modification of final fields in records and hidden classes (static and
>> instance fields).  But JNI has superpower and the current spec already
>> allows to modify the value of a final static field even after it's
>> constant folded (via JNI Set<type>Field and SetStatic<type>Field), then
>> all bets are off.
> It is fun to consider JNI to be more powerful than Unsafe. This seems backwards. The intent to break
> Unsafe users might be defensible, but this power oddity is still quite odd.
>

I think you misread the message.  There is no claim whether JNI or 
Unsafe is more powerful.

JNI is a supported API.   The above explains why I propose no spec 
change to make in JNI.  OTOH jdk.unsupported is unsupported but has more 
love by frameworks to avoid writing in native.  Adding a check in some 
sun.misc.Unsafe APIs (even it can be hacked to workaround it) is a clear 
notification what it does not intend to support.

Hope this helps.
Mandy
[1] 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067223.html