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

[David Holmes]

On 13/03/2018 8:13 AM, John Rose wrote:
> On Mar 12, 2018, at 12:05 AM, David Holmes <david.holmes at oracle.com 
> <mailto: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/
> 
> I reviewed this and I like it.

Thanks.

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

If members[0] != this, then it is guaranteed that members.length > 1, as 
it must contain at a minimum the current class as a member, and the 
actual nesthost in the [0] element. That's what the VM code is intended 
to guarantee. If that doesn't hold then something is badly broken and we 
don't know what we're actually returning. So I don't see how this 
"sanity check" helps anything. ?? Plus it leads to confusion for anyone 
later reading the code.

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

Glad it is okay.

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

Thanks. I'll file a separate cleanup issue and copy your comments there 
and look at it in more detail.

Thanks,
David


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