Hi Peter,
Thanks for looking at this. I'm glad I didn't try to respond last night
before your follow up emails came through :)
On 22/05/2018 8:36 PM, Peter Levart wrote:
Hi David,
On 05/15/2018 02:52 AM, David Holmes wrote:
Master webrev of all changes:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
I skimmed over reflection API changes.
In jl.Class:
3911 // returning a different class requires a security check
3912 SecurityManager sm = System.getSecurityManager();
3913 if (sm != null) {
3914 checkPackageAccess(sm,
3915 ClassLoader.getClassLoader(Reflection.getCallerClass()), true);
3916 }
...so here the "different" class is expected to be in the same package
as "this" class. Is this invariant enforced in VM so it need not be
checked here?
Yes. All nestmates have to be in the same runtime package, and this is
enforced in the VM. As Mandy explained this covers the case where you
manage to get hold of an implementation object (potentially from deep
inside some other package - perhaps module?) and call getClass() on it
(which is allowed) and then try to examine it's nest. If the
nest-host/member being returned is not the current class, then you
should perform the SM check. (As you already have the current class it
doesn't matter if it is returned.)
3871 * @apiNote The source language compiler is responsible for
deciding which classes
3872 * and interfaces are nestmates, by generating the appropriate
attributes
3873 * (JVMS 4.7.28 and 4.7.29) in the class file format (JVMS 4).
3874 * For example, the {@code javac} compiler
3875 * places a top-level class or interface into a nest with all
of its direct,
3876 * and indirect, {@linkplain #getDeclaredClasses() nested
classes and interfaces}
3877 * (JLS 8).
3878 * The top-level {@linkplain #getEnclosingClass() enclosing
class or interface}
3879 * is designated as the nest host.
Should the text warn about not relying on this implementation detail to
extract knowledge about nested/enclosing classes? Users should keep
using getDeclaredClasses()/getEnclosingClass() for that purpose as
nestmates may in future be used for other things too. OTOH, if users
want to do an equivalent of private access check (on behalf of real
caller and callee - for example in some kind of language runtime), it
would be better they use the nestmate API...
I think the fact this is an apiNote that explicitly calls out how the
javac compiler maps from the language concept of nested types to the VM
concept of nests, should be enough to guide the programmer here. As you
note there is no guarantee that the nest-host is the top-level class.
But beyond that the existing API's work quite differently in terms of
exploring the actual structure of nested types, whereas the nestmate API
is a "flat world".
It is possible (likely) that in the future VM nests will be used for
additional things - not all of which need be observable via a Java
specific API. That will be determined later.
For a language runtime that wants to know before hand whether a direct
private access will work, then yes isNestmate() is what they would use.
in j.l.r.AccessibleMember:
49 * <p> Java language access control prevents use of private
members *outside*
50 * *their top-level class;* package access members outside their
package; protected members
51 * outside their package or subclasses; and public members outside
their
52 * module unless they are declared in an {@link
Module#isExported(String,Module)
53 * exported} package and the user {@link Module#canRead reads}
their module.
Could this be interpreted also as "private members can only be accessed
from the top-level class"? I know that "nest" is not a Java language
term, so it can not be used in the context of Java language access
control. (If speaking of Java language, then even previous version of
this text was wrong - if it was right, then it wouldn't have to be
changed at all). So what about the following:
"Java language access control prevents use of private members outside
the set of classes composed of top-level class and its transitive
closure of nested classes".
Or would this be to lengthy and hard to understand?
Yes. :)
This was a minor correction to what was, as you noted, already an
inaccurate statement:
"Java language access control prevents use of private members outside
their class."
That's obviously not true. As per JLS 6.6.1:
"Otherwise, the member or constructor is declared private, and access is
permitted if and only if it occurs within the body of the top level type
(§7.6) that encloses the declaration of the member or constructor."
So "class" was changed to "top-level class" to match the JLS.
in j.l.r.Reflection:
140 // At this point we know that currentClass can access
memberClass.
141
142 if (Modifier.isPublic(modifiers)) {
143 return true;
144 }
145
146 // Check for nestmate access if member is private
147 if (Modifier.isPrivate(modifiers)) {
148 // assert: isSubclassof(targetClass, memberClass)
although just in a function of explaining the following comment, I think
the correct assert is
// assert targetClass == null || isSubclassof(targetClass, memberClass)
as for static members, targetClass is null.
Yes. I think I'll just delete the psuedo-assert though because I think
it's only true if the nestmate access is true - plus it was more a
mental note to myself when working on this.
149 // Note: targetClass may be outside the nest, but that
is okay
150 // as long as memberClass is in the nest.
151 boolean nestmates = areNestMates(currentClass, memberClass);
152 if (nestmates) {
153 return true;
154 }
155 }
I'm wondering about the frequency of reflective accesses of various
kinds of members. I imagine that most frequent are accesses of public
members, others are less so (I'm not counting accesses made via
.setAccessible(true) modified Field/Method/Constructor objects as they
are bypassing this Reflection.verifyMemberAccess method). But among
others I imagine reflective access to private members is the least
frequent as there's no practical reason to use reflection inside and
among classes that are most intimately logically coupled and "know" each
other's internals at compile time. So it would make sense to check for
private access at the end, after protected and package-private checks
(that's how existing logic was structured).
So how about putting the nestmate access logic just before the last if
(!successSoFar) { return false; }:
// Check for nestmate access if member is private
if (!successSoFar && Modifier.isPrivate(modifiers)) {
// assert: targetClass == null || isSubclassof(targetClass,
memberClass)
// Note: targetClass may be outside the nest, but that is okay
// as long as memberClass is in the nest.
successSoFar = areNestMates(currentClass, memberClass);
}
if (!successSoFar) {
return false;
}
This would not penalize access to package-private and protected members
with areNestMates() JNI calls and maybe caching will not be needed at all.
As per your later email the check is guarded by the isPrivate check so
we avoid the JNI call. Also I deliberately placed the check where it is
(in contrast to VM code where it is almost last) to avoid the whole
"successSoFar" mess. :) My view was that if this indeed turned out to
impact performance then we could revisit the placement of it.
Thanks,
David
Regards, Peter
