[nestmates] validation of ClassFile for hidden classes

[David Holmes]

Correction/clarification ...

On 13/11/2019 3: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.
> 
> 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.

I see from discussion on the experts list that there is move to not 
reject a classfile based on the semantics of attributes, so not 
ignoring/rejecting during classfile parsing is okay. But the 
specification will still have to address the interaction between static 
NestHost attribute and the use of dynamic nestmate injection.

Thanks,
David
-----

>> 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.
> 
> 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.
> 
> +      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.
> 
>> 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".
> 
> Thanks,
> David
> -----
> 
> 
>> Mandy
>> [1] http://hg.openjdk.java.net/valhalla/valhalla/rev/ad257875959a
>> [2] 
>> https://mail.openjdk.java.net/pipermail/valhalla-dev/2019-September/006420.html 
>>