[nestmates] validation of ClassFile for hidden classes

[David Holmes]

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.

> 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 
>