Nest host validation vs NestHost attribute performed by Lookup::defineHiddenClass

[Mandy Chung]

On 9/24/19 7:32 PM, John Rose wrote:
> I agree completely.
>
> You are right to note that depending on the dynamic nest implies that validation of static information must be done during the dHC call if it has not yet been done. (That’s worth another bullet point!)

+1
>
> A further idea: A broken NH or NMs element is ignored, with fallback to pretending that the attribute was not present. Let’s use this rule for hidden classes as well:  if a HC has a NH or NMs attribute then we should simply ignore it.

Hidden classes do not have names and they cannot participate in NH/NM 
attributes as you noted (2a) in your previous reply.  Why not 
disallowing defining a hidden class with NH/NM attributes as in the 
current proposal [1]?

Mandy
[1] 
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/8171335/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#defineHiddenClass(byte%5B%5D,boolean,java.lang.invoke.MethodHandles.Lookup.ClassOption...)

>> On Sep 24, 2019, at 6:15 PM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Hi Mandy,
>>
>> I think we need to consider the distinction between the runtime nest_host and the statically defined NestHost attribute. If we did eager validation and rejected bad NestHost attributes then the two would always be the same. But for pragmatic reasons we don't do that and only establish the runtime nest_host when an actual access check is needed. If the NestHost H validates then we have H as the runtime nest_host and there's no problem. If the NestHost doesn't validate then the runtime nest_host is logically the class C itself: it implicitly forms a singleton nest.
>>
>> Lookup.defineHiddenClass, when trying to inject into a nest, must use the actual runtime nest_host of the lookup class C. This is clear to me as the intent is to give the injected class the same access rights to a nest as C has. So defineHiddenClass needs to trigger nest host validation (ignoring exceptions), if it hasn't been performed yet, and use the resulting runtime nest_host: either H or C depending on the validation result.
>>
>> That's my 2c.
>>
>> David
>>
>>> On 25/09/2019 10:27 am, Mandy Chung wrote:
>>> This is regarding an incompatibility concern when LambdaMetaFactory and any other framework library converts the use of `Unsafe::defineAnonymousClass` to `Lookup::defineHiddenClass` when a caller caller `C` whose nest host `H` indicated by `NestHost` attribute is "bad".
>>> `NestHost` and `NestMembers` attributes are consulted by JVM access control (JVMS 5.4.4).  If `H` cannot be loaded, or is not in the same run-time package as `C`, or does not authorize nest membership (`H` has a `NestMembers attribute but there is no entry named `C`), then a linkage error occurs.
>>> OTOH, `Class::getNestHost` may return a Class that is not the named class indicated by `NestHost` attribute.  Specifically:
>>>     If there is a linkage error accessing the nest host, or if this class
>>>     or interface is not enumerated as a member of the nest by the nest host,
>>>     then it is considered to belong to its own nest and this is returned
>>>     as the host.
>>> If `C` has a bad nest host indicated by `NestHost` attribute, then effectively `NestHost` attribute is ignored and `C` is the host of its own nest as if `C` lacked of `NestHost` attribute.   No error occurs until `C` attempts to access a private member of `H` - consistency for VM and core reflection.
>>> Now `Lookup::defineHiddenClass` can be used to add a hidden class to a nest of the lookup class.
>>> If a framework uses C's full-power lookup object to call `Lookup::defineHiddenClass` to add a hidden class `X` into C's nest. What is the nest host of `X`?
>>> If C has a good nest host, of course X will have the same nest host as C.  If C has a bad nest host `H`, the lookup object on C is used to call `Lookup::defineHiddenClass` - should it fail with illegal nest host? Should it succeed to be consistent with core reflection (asC.class::getNestHost returns C - see more)?
>>> That is the current proposed behavior of `Lookup::defineHiddenClass` to validate that the lookup class must be the host of a nest.  If the lookup class has `NestHost` attribute, it is a member of some other class's nest and so fails.  The design principle is that the lookup class and the proper permission together authorizes the lookup object to add a new member into a nest.  In addition, C's nest host is H and adding a class with C as the nest host should be disallowed as this conceptually creates a "nested nest" (H is C's nest host, C is X's nest host).
>>> A framework library can't tell whether C is the class indicated by C's NestHost attribute. Take an example in practice.  C uses lambda expression or method reference expression but H cannot be found while C never accesses H's private member (If C accesses H's private member, then IAE must occur since it fails the access check). The first invocation of indy LMF for C class unfortunately throws BootstrapMethodError because LambdaMetaFactory calls`Lookup::defineHiddenClass` to define a lambda proxy class for C and IAE occurs as C is a member of `H` statically.
>>> It is expected that this error case should rarely occur but it does have an incompatibility risk that impacts existing applications.
>>> As Class::getNestHost returns C as the nest host, a framework library calls `defineHiddenClass` with the returned nest host but surprisely it throws an exception that nothing it can do.
>>> If C has a bad nest host `H`, it will not get an error until C attempts to access a private member of H.  I think we should revisit this validation and relax `defineHiddenClass` validation to check that the lookup class is the same class as Class::getNestHost returns.  The hidden class X has C as the nest host (but NOT H).  The VM access check will ensure that X will fail when accessing H's private members.  No nested nest is created since at runtime C is the host of its own nest and X is added to C's nest (where the static nestmate relationship is not in effect).
>>> Summary:
>>> `Lookup::defineHiddenClass` performs the nest host validation but won't consultthe `NestHost` attribute of the lookup class.
>>> Feedback?
>>> thanks
>>> Mandy