RFR: 8257596: Clarify trusted final fields for record classes

[Mandy Chung]

On Thu, 10 Dec 2020 14:13:27 GMT, Chris Hegarty <chegar at openjdk.org> wrote:

>> This is a follow-up on JDK-8255342 that removes non-specified JVM checks on classes with `RecordComponents` attributes.  That introduces a regression in `InstanceKlass::is_record` that returns true on a non-record class which has `RecordComponents` attribute present.   This causes unexpected semantics in `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust final fields for non-record classes.
>> 
>> I propose to change `InstanceKlass::is_record` to match the JLS semantic of a record class, i.e. final direct subclass of `java.lang.Record` with the presence of `RecordComponents` attribute.  There is no change to JVM class file validation.   Also I propose clearly define:
>>     - `JVM_IsRecord` returns true if the given class is a record i.e. final and direct subclass of `java.lang.Record` with `RecordComponents` attribute
>>     - `JVM_GetRecordComponents` returns an `RecordComponents` array  if `RecordComponents` attribute is present; otherwise, returns NULL.  This does not check if it's a record class or not.  So it may return non-null on a non-record class if it has `RecordComponents` attribute.  So `JVM_GetRecordComponents` returns the content of the classfile.
>
> Marked as reviewed by chegar (Reviewer).

Hi Remi,

> For me, it's backout JDK-8247444, as i said on the amber-spec-expert, i prefer VM to be oblivious about java.lang.Record.
> And in the future, the real fix is to change the spec of Field.set() to say that it's only allowed for plain java classes and have the implementation to directly asks the VM is a non static field is trusted or not.

This reply was before you realized that records are are permanent Java SE 16 feature :-)  If the future ended up requiring/desiring to allow final fields of a record class be modifiable reflectively (I double that!), `Field::set` spec can be updated to remove that restriction with no compatibility risk

> And there are also a related issue with the validation of the InnerClass/Enclosing attribute were the VM does a handshake between the inner class attribute and the enclosing class attribute when calling Class.getSimpleName(), again using the JLS definition of what an inner class is instead of using the VM definition, the content of the InnerClass attribute with no validation.

It's the implementation details of the core reflection how to determine if a class is a member of another class.   The `getSimpleName` spec and other related APIs (see JDK-8250226) need to define the behavior when there is an issue in determining the declaring class.

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

PR: https://git.openjdk.java.net/jdk/pull/1706