[nestmates] validation of ClassFile for hidden classes

[David Holmes]

Hi Mandy,

 > Does VM warning on by default?

Yes.

Thanks,
David

On 13/11/2019 4:53 pm, Mandy Chung wrote:
> 
> 
> On 11/12/19 9:45 PM, David Holmes wrote:
>> Hi Mandy,
>>
>> On 13/11/2019 10:45 am, Mandy Chung wrote:
>>> A recent review with Alex and John on [1][2] specifically on:
>>> - Hidden classes cannot participate in static nest membership as
>>>    they do not have names. NestHost/NestMembers attributes on hidden
>>>    classes are ignored.
>>>
>>> Instead of making validation of ClassFile for hidden classes and 
>>> ordinary
>>> classes somewhat different, the alternative is to have the validation
>>> for hidden classes and ordinary classes identical.  When another class
>>> (whether ordinary or hidden) names a hidden class as its declaring class
>>> or class member (via |InnerClasses| attribute), or as its nest host
>>> (via |NestHost| attribute), or as a member of its nest
>>> (via |NestMembers| attribute), the class may successfully be created
>>> while it may fail at run time where there is any attempt to resolve
>>> a hidden class by name via any of these attributes per existing
>>> specification.  No special rule for validation of hidden classes.
>>
>> The problem is not dealing with these attributes naming hidden classes 
>> as you can't name a hidden class in the first place; the problem is 
>> having these attributes in hidden classes themselves.
> 
> That's true.  I attempt to generalize (probably unsuccessfully) the 
> problem that can occur in a class named in these attributes that cannot 
> be found (where this class can be a hidden class or an ordinary class 
> that cannot be found).   There is no good benefit to place certain 
> restrictions on hidden classes.
> 
>>
>> You can certainly allow the hidden class to have all these attributes 
>> and reject their use at runtime (as and when that may happen). I don't 
>> really see the benefit in doing so however as in some cases at least 
>> the specification is going to have to account for this anyway e.g 
>> ignoring static nest attributes when dynamically injecting into a nest.
>>
>>> David,
>>>
>>> This is the revised webrev:
>>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/8171335/webrev-hidden-class-validation/ 
>>>
>>>
>>> If a hidden nestmate class has a `NestHost` or `NestMembers` attribute,
>>> and it's created to add to a member of a dynamic nest host (H), then
>>> the runtime nest host is set to `H` and these attributes will not
>>> be validated subsequently.
>>
>> Okay.
>>
>>>
>>> If a hidden class has a `NestHost` or `NestMembers` attribute and
>>> created with Lookup::defineHiddenClass without NESTMATE option,
>>> then the behavior is the same as an ordinary class with a bad
>>> `NestHost` or `NestMembers` attribute (e.g. HC's nest host is
>>> not yet validated.  When it attempts to access NH's private member,
>>> it would fail to resolve NH.)
>>
>> Okay.
>>
>>> I'm not sure if this is the best way to implement
>>> InstanceKlass::set_nest_host.  When there is a proposed JVMS draft,
>>> we may make more adjustment to the implementation.  So I hope
>>> this patch is good to push in the time being.
>>
>> I have a number of minor comments, but nothing to stop pushing of the 
>> patch.
>>
>>  364 // So we know that current class is "pristine" and its _nest_host 
>> must be NULL.
>>
>> It's no longer really "pristine" if it has a NestHost or NestMember 
>> attribute. I'd probably just add an additional comment:
>>
>> // If it has an explicit _nest_host_index or _nest_members, these will 
>> be ignored.
>>
> 
> Updated.
> 
>> This change:
>>
>> -  assert((host->_nest_host == NULL && host->_nest_host_index == 0) ||
>> -         (host->_nest_host == host), "proposed host is not a valid 
>> nest-host");
>> +  assert(host->_nest_host == NULL || host->_nest_host == host, 
>> "proposed host is not a valid nest-host");
>>
>> is not correct. This is asserting something about the klass we're 
>> going to be use as the hidden class's nest-host, not something about 
>> the hidden class. The proposed nest-host must be a valid nest-host.
>>
> 
> Thanks for catching it.  This is reverted.   I mistakenly read it the 
> other way.
> 
>> + log_trace(class, nestmates)("Type %s is a dynamic nest member of %s: 
>> the current class has a NestHost attribute",
>>
>> I'd be more inclined to make these warnings rather than just logging 
>> them. Either way it should state the static values are ignored.
>>
> 
> I leave it as logging for now.  We can do this in a future clean up. 
> Does VM warning on by default?
> 
>>> This patch also includes a small javadoc change.
>>
>> +     * then the name is given by the JVM of the following format:
>>
>> suggest:
>>
>> +     * then the name is defined by the JVM with the following format:
>>
>> similarly in the MethodHandle docs.
>>
>> -- 
>>
>> +     * NESTMATE} option, then the hidden class was added as a member to
>> +     * the nest of a {@linkplain MethodHandles.Lookup#lookupClass() 
>> lookup class}
>>
>> suggest:
>>
>> +     * NESTMATE} option, then the hidden class is added to
>> +     * the nest of the {@linkplain MethodHandles.Lookup#lookupClass() 
>> lookup class}
>>
>> -- 
>>
>> -     * Lookup::defineHiddenClass}.
>> +     * Lookup::defineHiddenClass} dynamically.
>>
>> Adding "dynamically" is not necessary here - the use of 
>> defineHiddenClass is implicitly "dynamic".
>>
> 
> OK.  Fixed.
> 
> thanks
> Mandy