[Nestmates] RFR: 8199309: [Nestmates] The new Class nestmate methods should have SecurityManager checks

[John Rose]

On Mar 12, 2018, at 12:05 AM, David Holmes <david.holmes at oracle.com> wrote:
> 
> So, let me exclude the cases of "what comes out is what went in"
> 
> http://cr.openjdk.java.net/~dholmes/8199309/webrev.v2/ <http://cr.openjdk.java.net/~dholmes/8199309/webrev.v2/>

I reviewed this and I like it.

The bit about "members.length > 1" bothers me slightly.
I know what the spec implies, but I went and read the JVM code
that enforces it and wondered if the invariant should be checked
here also, in case something gets out of line somewhere else.

Suggest:
   if (members.length > 1 || members[0] != this)

The cost wont't be noticeable, since the code just called a
function from jvm.cpp.

I agree with the language below:

> But that does leave a slight wording issue with the @throws SecurityException. The way it reads now, how ever getNestHost()/getNestMembers() behaves you should be able to get the same result with your own package access check. This isn't in the webrev but how about:
> 
> * @throws SecurityException
> *         If the returned class is not the current class, and
> *         if a security manager, <i>s</i>, is present and the caller's
> *         class loader is not the same as or an ancestor of the class
> *         loader for the returned class and invocation of {@link
> *         SecurityManager#checkPackageAccess s.checkPackageAccess()}
> *         denies access to the package of the current class
> 
> and similarly for getNestMembers:
> 
> * @throws SecurityException
> *         If any returned class is not the current class, and
> *         if a security manager, <i>s</i>, is present and the caller's
> *         class loader is not the same as or an ancestor of the class
> *         loader for the returned class and invocation of {@link
> *         SecurityManager#checkPackageAccess s.checkPackageAccess()}
> *         denies access to the package of the current class
> 
> It's a bit of a mouthful but ...

It's just right.  Thanks for refactoring this way.

A few more comments, on the code already committed, FWIW.

There's a typo "accesss" in InstanceKlass::has_nestmate_access_to.

The loop in InstanceKlass::has_nest_member should exit after the
first encounter where name == k->name(), since there's no more
work to do there.

It occurs to me that this loop (looking for name == k->name()) could
be improved slightly by first inspecting the resolution state of
_constants->klass_name_at(cp_index).  If it's already resolved,
then there's no need to mess with names; a pointer comparison
will finish the job.  That's probably a common occurrence, since
nest member checks tend to happen after classes are resolved.

I read all the nestmate query code in jvm.cpp and instanceKlass.cpp,
found it read well.  I had one point of confusion, though, which makes
me suggest a tweak next time you touch that code:  The query
ik->nest_members() looks parallel to ik->nest_host(), but n_m is
just unvalidated indexes and n_h is validated.  I suppose the
thing could be named nest_member_indexes, but that seems like
overkill.  But a comment in instanceKlass.hpp on nest_members
and nest_host_index might say clearly that the data in those
slots are not validated.

— John