Check module visibility in method handles #1801

pdbain-ibm · 2018-04-30T16:55:30Z

Ensure target classes, paramater types, and return types are visible.

Signed-off-by: Peter Bain peter_bain@ca.ibm.com

pdbain-ibm · 2018-04-30T16:59:55Z

DanHeidinga · 2018-04-30T17:36:00Z

The PR only adds this check to findVirtual. What about the other find{static,special,bind,setter,getter} methods? Do they need the module access checks as well?

pdbain-ibm · 2018-04-30T17:49:47Z

@dan The personal build has nearly finished and shows no errors so far. Would you kindly run a Jenkins sanity build?

DanHeidinga · 2018-04-30T17:31:39Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java

@@ -47,7 +47,7 @@
 import com.ibm.oti.vm.VM;
 import com.ibm.oti.vm.VMLangAccess;

-/*[IF Sidecar19-SE]
+/*[IF Sidecar19-SE]*/


why this change?

To reduce the syntax errors when editing in the new code in Eclipse.

Doesn't that just move the syntax error from 9 to 8?

No: this change eliminates most of the syntax errors in the unprocessed code.

DanHeidinga · 2018-04-30T17:33:02Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java

@@ -634,14 +636,26 @@ public MethodHandle findStatic(Class<?> clazz, String methodName, MethodType typ
 		 * @param methodName - the name of the method
 		 * @param type - the type of the method
 		 * @return a MethodHandle that will do virtual dispatch on the first argument
-		 * @throws IllegalAccessException - if method is static or access is refused
+		 * @throws IllegalAccessException - if method is static or access is refused or clazz's package is not exported to the current module 


This javadoc will be generated for both 8 & 9+. Module should only be mentioned for 9

DanHeidinga · 2018-04-30T17:33:16Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java

@@ -634,14 +636,26 @@ public MethodHandle findStatic(Class<?> clazz, String methodName, MethodType typ
 		 * @param methodName - the name of the method
 		 * @param type - the type of the method
 		 * @return a MethodHandle that will do virtual dispatch on the first argument
-		 * @throws IllegalAccessException - if method is static or access is refused
+		 * @throws IllegalAccessException - if method is static or access is refused or clazz's package is not exported to the current module 
+		 * @throws IllegalAccessError - if the type's packages are not exported to the current module 


module mentioned in 8

DanHeidinga · 2018-04-30T17:50:02Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java

 		 * @throws NullPointerException - if clazz, methodName or type is null
 		 * @throws NoSuchMethodException - if clazz has no virtual method named methodName with signature matching type
 		 * @throws SecurityException - if any installed SecurityManager denies access to the method
 		 */
 		public MethodHandle findVirtual(Class<?> clazz, String methodName, MethodType type) throws IllegalAccessException, NoSuchMethodException {
 			nullCheck(clazz, methodName, type);

+			/*[IF Sidecar19-SE]*/
+			if (INTERNAL_PRIVILEGED != accessMode) {


shouldn't this be added to checkAccess()?

pdbain-ibm · 2018-04-30T17:53:02Z

What about the other find{static,special,bind,setter,getter} methods?

Possibly. We don't have tests for these yet.

pdbain-ibm · 2018-05-02T17:05:35Z

find{static,special,bind,setter,getter} methods

Working on tests for those.

DanHeidinga · 2018-05-04T19:44:16Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java

+				MethodType type = handle.type();
+				Module accessModule = accessClass.getModule();
+				checkClassModuleVisibility(accessModule, targetClass, false);
+				checkClassModuleVisibility(accessModule, type.returnType, true);


There are two calls to checkClassModuleVisibility, one setting throwError to false while the other says true. Which is intended?

The throwable depends on the context. If the target class is not visible, we need to throw an IllegalAccessException. If the parameter or return types are not visible, we need to throw a IllegalAccessError.

DanHeidinga · 2018-05-04T19:45:10Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java

+				/*[MSG "K0664", "{0} is not in an exported package"]*/
+				String message = Msg.getString("K0664", targetClass.getPackageName()); //$NON-NLS-1$
+				if (throwError) {
+					throw new IllegalAccessError(message);


likely better to only throw one exception and let the code that needs the Error wrap the Exception.

DanHeidinga · 2018-05-04T19:45:34Z

jcl/src/java.base/share/classes/com/ibm/oti/util/ExternalMessages-MasterIndex.properties

@@ -1160,6 +1160,7 @@ K0660="Internal error while obtaining private constructor."
 K0661="Internal error while obtaining GcInfo instance."
 K0662="maxDepth must not be negative."
 K0663="Invalid or unsupported dump agent option, cannot be triggered."
+K0664="{0} is not in an exported package"


Is there no existing message that can be reused?

Yes. Fixed.

DanHeidinga · 2018-05-04T19:46:23Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java

+				Module accessModule = accessClass.getModule();
+				checkClassModuleVisibility(accessModule, targetClass, false);
+				checkClassModuleVisibility(accessModule, type.returnType, true);
+				for (Class<?> c: type.parameterArray()) {


type.parameterArray() allocates a new array - since this code is in the j.l.i package and we know it won't modify the array, better to use type.arguments directly

DanHeidinga · 2018-05-04T19:46:51Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java

+		 * @throws IllegalAccessError thrown if throwError is true
+		 * @throws IllegalAccessException thrown if throwError is false
+		 */
+		static void checkClassModuleVisibility(Module accessModule, Class<?> targetClass, boolean throwError) 


Please put the throw clauses on the same line.

DanHeidinga · 2018-05-04T19:58:54Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java

+			if (!Objects.equals(accessModule, targetModule)
+					&& !(targetModule.isExported(targetClass.getPackageName(), accessModule)
+							&& accessModule.canRead(targetModule))) {
+				/*[MSG "K0664", "{0} is not in an exported package"]*/


Can you add the two modules to this message? It will be easier for a user to resolve an access failure if they get the module info in the exception.

DanHeidinga · 2018-05-04T19:59:42Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java

+		static void checkClassModuleVisibility(Module accessModule, Class<?> targetClass, boolean throwError) 
+				throws IllegalAccessError, IllegalAccessException {
+			Module targetModule = targetClass.getModule();
+			if (!Objects.equals(accessModule, targetModule)


Can you rewrite this as positive conditions?

pdbain-ibm · 2018-05-07T20:30:27Z

Passes designer tests. Will run a larger regression suite tomorrow.

pdbain-ibm · 2018-05-08T13:28:27Z

JCL tests pass. @DanHeidinga or @pshipton could you take another look at the code?

DanHeidinga · 2018-05-08T13:43:21Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java

+						checkClassModuleVisibility(accessModule, c);
+					}
+				} catch (IllegalAccessException e) {
+					throw new IllegalAccessError(e.getMessage());


It's better to chain the exceptions by setting the IllegalAccessException as the cause - either by initCause() or through an appropriate constructor - so the original stack trace is preserved.

This method is used a lot of places. Is IllegalAccessError expected by the callers? Wouldn't it be more consistent to have this function throw IAException and only callers that expect IAError to do the transformation?

DanHeidinga · 2018-05-08T13:46:01Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java

-				
-				/* The following access checking is for a normal class (non-member class) */
+
+					/* The following access checking is for a normal class (non-member class) */


This comment applies to the else if block, if it applies to anything at all, and should have kept its original indentation

DanHeidinga · 2018-05-08T14:13:03Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java

+		 * @param type - the type of the method
+		 * @return a MethodHandle that will do virtual dispatch on the first argument
+		 * @throws IllegalAccessException - if method is static or access is refused or clazz's package is not exported to the current module 
+		 * @throws IllegalAccessError - if the type's packages are not exported to the current module 


Has the RI thrown IAError from findVirtual?

The convention is usually that the VM throws IllegalAccessError and reflective style operations throw Exceptions. We're using the same code to do double duty as the VM is calling into Java for these lookups.

I would expect exception / error conversion to happen at the boundaries.

Has the RI thrown IAError from findVirtual?

Yes. This matches the behaviour of the reference implementation.

DanHeidinga · 2018-05-08T14:17:08Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java

 			/*[MSG "K0587", "'{0}' no access to: '{1}'"]*/
 			throw new IllegalAccessException(com.ibm.oti.util.Msg.getString("K0587", accessClass.getName(), targetClass.getName()));  //$NON-NLS-1$
 		}
+
+		void checkLookupVisibility(Class<?> targetClass) throws IllegalAccessException, IllegalAccessError {
+			if (this.equals(PUBLIC_LOOKUP)) {


Rather than checking is identically PUBLIC_LOOKUP, we should check for the UNCONDITIONAL mode access bit - see https://docs.oracle.com/javase/9/docs/api/java/lang/invoke/MethodHandles.html#publicLookup--

DanHeidinga · 2018-05-08T14:18:55Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandles.java

+		 * @param targetClass Class which the referring class is accessing
+		 * @throws IllegalAccessException if the targetClass is not visible
+		 */
+		static void checkClassModuleVisibility(Module accessModule, Class<?> targetClass) throws IllegalAccessError, IllegalAccessException {


This probably also needs to take the UNCONDITIONAL access bit into account (ie: public lookup)

pdbain-ibm · 2018-05-11T12:16:14Z

The current change causes a regression in another test. I am investigating.

Check that the target class, parameter types, and return types of a method handle are obey module access restrictions. Signed-off-by: Peter Bain <peter_bain@ca.ibm.com>

pdbain-ibm · 2018-05-11T16:39:38Z

Regression was due to stale base code.
@DanHeidinga or @pshipton would you kindly review and run a PR build?

pdbain-ibm · 2018-05-18T12:35:14Z

@DanHeidinga or @pshipton do you have any comments on this PR? If not, could you
please run a sanity build?

Thank you.

DanHeidinga · 2018-05-22T03:17:20Z

Jenkins test sanity plinux jdk8,jdk9

DanHeidinga added jdk9 comp:vm labels Apr 30, 2018

pdbain-ibm changed the title ~~WIP Check module visibility in method handles~~ Check module visibility in method handles Apr 30, 2018

DanHeidinga reviewed Apr 30, 2018

View reviewed changes

pdbain-ibm force-pushed the modularity_functional branch from 1682dda to b8a104d Compare May 1, 2018 12:14

DanHeidinga reviewed May 4, 2018

View reviewed changes

pdbain-ibm force-pushed the modularity_functional branch from b8a104d to 1ea8a4b Compare May 7, 2018 20:28

pdbain-ibm changed the title ~~Check module visibility in method handles~~ WIP Check module visibility in method handles May 7, 2018

pdbain-ibm changed the title ~~WIP Check module visibility in method handles~~ Check module visibility in method handles May 8, 2018

DanHeidinga reviewed May 8, 2018

View reviewed changes

pshipton added this to the Release 0.9.0 milestone May 9, 2018

pdbain-ibm force-pushed the modularity_functional branch from 1ea8a4b to 032fe21 Compare May 9, 2018 14:45

pdbain-ibm force-pushed the modularity_functional branch from 032fe21 to 19f688c Compare May 11, 2018 15:40

Fix module access checks for method handles

d137536

Check that the target class, parameter types, and return types of a method handle are obey module access restrictions. Signed-off-by: Peter Bain <peter_bain@ca.ibm.com>

pdbain-ibm force-pushed the modularity_functional branch from 19f688c to d137536 Compare May 11, 2018 16:35

DanHeidinga self-assigned this May 22, 2018

DanHeidinga merged commit 286d0a3 into eclipse-openj9:master May 22, 2018

pdbain-ibm deleted the modularity_functional branch May 22, 2018 14:37

pdbain-ibm mentioned this pull request Jun 1, 2018

MethodHandles.lookup().findVirtual() on unexported package does not throw exception #1738

Closed


		/* The following access checking is for a normal class (non-member class) */

		/* The following access checking is for a normal class (non-member class) */

Check module visibility in method handles #1801

Check module visibility in method handles #1801

Conversation

pdbain-ibm commented Apr 30, 2018

pdbain-ibm commented Apr 30, 2018

DanHeidinga commented Apr 30, 2018

pdbain-ibm commented Apr 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdbain-ibm commented Apr 30, 2018

pdbain-ibm commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdbain-ibm commented May 7, 2018

pdbain-ibm commented May 8, 2018 • edited by DanHeidinga Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdbain-ibm commented May 11, 2018

pdbain-ibm commented May 11, 2018

pdbain-ibm commented May 18, 2018

DanHeidinga commented May 22, 2018

pdbain-ibm commented May 8, 2018 •

edited by DanHeidinga

Loading