eclipse-openj9 / openj9

Eclipse OpenJ9: A Java Virtual Machine for OpenJDK that's optimized for small footprint, fast start-up, and high throughput. Builds on Eclipse OMR (https://github.com/eclipse/omr) and combines with the Extensions for OpenJDK for OpenJ9 repo.
Other
3.28k stars 721 forks source link

Java 14 MethodHandles::privateLookupIn may not produce a Lookup with full privilege access #8571

Closed pshipton closed 4 years ago

pshipton commented 4 years ago

From the Java 14 release notes " The Lookup object produced by MethodHandles::privateLookupIn in this release may not have full privilege access. A lookup which possesses both PRIVATE and MODULE access modes is said to possess full privilege access that can be tested with Lookup::hasFullPrivilegeAccess method. Previously a Lookup returned from MethodHandles::privateLookupIn can be used to look up caller-sensitive methods. In Java SE 14, it may fail to look up caller-sensitive methods if the lookup does not have full privilege access (even it has private access mode). "

pshipton commented 4 years ago

@DanHeidinga pls take a look.

adam-thorpe commented 4 years ago

Test for this: https://github.com/ibmruntimes/openj9-openjdk-jdk14/blob/openj9/test/jdk/java/lang/invoke/MethodHandles/privateLookupIn/test/p/PrivateLookupInTests.java Which is run under java/lang/invoke/MethodHandles/privateLookupIn/Driver.java

adam-thorpe commented 4 years ago

Above test is looking for a currently unimplemented method LookUp::hasFullPrivilegeAccess() java/lang/invoke/modules/Driver.java and Driver1.java are looking for LookUp::previousLookupClass()

DanHeidinga commented 4 years ago

@ChengJin01 Can you take a look at this? Its need for the JDK14 release and so is a priority

ChengJin01 commented 4 years ago

Will check out to see how to deal with this situation.

ChengJin01 commented 4 years ago

I created a temp fix for the APIs involved but I am struggling to establish a test environment to reproduce the problem locally and verify my fix to see whether it covers every failing tests in the test suites (it also doesn't work in the internal jenkins as confirmed by @llxia )

@adam-thorpe , @sophia-guo, any idea how to set the test suite ups to run it locally?

ChengJin01 commented 4 years ago

With the help of @sophia-guo and @llxia, I manged to reproduce the failure locally:

        /.../openjdk-tests/openjdk/openjdk-jdk/test/jdk/java/lang/invoke/MethodHandles/privateLookupIn/test/p/PrivateLookupInTests.java:77: 
error: cannot find symbol
                assertTrue(lookup.hasFullPrivilegeAccess());
                                 ^
          symbol:   method hasFullPrivilegeAccess()
          location: variable lookup of type Lookup
        /.../openjdk-tests/openjdk/openjdk-jdk/test/jdk/java/lang/invoke/MethodHandles/privateLookupIn/test/p/PrivateLookupInTests.java:117: 
error: cannot find symbol
                assertFalse(lookup.hasFullPrivilegeAccess());
                                  ^
          symbol:   method hasFullPrivilegeAccess()
          location: variable lookup of type Lookup
        2 errors

I will create a build with my changes to see how it goes next.

DanHeidinga commented 4 years ago

See also the javadoc: http://cr.openjdk.java.net/~iris/se/14/latestSpec/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#hasPrivateAccess()

and for a discussion of the modes: http://cr.openjdk.java.net/~iris/se/14/latestSpec/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#privacc

We'll also need to review how we handle removing access bits in apis like dropLookupMode(int)

ChengJin01 commented 4 years ago

According to the Java 14 Spec, there are two new APIs added to the Spec:

public boolean hasFullPrivilegeAccess()
public Class<?> previousLookupClass()

and the content of 3 APIs are updated with the new APIs above.

public boolean hasPrivateAccess()
public static MethodHandles.Lookup privateLookupIn(..)
public MethodHandles.Lookup in(...)

I need to double-check one by once as my code changes don't cover all the situations:

STDOUT:
[TestNG] Running:
  java/lang/invoke/MethodHandles/privateLookupIn/Driver.java

config p.PrivateLookupInTests.init(): success
test p.PrivateLookupInTests.testAllAccessCallerSameModule(): success
test p.PrivateLookupInTests.testArrayClassAsTargetClass(): success
test p.PrivateLookupInTests.testCallerDoesNotRead(): success
test p.PrivateLookupInTests.testNotOpenToCaller(): success
test p.PrivateLookupInTests.testNullCaller(): success
test p.PrivateLookupInTests.testNullTargetClass(): success
test p.PrivateLookupInTests.testPrimitiveArrayClassAsTargetClass(): success
test p.PrivateLookupInTests.testPrimitiveClassAsTargetClass(): success
test p.PrivateLookupInTests.testPublicLookupSameModule(): success
test p.PrivateLookupInTests.testReducedAccessCallerSameModule(): success
test p.PrivateLookupInTests.testTargetClassInOpenModule(): failure
java.lang.AssertionError: expected [false] but found [true]
        at testng/org.testng.Assert.fail(Assert.java:94)
        at testng/org.testng.Assert.failNotEquals(Assert.java:496)
        at testng/org.testng.Assert.assertFalse(Assert.java:63)
        at testng/org.testng.Assert.assertFalse(Assert.java:73)
        at test/p.PrivateLookupInTests.testTargetClassInOpenModule(PrivateLookupInTests.java:117)
...
test p.PrivateLookupInTests.testTargetClassInUnnamedModule(): success

===============================================
java/lang/invoke/MethodHandles/privateLookupIn/Driver.java
Total tests run: 12, Failures: 1, Skips: 0

against the failing test case:

    // test reads m1, open module m1 containing p1
    public void testTargetClassInOpenModule() throws Throwable {
        // m1/p1.Type
        Class<?> clazz = Class.forName("p1.Type");
        assertEquals(clazz.getModule().getName(), "m1");

        // ensure that this module reads m1
        Module thisModule = getClass().getModule();
        Module m1 = clazz.getModule();
        thisModule.addReads(clazz.getModule());
        assertTrue(m1.isOpen("p1", thisModule));

        Lookup lookup = MethodHandles.privateLookupIn(clazz, MethodHandles.lookup());
        assertTrue(lookup.lookupClass() == clazz);
        assertTrue((lookup.lookupModes() & PRIVATE) != 0);
        assertFalse(lookup.hasFullPrivilegeAccess()); <-----
ChengJin01 commented 4 years ago

@DanHeidinga ,

Given that previousLookupClass() and in() are not covered in this test suite, should we address these two APIs in this issue or just wait till they are detected by https://github.com/ibmruntimes/openj9-openjdk-jdk14/blob/openj9/test/jdk/java/lang/invoke/AccessControlTest.java (to be address in another issue) ?

DanHeidinga commented 4 years ago

Let's address them here as we're aware they will be an issue. Easier if all the changes are together

ChengJin01 commented 4 years ago

Except the changes with the fully private access in hasPrivateAccess() and hasFullPrivilegeAccess(), a new concept called previous lookup class has been introduced since Java 14, which affects most of lookup specific operations, specifically including

public String toString() 
public Class<?> previousLookupClass()
Lookup.PUBLIC_LOOKUP
public static MethodHandles.Lookup lookup()
public static MethodHandles.Lookup privateLookupIn(..)
public MethodHandles.Lookup in(...)
public Class<?> accessClass(Class<?> targetClass)
...

Currently already finished the changes in (not yet verified)

public boolean hasPrivateAccess()
public boolean hasFullPrivilegeAccess()
public String toString() 
public Class<?> previousLookupClass()
public MethodHandles.Lookup dropLookupMode(int dropMode)
Lookup.PUBLIC_LOOKUP
public static MethodHandles.Lookup lookup()
public static MethodHandles.Lookup privateLookupIn(..)
public MethodHandles.Lookup in(...)

and now looking at accessClass() against the API Spec at http://cr.openjdk.java.net/~iris/se/14/build/latest/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#accessClass(java.lang.Class) It seems the description has been totally modified to accommodate the new concept as compared to the same API in Java 11. Probably need to refactor most of code in there (e.g. a new version of checkClassModuleVisibility) to deal with all situations described in this API Spec.

ChengJin01 commented 4 years ago

Already finished all Lookup specific code changes in the APIs listed above and working on solve the compilation errors as these APIs are not backward-compatible to the existing hehaivour. e.g.

Exception in thread "main" java/lang/IllegalAccessError
        at java/lang/invoke/MethodHandle.resolveInvokeDynamic (java.base@9/MethodHandle.java:1170)
        at jdk/internal/module/SystemModuleFinders$1.findAll (java.base@9/SystemModuleFinders.java:220)
...

against the code at MethodHandle.java

1157         private static final MethodHandle resolveInvokeDynamic(long j9class, String name, String methodDescriptor     , long bsmData) throws Throwable {
1158                 MethodHandle result = null;
1159                 MethodType type = null;
1160
1161                         VMLangAccess access = VM.getVMLangAccess();
1162                         Object internalRamClass = access.createInternalRamClass(j9class);
1163                         Class<?> classObject = getClassFromJ9Class(j9class);
1164
1165                         type = MethodType.vmResolveFromMethodDescriptorString(methodDescriptor, access.getClasslo     ader(classObject), null);
1166      updated code--->  final MethodHandles.Lookup lookup = new MethodHandles.Lookup(classObject, null, false);
1167                         try {
1168                                 lookup.accessCheckArgRetTypes(type);
1169                         } catch (IllegalAccessException e) {
1170        ---->  IllegalAccessError err = new IllegalAccessError();
ChengJin01 commented 4 years ago

Fixed all compilation issues in code changes and now working on the test failures in https://github.com/ibmruntimes/openj9-openjdk-jdk14/blob/openj9/test/jdk/java/lang/invoke/DropLookupModeTest.java.

DanHeidinga commented 4 years ago

Is there a draft PR with the changes so far? It would be good to get early reviews going on the changes

ChengJin01 commented 4 years ago

Currently working on the test failures in https://github.com/ibmruntimes/openj9-openjdk-jdk14/blob/openj9/test/jdk/java/lang/invoke/AccessControlTest.java. It looks like there is still code issue with the method in() when dealing with the case of UNCONDITIONAL.

ChengJin01 commented 4 years ago

@DanHeidinga,

One of tests at https://github.com/ibmruntimes/openj9-openjdk-jdk14/blob/openj9/test/jdk/java/lang/invoke/DropLookupModeTest.java is inconsistent with the Java 14 Spec in the case of UNCONDITIONAL as follows:

public void testBasic() {
...
lookup = fullPowerLookup.dropLookupMode(UNCONDITIONAL);
        assertTrue(lookup.lookupClass() == lc);
        assertTrue(lookup.lookupModes() == (PUBLIC|MODULE|PACKAGE|PRIVATE)); <--- 

This assertion is wrong as it still follows the Java 11 spec at http://cr.openjdk.java.net/~iris/se/11/build/latest/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#dropLookupMode(int):

PROTECTED and UNCONDITIONAL are always dropped 
and so the resulting lookup mode will never have these access capabilities.

rather than the Java 14 Spec at http://cr.openjdk.java.net/~iris/se/14/build/latest/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#dropLookupMode(int)

PROTECTED is always dropped and so 
the resulting lookup mode will never have this access capability. ...
If UNCONDITIONAL is dropped then the resulting lookup has no access. <----

However, the RI (Hotspot) JDK14 also sticks to the Java 11 Spec and ends up with PUBLIC|MODULE|PACKAGE|PRIVATE for the access modes after dropping UNCONDITIONAL.

Should we match the behavior of RI or strictly follow the Java 14 Spec to return NO_ACCESS in the case of UNCONDITIONAL?

If we follow the Spec, then the test above should be excluded or modified as follows:

lookup = fullPowerLookup.dropLookupMode(UNCONDITIONAL);
        assertTrue(lookup.lookupClass() == lc);
        assertTrue(lookup.lookupModes() == 0); <----- no access in such case

If not, then we will keep the same behaviour as Java 11 Spec to simply discard the UNCONDITIONAL bit.

ChengJin01 commented 4 years ago

Other weird test case at https://github.com/ibmruntimes/openj9-openjdk-jdk14/blob/openj9/test/jdk/java/lang/invoke/DropLookupModeTest.java such as:

@Test(dataProvider = "unconditionals")
    public void testUnconditionalLookup(Lookup unconditionalLookup, Class<?> expected) {
        assertTrue(unconditionalLookup.lookupModes() == UNCONDITIONAL);
      ...
        assertPublicLookup(unconditionalLookup.dropLookupMode(PUBLIC), expected); <-------

    private void assertPublicLookup(Lookup lookup, Class<?> expected) {
        assertTrue(lookup.lookupClass() == expected);
        assertTrue(lookup.lookupModes() == UNCONDITIONAL);
    }

According to the Java 14 & Java 11 Spec:

If PUBLIC is dropped then the resulting lookup has no access

which means the resulting lookup mode should be NO_ACCESSrather than UNCONDITIONALas long as PUBLICis dropped.

It surprised me that the test above passed on the RI (Hotspot) JDK14. It looks like RI removed all mode bits except UNCONDITIONAL which is inconsistent with the Java 14 Spec (all mode bits including UNCONDITIONAL should be removed when PUBLIC is dropped, the same as Java 11 Spec).

Such similar issue were also detected on other Jtreg tests. So we need to determine whether to totally match the behaviour of RI regardless of the Java 14 Spec if any inconsistency so as to avoid excluding/modifying any Jtreg test.

DanHeidinga commented 4 years ago

@andrew-m-leonard Can you bring these issues to the OpenJDK mailing list? Cheng has identified two JTREG tests that the RI passes which appear to contradict the spec

DanHeidinga commented 4 years ago

@ChengJin01 the review has been delayed due to github soft outage issues - I can't currently comment on the review

ChengJin01 commented 4 years ago

@DanHeidinga, no worries on the review as the code changes is still being updated.

Currently I will modifying the failing test cases against the Java 14 Spec to see how it goes.

ChengJin01 commented 4 years ago

Also the same issue was detected at https://github.com/ibmruntimes/openj9-openjdk-jdk14/blob/openj9/test/jdk/java/lang/invoke/AccessControlTest.java

        LookupCase dropLookupMode(int modeToDrop) {
            int oldModes = lookupModes();
            int newModes = oldModes & ~(modeToDrop | PROTECTED);
            switch (modeToDrop) {
                case PUBLIC: newModes &= ~(MODULE|PACKAGE|PROTECTED|PRIVATE); break; 
<---- should be newModes = 0 for PUBLIC according to Java 14 Spec

                case MODULE: newModes &= ~(PACKAGE|PRIVATE); break;
                case PACKAGE: newModes &= ~(PRIVATE); break;
                case PROTECTED:
                case PRIVATE:
                case UNCONDITIONAL: break; 
<---- should be newModes = 0 for UNCONDITIONAL according to Java 14 Spec
                default: throw new IllegalArgumentException(modeToDrop + " is not a valid mode to drop");
}
andrew-m-leonard commented 4 years ago

maillist discussion: https://mail.openjdk.java.net/pipermail/jdk-dev/2020-February/003973.html

andrew-m-leonard commented 4 years ago

see response from Chris Hegarty... @ChengJin01 @DanHeidinga

ChengJin01 commented 4 years ago

Just posted the response from Chris at https://mail.openjdk.java.net/pipermail/jdk-dev/2020-February/003974.html

> Which is correct the code/testcase or the jdk14 spec ?

I had the same observation recently when reading the updated Java 14
version of this spec. The wording is not strictly incorrect, but could
benefit from a little clarification.

The reason I say that it is not _strictly_ incorrect is that it says
"is dropped". For a lookup mode to actually be dropped then it must
first be held by the lookup. In your examples the lookup does not hold
the access mode that is passed to be dropped, so the mode is not
actually dropped, hence a _no access_ lookup is not returned.

I think that the implementation is behaving as desired, but I do think
that the spec wording could be improved a little ( since I had similar
initial confusion about this point, just as you had ).

@DanHeidinga, if Chris's clarification is logically reasonable (only drop when the mode exists in the access mode; otherwise do nothing), then our existing implementation in dropLookupMode() (irrelevant to Java 14) was totally wrong from very beginning as it chooses to drop all modes involved whether nor not the mode (to be dropped) exists in the access mode.

If so, we need to refactor the existing code of dropLookupMode() to match the behaviour. Anyway, the API Spec should be updated to avoid any confusion in the future.

ChengJin01 commented 4 years ago

https://docs.oracle.com/javase/9/docs/api/java/lang/invoke/MethodHandles.Lookup.html#dropLookupMode-int-

Creates a lookup on the same lookup class which this lookup object finds members, 
but with a lookup mode that has lost the given lookup mode

http://cr.openjdk.java.net/~iris/se/11/build/latest/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#dropLookupMode(int)

Creates a lookup on the same lookup class which this lookup object finds members, 
but with a lookup mode that has lost the given lookup mode. 

http://cr.openjdk.java.net/~iris/se/14/build/latest/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#accessClass(java.lang.Class)

Creates a lookup on the same lookup class which this lookup object finds members, 
but with a lookup mode that has lost the given lookup mode.

The description of the mode to be dropped never changes when dropLookupMode() was introduced since Java 9.

ChengJin01 commented 4 years ago

I already modified the code in dropLookupMode() (at https://github.com/eclipse/openj9/pull/8657) to deal with the situation and passed all tests in https://github.com/ibmruntimes/openj9-openjdk-jdk14/blob/openj9/test/jdk/java/lang/invoke/DropLookupModeTest.java.

Now keep working on the failing tests captured in https://github.com/ibmruntimes/openj9-openjdk-jdk14/blob/openj9/test/jdk/java/lang/invoke/AccessControlTest.java.

andrew-m-leonard commented 4 years ago

copy of post by Mandy Chung (Oracle): I see the confusion and we should improve the documentation. I have created JDK-8240242 to improve the javadoc.

I also recommend to check out the Access modes section added in 14 that gives detail examples on the lookup object resulting from Lookup::in and dropLookupModes.

https://download.java.net/java/early_access/jdk15/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#access-modes

ChengJin01 commented 4 years ago

@andrew-m-leonard,

Many thanks for posting the response from Mandy Chung (Oracle).

I went over the Access modes section added in https://download.java.net/java/early_access/jdk14/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#access-modes and https://download.java.net/java/early_access/jdk15/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#access-modes but neither of them mentioned that the mode to be dropped should exist in the existing access mode. I think this is not specific to Java 14 but Java 11 when dropLookupModeswas introduced for the first time. So they should update the doc in such case.

As for Lookup::in(), could you help to forward my questions as to teleporting to Oracle or the OpenJDK mailing list as follows:

1) For teleporting across modules, given that teleporting to and back to the same named module is allowed to have PUBLIC access according to the Spec at https://download.java.net/java/early_access/jdk14/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#access-modes:

Cross-module lookups
...
 Teleporting across modules drops the ability to access non-exported classes 
in both the module of the new lookup class and the module of the old lookup class 
and the resulting Lookup remains only PUBLIC access. 

e.g.

CL = MethodHandles.lookup() in C
CL.in(D) different module
CL.in(D).in(C) hop back to the original module

then why just teleporting to and back to the unnamed module should lost all access including PUBLIC (detected in https://github.com/ibmruntimes/openj9-openjdk-jdk14/blob/openj9/test/jdk/java/lang/invoke/AccessControlTest.java) e.g.

    private void addLookupEdge(LookupCase l1, Class<?> c2, LookupCase l2, int dropAccess) {
    LookupCase expect = dropAccess == 0 ? l1.in(c2) : l1.in(c2).dropLookupMode(dropAccess);
    <----------dropAccess is 0 in which case it calls l1.in(c2)

   The following messages are added in the test code for troubleshooting:
    dropAccess = 0
    l1 = Object/test.java.lang.invoke.AccessControlTest/public/loader#0
    l1.lookupClass = class java.lang.Object
    l1.lookupClass().getModule = module java.base
    l1.prevLookupClass = class test.java.lang.invoke.AccessControlTest
    l1.lookupModes = 1 (PUBLIC)

    c2 = class test.java.lang.invoke.AccessControlTest
    c2.getModule = unnamed module @50019bf9
    c2.getModule()) and l1.prevAccessClassModule in the same module

    expect = AccessControlTest/java.lang.Object/noaccess
    expect.lookupModes = 0 <---------- why all access should be dropped?

So, I am wondering whether there is any difference in dealing with namedand unnamed modules in teleporting across modules (never emphasized on this in the doc).

2) Meanwhile, please clarify another confusion as to Lookup::in in the doc as follows:

Cross-module lookups
...
If the target class is in a different module from M1 (C's module), 
C becomes the new previous lookup class and the target class becomes 
the new lookup class. In that case, if there was already a previous lookup class in M0, 
and it differs from M1 and M2, then the resulting lookup drops all privileges. 

and in Lookup::in:

If the new lookup class, the old lookup class and the previous lookup class 
are all in different modules i.e. teleporting to a third module, 
all access modes are lost. 

My question is, does it mean (M0 != M1 and M0 != M2) or (M0 != M1 and M0 != M2 and M1 != M2) or both of them? any difference in the case of named or unnamed modules?

If either of them is true, then why the test code only check (M0 != M1 and M1 != M2) (different from the Spec) at https://github.com/ibmruntimes/openj9-openjdk-jdk14/blob/openj9/test/jdk/java/lang/invoke/AccessControlTest.java ?

         * [A7] If the new lookup class, the old lookup class and the previous lookup class
         * are all in different modules i.e. teleporting to a third module,
         * all access modes are lost.

        public LookupCase in(Class<?> c2) {
Module m0 = prevLookupClass() != null ? prevLookupClass.getModule() : c1.getModule();
Module m1 = c1.getModule();
Module m2 = c2.getModule();
...
            if (m2 != m1 && m0 != m1) { <-------
                // hop to a third module; lose all access
                changed |= (PUBLIC|MODULE|PACKAGE|PRIVATE|PROTECTED);  // [A7]
            }
...
int modes2 = modes1 & ~changed;
...
LookupCase l2 = new LookupCase(c2, plc, modes2);

There should be some explanation from Oracle at this point against the Spec.

ChengJin01 commented 4 years ago

@andrew-m-leonard,

There is another case I'd like to confirm with Oracle is testDropLookupMode() at test/jdk/java/lang/invoke/modules/m3/jdk/test/ModuleAccessTest.java

   public void testDropLookupMode() throws Exception {
        Lookup lookup = MethodHandles.privateLookupIn(m5.type1, m4.lookup);
        assertTrue((lookup.lookupModes() & MODULE) == 0); <--- MODULE doesn't exist
        ...
        Lookup lookup3 = lookup.dropLookupMode(MODULE);
---> assertTrue(lookup3.lookupModes() == (lookup.lookupModes() & ~(PROTECTED|PRIVATE|PACKAGE)));

Based on the expected result above, does it mean PRIVATE, PACKAGE should be dropped whether or not MODULE exists in the access mode (PROTECTED is dropped by default) ? If so, the document should be updated to explicitly clarify the exception case.

ChengJin01 commented 4 years ago

All jtreg test failures with Lookup have been fixed and all tests passed with the latest code in #8657 as follows: [1]export JDK_CUSTOM_TARGET=test/jdk/java/lang/invoke/MethodHandles/privateLookupIn/Driver.java jtreg_privateLookupIn_log.txt

PASSED test targets:
        jdk_custom_0

TOTAL: 1   EXECUTED: 1   PASSED: 1   FAILED: 0   DISABLED: 0   SKIPPED: 0
ALL TESTS PASSED

[2]export JDK_CUSTOM_TARGET=test/jdk/java/lang/invoke/lookup/LookupClassTest.java jtreg_LookupClassTest_log.txt

PASSED test targets:
        jdk_custom_0

TOTAL: 1   EXECUTED: 1   PASSED: 1   FAILED: 0   DISABLED: 0   SKIPPED: 0
ALL TESTS PASSED

[3]export JDK_CUSTOM_TARGET=test/jdk/java/lang/invoke/DropLookupModeTest.java (fixed two failing test cases against the Java 14 Spec with UNCONDITIONAL) jtreg_DropLookupModeTest_log.txt

PASSED test targets:
        jdk_custom_0

TOTAL: 1   EXECUTED: 1   PASSED: 1   FAILED: 0   DISABLED: 0   SKIPPED: 0
ALL TESTS PASSED

[4]export JDK_CUSTOM_TARGET=test/jdk/java/lang/invoke/AccessControlTest.java jtreg_AccessControlTest_log.txt

PASSED test targets:
        jdk_custom_0

TOTAL: 1   EXECUTED: 1   PASSED: 1   FAILED: 0   DISABLED: 0   SKIPPED: 0
ALL TESTS PASSED

[5]export JDK_CUSTOM_TARGET=test/jdk/java/lang/invoke/modules/Driver.java (Basic test case for module access checks and Lookup.in) jtreg_LookupIn_log.txt

PASSED test targets:
        jdk_custom_0

TOTAL: 1   EXECUTED: 1   PASSED: 1   FAILED: 0   DISABLED: 0   SKIPPED: 0
ALL TESTS PASSED

[6]export JDK_CUSTOM_TARGET=test/jdk/java/lang/invoke/modules/Driver1.java (Basic test case for module access checks and Lookup.in and MethodHandles.privateLookupIn) test/jdk/java/lang/invoke/modules/m3/jdk/test/ModuleAccessTest.java jtreg_ModuleAccessTest_log.txt

PASSED test targets:
        jdk_custom_0

TOTAL: 1   EXECUTED: 1   PASSED: 1   FAILED: 0   DISABLED: 0   SKIPPED: 0
ALL TESTS PASSED

The code change will be modified accordingly if any update from Oracle as to the behaviour of Lookup:in(). Now I will launch personal builds to deal with the internal tests.

ChengJin01 commented 4 years ago

I double-checked the test cases with Lookup::in() previously mentioned at https://github.com/eclipse/openj9/issues/8571#issuecomment-593527947 against the Java 14 Spec at http://cr.openjdk.java.net/~iris/se/14/build/latest/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#in(java.lang.Class) and https://download.java.net/java/early_access/jdk14/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#in(java.lang.Class)

and noticed that the keyword accessible in the statement of If the new lookup class is not accessible to this lookup... is a hyperlink to https://download.java.net/java/early_access/jdk14/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#accessClass(java.lang.Class), which means the access check has to done by calling Lookup::accessClass() since Java 14 in Lookup::in() (the hyperlink doesn't occur in Java 11, 12 and 13 Spec).

For now, the failing test cases at https://github.com/eclipse/openj9/issues/8571#issuecomment-593527947 was specially addressed to get it work. Given that accessClass() needs to be explicitly called in Lookup::in() at this point, I will try to replace the special code there with the invocation of accessClass() to see how it goes. If everything works fine with this replacement, that means Oracle already clarified in the Java 14 Spec as to how to check the accessibility of the new lookup class and there is no need to check with Oracle at this point.

ChengJin01 commented 4 years ago

I modified the code in Lookup::in() to call accessClass() to check the accessibility of the requested lookup class (as explicitly required in Java 14), all jtreg tests still passed with the new changes. jtreg_Lookup_tests_log.txt

So the only test case left to be clarified from Oracle is testDropLookupMode() at test/jdk/java/lang/invoke/modules/m3/jdk/test/ModuleAccessTest.java (already mentioned at https://github.com/eclipse/openj9/issues/8571#issuecomment-593683239):

   public void testDropLookupMode() throws Exception {
        Lookup lookup = MethodHandles.privateLookupIn(m5.type1, m4.lookup);
        assertTrue((lookup.lookupModes() & MODULE) == 0); <--- MODULE doesn't exist
        ...
        Lookup lookup3 = lookup.dropLookupMode(MODULE);
---> assertTrue(lookup3.lookupModes() == (lookup.lookupModes() & ~(PROTECTED|PRIVATE|PACKAGE)));

Based on the expected result above, does it mean PRIVATE, PACKAGE should be dropped whether or not MODULE exists in the access mode (PROTECTED is dropped by default) ? If so, the document should be updated to explicitly clarify the exception case.

FYI @andrew-m-leonard