RFR: 8022718 : Runtime accessibility checking: protected class, if extended, should be accessible from another package

[John Rose]

On Oct 9, 2013, at 11:49 AM, David Chase <david.r.chase at oracle.com> wrote:

> bug: https://bugs.openjdk.java.net/browse/JDK-8022718
> 
> webrev: http://cr.openjdk.java.net/~drchase/8022718/webrev.00/
> 
> Problem: Needed implementation for change to the spec for JVM behavior (from the bug report):
> 
>> JSR 335 spec, chapter "15.28.2 Run-time Evaluation of Method References" contains following assertion:
>> 
>> If the method reference has the form ExpressionName :: NonWildTypeArgumentsopt Identifier or Primary :: NonWildTypeArgumentsopt Identifier, the body of the invocation method has the effect of invoking the compile-time declaration of the method reference, as described in 15.12.4.3, 15.12.4.4, 15.12.4.5.[jsr335-15.28.2-41]
>> 
>> This assertions refers to chapter "15.12.4.3. Check Accessibility of Type and Method" which contains following assertions:
>> 
>> Let C be the class containing the method invocation, and let T be the qualifying type of the method invocation (§13.1), and let m be the name of the method as determined at compile-time (§15.12.3).
>> ...
>> If T is in a different package than C, and T is protected, then T is accessible if and only if C is a subclass of T.
>> ...

JSR 292 doesn't implement the Java language spec; it implements the JVM spec.  In this case there is a difference.

Mixing language and JVM spec. can lead to confusion down the road.

The error is that Class.getModifiers (which is correctly saying "protected") is information about the source code.  The JVM does not look at this for checking access; it looks at the more obscure Reflection.getClassAccessFlags.  Those flags will say "public" (correctly) for a class compiled as "protected".

In order to align exactly with JVM behavior (not Java semantics) the Reflection.getClassAccessFlags need to be looked at.  So I think this fix is wrong.  I think the root cause of this problem was me reaching for Class.getModifiers when I should have grabbed Reflection.getClassAccessFlags.

About BogoLoader:  I would prefer to see it hoisted into a place where more tests can use it.  Consider test/java/lang/invoke/indify/Indify.java as a model; name it test/java/lang/invoke/loader/BogoLoader.java.

— John

> Note that this cannot be expressed in compatibly compiled Java (yet).  Testing requires either monkeying with change and recompilation, or doctoring bytecodes at runtime.
> 
> Fix:
> Turns out that all that was needed was the missing case of checking the conditions for protected access,
> despite suggestions in the bug report that something different might be needed (the different version of the
> access flags was not suitable, and not using them makes for a smaller change anyway).  The new behavior
> needed to be mentioned in the Javadoc, so I did.
> 
> Testing:
> 1) wrote new self-contained test to verify behavior (*)
> 2) jtreg of jdk/lambda java/lang/invoke java/util/stream  (passed)
> 3) running ute on vm.quick.testlist
> 4) running JPRT on testset core (well, trying to run JPRT).
> 
> Issues:
> 
> - It's highly likely I botched the Javadoc changes; I'm sure we have standards, and I'm sure I don't know them.
> 
> - (*) The bug, as reported, describes as "wrong" the behavior obtained if the -PUBLIC +PROTECTED access mode appears on the innerclass attributes, not on the class attributes.  I explored this space while developing the test, and if the class itself is marked protected, the failure occurs much earlier in the game when an attempt is made to load the subclass.  The test is written to mimic standalone (with bytecode rewriting) what was seen in the test.
> 
> - I decided to put the test in its own subdirectory of test/java/lang/invoke with its own informative name, and not directly in that directory, because all these invocation/protection tests have overlapping and near-overlapping class names, and so it would be confusing not to do this.  Unfortunately, that means there's now two copies of the BogoLoader checked in to the test tree.
> 
> David
>