[nestmates] validation of ClassFile for hidden classes

[Mandy Chung]

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