Skip parameter access check for reflect objects

[JasonFengJ9]

Skip parameter access check for reflect objects

Introduced a boolean flag to indicate that MethodType parameters should be skipped for access checking;
Skip parameter access check for reflect objects;
Adopted this extra parameter in other APIs.

Manually verified that this PR fixes #2355

Peer reviewer: @tajila
Senior reviewer: @gacholio

Signed-off-by: Jason Feng fengj@ca.ibm.com

[tajila]

don't MethodHandle lookups also get the implied read access?

[JasonFengJ9]

j.l.MethodHandles.findSpecial() performs checkSpecialAccess(specialToken) which ensures PRIVATE access or accessClass equals callerClass hence checkClassModuleVisibility() Objects.equals(accessModule, targetModule) is true regardless of reflectiveAccess. In this case specifying true or false in this checkAccess(handle, reflectiveAccess) won't make a difference.

On the other hand, not all MethodHandle lookups have the implied read access, for example, j.l.MethodHandles.findVirtual() in following code snippet doesn't imply read access to unnamed modules:

Class<?> lookupTarget = Class.forName("class.to.be.loaded.from.classpath");
MethodHandles.lookup().findVirtual(lookupTarget, "testFindSpecial", MethodType.methodType(void.class));

This should throw java.lang.IllegalAccessException: com.greetings no access to: atestse.java_lang_invoke.

[JasonFengJ9]

@tajila does comment above address your concern?

[irinarada]

@JasonFengJ9 - given the above and this being marked as a blocker for the OpenJ9 builds, do we have an ETA for this?

[irinarada]

@JasonFengJ9 - any update? this is blocking RC2

[JasonFengJ9]

Still investigating to figure out a way to match RI behaviours when fixing the issue reported.
Hopefully the PR will be updated for another reviewing in one day or two.

[JasonFengJ9]

The behaviour difference between OpenJ9 and OpenJDK seems in following code snippet:

j.l.i.MethodHandles.unreflectConstructor(Constructor)

In the scenario reported, this code was invoked from module java.base, incoming Constructor public jdk.nashorn.javaadapters.java_lang_Iterable(jdk.nashorn.internal.runtime.ScriptObject) is from a unnamed module in which the Constructor parameter is a class from a named module jdk.scripting.nashorn that its package jdk.nashorn.internal.runtime is not exported unconditionally.

Following a simplified testcase can reproduce the problem:
Module module.one doesn't export package module.one.pkg unconditionally.

module module.one {
  exports module.one.pkg to module.notexist;
}

A unnamed module implements a method getReflectConstructor() to return a reflected constructor which has a parameter ModuleOne from named module module.one.

  public static Constructor<?> getReflectConstructor() {
    Constructor constructor = null;
    try {
        Class<?> moduleOneClz = Class.forName("unnamed.module.pkg.ConstructorNamedModule");
        constructor = moduleOneClz.getConstructor(ModuleOne.class);
        return constructor;
    } catch (Throwable e) {
        e.printStackTrace();
    }
    return constructor;
  }
public class ConstructorNamedModule {
  public ConstructorNamedModule(ModuleOne mo) {
  }
}

Another named module ModuleTwo invokes j.l.i.MethodHandles.unreflectConstructor(Constructor)

      MethodHandles.Lookup lookup = MethodHandles.lookup();
      MethodHandle reflectConstructor = lookup.unreflectConstructor(UnnamedModule.getReflectConstructor());

OpenJ9 throws following error:

java.lang.IllegalAccessError: 'module.two' no access to: 'module.one.pkg'
	at java.lang.invoke.MethodHandles$Lookup.checkAccess(java.base@9.0.4-adoptopenjdk/MethodHandles.java:370)
	at java.lang.invoke.MethodHandles$Lookup.checkAccess(java.base@9.0.4-adoptopenjdk/MethodHandles.java:314)
	at java.lang.invoke.MethodHandles$Lookup.unreflectConstructor(java.base@9.0.4-adoptopenjdk/MethodHandles.java:1144)
	at module.two.pkg.ModuleTwo.testAll(module.two/ModuleTwo.java:21)
	at module.two.pkg.ModuleTwo.main(module.two/ModuleTwo.java:9)
Caused by: java.lang.IllegalAccessException: 'module.two' no access to: 'module.one.pkg'
	at java.lang.invoke.MethodHandles$Lookup.checkClassModuleVisibility(java.base@9.0.4-adoptopenjdk/MethodHandles.java:660)
	at java.lang.invoke.MethodHandles$Lookup.checkAccess(java.base@9.0.4-adoptopenjdk/MethodHandles.java:367)
	... 4 more

Note this java.lang.IllegalAccessError is not caused by a check for module readability (actually adding --add-reads module.two=module.one got same error). It is due to the package in question was not exported unconditionally (not from module.one to module.two either).

if (INTERNAL_PRIVILEGED != accessMode) {
	Module targetModule = targetClass.getModule();
	String targetClassPackageName = targetClass.getPackageName();
	if ((UNCONDITIONAL & accessMode) == UNCONDITIONAL) {
		/* publicLookup objects can see all unconditionally exported packages. */
		if (!targetModule.isExported(targetClassPackageName)) {
from here  --> throw new IllegalAccessException(Msg.getString("K0587", accessModule.getName(), targetClassPackageName)); //$NON-NLS-1$   
		}
	} else if (!(
		Objects.equals(accessModule, targetModule)
			|| (targetModule.isExported(targetClassPackageName, accessModule) && accessModule.canRead(targetModule)))
			) {
		String message = Msg.getString("K0587", accessModule.getName(), targetClassPackageName); //$NON-NLS-1$
		throw new IllegalAccessException(message);
	}
}

OpenJDK runs this test without error. The only possible way to avoid such IllegalAccessError from my observation is to skip the access check for the Constructor parameter(s).

However I haven't find any specification to support such behaviours yet.

[JasonFengJ9]

@tajila please have another look.

Will add automated tests a bit later.

[JasonFengJ9]

@gacholio could you review?

[gacholio]

jenkins test sanity xlinux jdk8

[DanHeidinga]

@JasonFengJ9 Are you planning to add an FV test for this case?

[JasonFengJ9]

@DanHeidinga yes, it's in progress.