Fix checking method handle access #3592

pdbain-ibm · 2018-11-07T16:44:34Z

Use the defining class of a class member or the target class of the lookup as
appropriate. Also reinstate missing external message definition and replace duplicated
code with a common method.

Fixes #3301

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

pdbain-ibm · 2018-11-14T14:50:55Z

Will update to show the correspondence with bytecode behaviour.

pdbain-ibm · 2018-11-14T17:30:22Z

resolveField.cpp:findFieldAndCheckVisibility() calls checkVisibility() on the defining class of the target field (ca. line 324). visible.c:checkVisibility() succeeds if the target field is private and in the same class as the access class (ca. line 134). This corresponds to MethodHandles.checkAccess() ca. line 435.

checkVisibility() passes if the field is protected and in a superclass of the access class (ca. line 159). This corresponds to MethodHandles.checkAccess() ca. line 460.

Minor tweak to commit, tested with JTReg test and Jsr292 tests.
Would a committer please run sanity and extended builds. Any platform will do.
Thank you

pdbain-ibm · 2018-11-27T14:28:22Z

This fixes #3301 JTReg test fail : java/lang/invoke/ProtectedMemberDifferentPackage/Test.java. Would @pshipton or other committer please run a pull request build?

pdbain-ibm · 2018-12-04T13:35:39Z

Updating and retesting this...

pdbain-ibm · 2018-12-04T20:54:17Z

Ready for review @DanHeidinga

DanHeidinga · 2018-12-06T19:39:39Z

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

+		 * @return true if accessMode is INTERNAL_PRIVILEGED
+		 * @throws IllegalAccessException if accessMode is NO_ACCESS
+		 */
+		private boolean isNonPrivilegedAccessThrowOnNoAccess() throws IllegalAccessException {


Given this is PR is a change to code that's both historically messy and a source of bugs, can you revert the unnecessary parts so it is easier to determine what is required?

It'll make it much easier to understand the history of this in the future.

DanHeidinga · 2018-12-11T15:39:23Z

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

@@ -407,7 +409,7 @@ void checkAccess(Class<?> clazz) throws IllegalAccessException {
 		 * @throws IllegalAccessException If the member is not accessible.
 		 * @throws IllegalAccessError If a handle argument or return type is not accessible.
 		 */
-		private void checkAccess(Class<?> targetClass, String name, int memberModifiers, MethodHandle handle, boolean skipAccessCheckPara) throws IllegalAccessException {
+		private void checkAccess(Class<?> targetClass, Class<?> definingClass, String name, int memberModifiers, MethodHandle handle, boolean skipAccessCheckPara) throws IllegalAccessException {


I really dislike changing the meaning of the variable name here as it makes it hard to review whether changes were deliberate or not.

DanHeidinga · 2018-12-11T15:40:15Z

Sorry, just realized I hadn't completed the review yet. It's been sitting in a pending state for the last few days

pdbain-ibm · 2018-12-11T17:36:48Z

@DanHeidinga
Thanks for the feedback. I reverted non-essential changes and renamed the argument for clarity.
Tested on the jtreg testcase.

ChengJin01

The existing implementation of checkClassAccess() is intended to check the defining class but I am not sure whether it is correct or sufficient to check the reference class rather than the defining class.

In addition, such check (for the reference class) might only apply to the situation when the reference class is the same as or a subclass of the defining class but there is no change reflecting the relationship.

ChengJin01 · 2019-01-11T15:56:10Z

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

 						/* The only methods array classes have are defined on Object and thus accessible */
 						return;
 					}

-					if (isSamePackage(accessClass, targetClass)) {
+					if (isSamePackage(accessClass, referenceClass)) {


The API Spec says at https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html

Access Checking ... If the desired member is protected, the usual JVM rules apply, including the requirement that the lookup class must be either be in the same package as the desired member, or must inherit that member. (See the Java Virtual Machine Specification, sections 4.9.2, 5.4.3.5, and 6.4.) In addition, if the desired member is a non-static field or method in a different package, the resulting method handle may only be applied to objects of the lookup class or one of its subclasses.

So changing to referenceClass seems incorrect at this point.

In this case, the method is not public, so we need to check for package access. Consider the case where the lookup class is in the same package as the reference class, but a different package from the defining class. If we check against the defining class we would fail, even though the method is visible via the reference class.

Note also that the reference class may broaden the visibility of the method.

ChengJin01 · 2019-01-11T16:00:38Z

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

@@ -497,7 +506,7 @@ private void checkAccess(Class<?> targetClass, String name, int memberModifiers,
 				}
 			} else {
 				/* default (package access) */
-				if ((PACKAGE == (accessMode & PACKAGE)) && isSamePackage(accessClass, targetClass)){
+				if ((PACKAGE == (accessMode & PACKAGE)) && isSamePackage(accessClass, referenceClass)){


I am not too sure whether it is correct to change to referenceClass if referenceClass is not a subclass of targetClass

referenceClass must be a subtype of definingClass in order to inherit the method.

ChengJin01 · 2019-01-11T16:12:17Z

Introducing the reference class to checkAccess() is logically correct to address the case but how to deal with the relationship between the defining class and the reference class is up to us as the API Spec doesn't explain in details at this point.

pdbain-ibm · 2019-01-11T17:08:45Z

In reply to
#3592 (comment), most of the uses of definingClass pertain to private methods or access via inheritance from the defining class to the lookup class in cases where the lookup class does not extend the reference class.

ChengJin01

I have no more comment as the relationship between the reference class and the defining class is confirmative as explained above at this point.

pdbain-ibm · 2019-01-11T20:53:17Z

Found an issue when testing with a VarHandles test case: the reference class can sometimes be null.
Will update the changeset.

Use the defining class of a class member or the reference class of the lookup as appropriate. Also reinstate missing external message definition. Signed-off-by: Peter Bain <peter_bain@ca.ibm.com>

The reference class may be null in the case of varhandles. Signed-off-by: Peter Bain <peter_bain@ca.ibm.com>

pdbain-ibm · 2019-01-14T14:32:16Z

I added a second commit to fix the null reference class problem. I will squash it when this is approved.

DanHeidinga · 2019-01-14T20:05:30Z

Jenkins test all xlinux jdk11,jdk8

DanHeidinga · 2019-01-14T20:05:59Z

Jenkins test sanity xlinux jdk11

DanHeidinga · 2019-01-14T20:06:16Z

Jenkins test extended xlinux jdk11

pshipton added the comp:vm label Nov 9, 2018

pdbain-ibm changed the title ~~Fix checking method handle access~~ WIP Fix checking method handle access Nov 14, 2018

pdbain-ibm force-pushed the checkaccess branch from 23932af to 91dd131 Compare November 14, 2018 17:27

pdbain-ibm changed the title ~~WIP Fix checking method handle access~~ Fix checking method handle access Nov 14, 2018

pdbain-ibm force-pushed the checkaccess branch from 91dd131 to 513a119 Compare December 4, 2018 14:02

DanHeidinga added this to the Release 0.12.0 (Java 8, 11 Jan refresh) milestone Dec 4, 2018

This was referenced Dec 5, 2018

JTReg test fail : java/lang/invoke/ProtectedMemberDifferentPackage/Test.java #3301

Closed

Update class visibility checking #3499

Open

DanHeidinga reviewed Dec 11, 2018

View reviewed changes

pdbain-ibm force-pushed the checkaccess branch 2 times, most recently from 7894681 to bb0faa6 Compare December 11, 2018 17:33

ChengJin01 suggested changes Jan 11, 2019

View reviewed changes

ChengJin01 reviewed Jan 11, 2019

View reviewed changes

ChengJin01 approved these changes Jan 11, 2019

View reviewed changes

pdbain-ibm force-pushed the checkaccess branch from bb0faa6 to 9a98c29 Compare January 11, 2019 17:59

pdbain-ibm changed the title ~~Fix checking method handle access~~ WIP Fix checking method handle access Jan 11, 2019

Fix checking method handle access

9a05abd

Use the defining class of a class member or the reference class of the lookup as appropriate. Also reinstate missing external message definition. Signed-off-by: Peter Bain <peter_bain@ca.ibm.com>

pdbain-ibm force-pushed the checkaccess branch from 9a98c29 to 617e34c Compare January 14, 2019 14:21

MethodHandles.checkAccess: null check referenceClass

e628544

The reference class may be null in the case of varhandles. Signed-off-by: Peter Bain <peter_bain@ca.ibm.com>

pdbain-ibm force-pushed the checkaccess branch from 617e34c to e628544 Compare January 14, 2019 14:23

pdbain-ibm changed the title ~~WIP Fix checking method handle access~~ Fix checking method handle access Jan 14, 2019

DanHeidinga merged commit 9578b8e into eclipse-openj9:master Jan 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix checking method handle access #3592

Fix checking method handle access #3592

pdbain-ibm commented Nov 7, 2018 •

edited

Loading

pdbain-ibm commented Nov 14, 2018

pdbain-ibm commented Nov 14, 2018

pdbain-ibm commented Nov 27, 2018

pdbain-ibm commented Dec 4, 2018

pdbain-ibm commented Dec 4, 2018

DanHeidinga Dec 6, 2018

DanHeidinga Dec 11, 2018

DanHeidinga commented Dec 11, 2018

pdbain-ibm commented Dec 11, 2018

ChengJin01 left a comment

ChengJin01 Jan 11, 2019 •

edited

Loading

pdbain-ibm Jan 11, 2019

ChengJin01 Jan 11, 2019 •

edited

Loading

pdbain-ibm Jan 11, 2019

ChengJin01 commented Jan 11, 2019 •

edited

Loading

pdbain-ibm commented Jan 11, 2019

ChengJin01 left a comment

pdbain-ibm commented Jan 11, 2019 •

edited

Loading

pdbain-ibm commented Jan 14, 2019

DanHeidinga commented Jan 14, 2019

DanHeidinga commented Jan 14, 2019

DanHeidinga commented Jan 14, 2019

Fix checking method handle access #3592

Fix checking method handle access #3592

Conversation

pdbain-ibm commented Nov 7, 2018 • edited Loading

pdbain-ibm commented Nov 14, 2018

pdbain-ibm commented Nov 14, 2018

pdbain-ibm commented Nov 27, 2018

pdbain-ibm commented Dec 4, 2018

pdbain-ibm commented Dec 4, 2018

DanHeidinga Dec 6, 2018

Choose a reason for hiding this comment

DanHeidinga Dec 11, 2018

Choose a reason for hiding this comment

DanHeidinga commented Dec 11, 2018

pdbain-ibm commented Dec 11, 2018

ChengJin01 left a comment

Choose a reason for hiding this comment

ChengJin01 Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

pdbain-ibm Jan 11, 2019

Choose a reason for hiding this comment

ChengJin01 Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

pdbain-ibm Jan 11, 2019

Choose a reason for hiding this comment

ChengJin01 commented Jan 11, 2019 • edited Loading

pdbain-ibm commented Jan 11, 2019

ChengJin01 left a comment

Choose a reason for hiding this comment

pdbain-ibm commented Jan 11, 2019 • edited Loading

pdbain-ibm commented Jan 14, 2019

DanHeidinga commented Jan 14, 2019

DanHeidinga commented Jan 14, 2019

DanHeidinga commented Jan 14, 2019

pdbain-ibm commented Nov 7, 2018 •

edited

Loading

ChengJin01 Jan 11, 2019 •

edited

Loading

ChengJin01 Jan 11, 2019 •

edited

Loading

ChengJin01 commented Jan 11, 2019 •

edited

Loading

pdbain-ibm commented Jan 11, 2019 •

edited

Loading