This is an automated email from the ASF dual-hosted git repository. sunlan pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/groovy.git
commit 6b0c41604d1e7f1f0156402b20ec8caa497f5789 Author: Daniel Sun <[email protected]> AuthorDate: Mon May 27 22:07:57 2019 +0800 GROOVY-9103: CLONE - CLONE - Fix warning "An illegal reflective access operation has occurred" --- src/main/java/groovy/lang/MetaClassImpl.java | 30 ++++++++++- .../codehaus/groovy/reflection/CachedMethod.java | 4 ++ .../groovy/reflection/ReflectionUtils.java | 7 ++- .../runtime/callsite/PogoMetaMethodSite.java | 7 +-- .../org/codehaus/groovy/vmplugin/VMPlugin.java | 10 ++++ .../org/codehaus/groovy/vmplugin/v5/Java5.java | 5 ++ .../org/codehaus/groovy/vmplugin/v9/Java9.java | 36 +++++++++++-- src/test/groovy/bugs/Groovy9103Bug.groovy | 62 ++++++++++++++++++++++ 8 files changed, 147 insertions(+), 14 deletions(-) diff --git a/src/main/java/groovy/lang/MetaClassImpl.java b/src/main/java/groovy/lang/MetaClassImpl.java index 171bf7c..8516472 100644 --- a/src/main/java/groovy/lang/MetaClassImpl.java +++ b/src/main/java/groovy/lang/MetaClassImpl.java @@ -20,6 +20,7 @@ package groovy.lang; import org.apache.groovy.internal.util.UncheckedThrow; import org.apache.groovy.util.BeanUtils; +import org.apache.groovy.util.SystemUtil; import org.codehaus.groovy.GroovyBugError; import org.codehaus.groovy.ast.ClassNode; import org.codehaus.groovy.classgen.asm.BytecodeHelper; @@ -2192,10 +2193,12 @@ public class MetaClassImpl implements MetaClass, MutableMetaClass { MetaBeanProperty mp = (MetaBeanProperty) element; boolean setter = true; boolean getter = true; - if (mp.getGetter() == null || mp.getGetter() instanceof GeneratedMetaMethod || mp.getGetter() instanceof NewInstanceMetaMethod) { + MetaMethod getterMetaMethod = mp.getGetter(); + if (getterMetaMethod == null || getterMetaMethod instanceof GeneratedMetaMethod || getterMetaMethod instanceof NewInstanceMetaMethod) { getter = false; } - if (mp.getSetter() == null || mp.getSetter() instanceof GeneratedMetaMethod || mp.getSetter() instanceof NewInstanceMetaMethod) { + MetaMethod setterMetaMethod = mp.getSetter(); + if (setterMetaMethod == null || setterMetaMethod instanceof GeneratedMetaMethod || setterMetaMethod instanceof NewInstanceMetaMethod) { setter = false; } if (!setter && !getter) continue; @@ -2207,11 +2210,34 @@ public class MetaClassImpl implements MetaClass, MutableMetaClass { // element = new MetaBeanProperty(mp.getName(), mp.getType(), null, mp.getSetter()); // } } + + if (!ALLOW_ILLEGAL_ACCESS_PROPERTIES) { + if (element instanceof MetaBeanProperty) { + MetaBeanProperty mbp = (MetaBeanProperty) element; + boolean getterAccessible = canAccessLegally(mbp.getGetter()); + boolean setterAccessible = canAccessLegally(mbp.getSetter()); + + if (!(getterAccessible && setterAccessible)) continue; + } + } + ret.add(element); } return ret; } + private static final boolean ALLOW_ILLEGAL_ACCESS_PROPERTIES = SystemUtil.getBooleanSafe("groovy.allow.illegal.access.properties"); + + private static boolean canAccessLegally(MetaMethod accessor) { + boolean accessible = true; + if (accessor instanceof CachedMethod) { + CachedMethod cm = (CachedMethod) accessor; + accessible = cm.canAccessLegally(MetaClassImpl.class); + } + + return accessible; + } + /** * return null if nothing valid has been found, a MetaMethod (for getter always the case if not null) or * a LinkedList<MetaMethod> if there are multiple setter diff --git a/src/main/java/org/codehaus/groovy/reflection/CachedMethod.java b/src/main/java/org/codehaus/groovy/reflection/CachedMethod.java index 4184ffc..fb342bf 100644 --- a/src/main/java/org/codehaus/groovy/reflection/CachedMethod.java +++ b/src/main/java/org/codehaus/groovy/reflection/CachedMethod.java @@ -370,6 +370,10 @@ public class CachedMethod extends MetaMethod implements Comparable { return cachedMethod; } + public boolean canAccessLegally(Class<?> callerClass) { + return ReflectionUtils.checkAccessible(callerClass, cachedMethod.getDeclaringClass(), cachedMethod.getModifiers(), false); + } + private boolean makeAccessibleDone = false; private void makeAccessibleIfNecessary() { if (!makeAccessibleDone) { diff --git a/src/main/java/org/codehaus/groovy/reflection/ReflectionUtils.java b/src/main/java/org/codehaus/groovy/reflection/ReflectionUtils.java index c6bf9f4..f530129 100644 --- a/src/main/java/org/codehaus/groovy/reflection/ReflectionUtils.java +++ b/src/main/java/org/codehaus/groovy/reflection/ReflectionUtils.java @@ -165,11 +165,14 @@ public class ReflectionUtils { return methodList; } - public static boolean checkCanSetAccessible(AccessibleObject accessibleObject, - Class<?> caller) { + public static boolean checkCanSetAccessible(AccessibleObject accessibleObject, Class<?> caller) { return VM_PLUGIN.checkCanSetAccessible(accessibleObject, caller); } + public static boolean checkAccessible(Class<?> callerClass, Class<?> declaringClass, int memberModifiers, boolean allowIllegalAccess) { + return VM_PLUGIN.checkAccessible(callerClass, declaringClass, memberModifiers, allowIllegalAccess); + } + public static boolean trySetAccessible(AccessibleObject ao) { try { return VM_PLUGIN.trySetAccessible(ao); diff --git a/src/main/java/org/codehaus/groovy/runtime/callsite/PogoMetaMethodSite.java b/src/main/java/org/codehaus/groovy/runtime/callsite/PogoMetaMethodSite.java index 927d2a7..49a2da0 100644 --- a/src/main/java/org/codehaus/groovy/runtime/callsite/PogoMetaMethodSite.java +++ b/src/main/java/org/codehaus/groovy/runtime/callsite/PogoMetaMethodSite.java @@ -157,11 +157,8 @@ public class PogoMetaMethodSite extends PlainObjectMetaMethodSite { final Method reflect; public PogoCachedMethodSite(CallSite site, MetaClassImpl metaClass, CachedMethod metaMethod, Class[] params) { - super(site, metaClass, metaMethod, params); - reflect = metaMethod.setAccessible(); - -// super(site, metaClass, CallSiteHelper.transformMetaMethod(metaClass, metaMethod, params, site), params); -// reflect = ((CachedMethod) super.metaMethod).setAccessible(); + super(site, metaClass, CallSiteHelper.transformMetaMethod(metaClass, metaMethod, params, site.getArray().owner), params); + reflect = ((CachedMethod) super.metaMethod).setAccessible(); } public Object invoke(Object receiver, Object[] args) throws Throwable { diff --git a/src/main/java/org/codehaus/groovy/vmplugin/VMPlugin.java b/src/main/java/org/codehaus/groovy/vmplugin/VMPlugin.java index 103d185..c0cd3bd 100644 --- a/src/main/java/org/codehaus/groovy/vmplugin/VMPlugin.java +++ b/src/main/java/org/codehaus/groovy/vmplugin/VMPlugin.java @@ -73,6 +73,16 @@ public interface VMPlugin { boolean checkCanSetAccessible(AccessibleObject accessibleObject, Class<?> callerClass); /** + * check whether the member can be accessed or not + * @param callerClass callerClass the callerClass to invoke {@code setAccessible} + * @param declaringClass the type of member owner + * @param memberModifiers modifiers of member + * @param allowIllegalAccess whether to allow illegal access + * @return the result of checking + */ + boolean checkAccessible(Class<?> callerClass, Class<?> declaringClass, int memberModifiers, boolean allowIllegalAccess); + + /** * Set the {@code accessible} flag for this reflected object to {@code true} * if possible. * diff --git a/src/main/java/org/codehaus/groovy/vmplugin/v5/Java5.java b/src/main/java/org/codehaus/groovy/vmplugin/v5/Java5.java index 56dd1e4..2b48db4 100644 --- a/src/main/java/org/codehaus/groovy/vmplugin/v5/Java5.java +++ b/src/main/java/org/codehaus/groovy/vmplugin/v5/Java5.java @@ -572,6 +572,11 @@ public class Java5 implements VMPlugin { } @Override + public boolean checkAccessible(Class<?> callerClass, Class<?> declaringClass, int memberModifiers, boolean allowIllegalAccess) { + return true; + } + + @Override public boolean trySetAccessible(AccessibleObject ao) { try { ao.setAccessible(true); diff --git a/src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java b/src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java index 0fa4b42..39c507b 100644 --- a/src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java +++ b/src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java @@ -42,6 +42,7 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Member; import java.lang.reflect.Method; import java.lang.reflect.Modifier; +import java.math.BigInteger; import java.util.Arrays; import java.util.HashMap; import java.util.HashSet; @@ -192,6 +193,11 @@ public class Java9 extends Java8 { } if (declaringClass == theClass) { + MetaMethod bigIntegerMetaMethod = transformBigIntegerMetaMethod(metaMethod, params, theClass); + if (bigIntegerMetaMethod != metaMethod) { + return bigIntegerMetaMethod; + } + // GROOVY-9081 "3) Access public members of private class", e.g. Collections.unmodifiableMap([:]).toString() // try to find the visible method from its superclasses List<Class<?>> superclassList = findSuperclasses(theClass); @@ -224,7 +230,24 @@ public class Java9 extends Java8 { return metaMethod; } - private static Optional<MetaMethod> getAccessibleMetaMethod(MetaMethod metaMethod, Class<?>[] params, Class<?> caller, Class<?> sc) { + private static MetaMethod transformBigIntegerMetaMethod(MetaMethod metaMethod, Class<?>[] params, Class<?> theClass) { + if (BigInteger.class != theClass) { + return metaMethod; + } + + if (MULTIPLY.equals(metaMethod.getName()) + && (1 == params.length && (Integer.class == params[0] || Long.class == params[0] || Short.class == params[0]))) { + try { + return new CachedMethod(BigInteger.class.getDeclaredMethod(MULTIPLY, BigInteger.class)); + } catch (NoSuchMethodException e) { + throw new GroovyBugError("Failed to transform " + MULTIPLY + " method of BigInteger", e); + } + } + + return metaMethod; + } + + private Optional<MetaMethod> getAccessibleMetaMethod(MetaMethod metaMethod, Class<?>[] params, Class<?> caller, Class<?> sc) { List<MetaMethod> metaMethodList = getMetaMethods(metaMethod, params, sc); for (MetaMethod mm : metaMethodList) { if (checkAccessible(caller, mm.getDeclaringClass().getTheClass(), mm.getModifiers(), false)) { @@ -239,7 +262,8 @@ public class Java9 extends Java8 { return optionalMethod.stream().map(CachedMethod::new).collect(Collectors.toList()); } - private static boolean checkAccessible(Class<?> callerClass, Class<?> declaringClass, int modifiers, boolean allowIllegalAccess) { + @Override + public boolean checkAccessible(Class<?> callerClass, Class<?> declaringClass, int memberModifiers, boolean allowIllegalAccess) { Module callerModule = callerClass.getModule(); Module declaringModule = declaringClass.getModule(); String pn = declaringClass.getPackageName(); @@ -251,13 +275,13 @@ public class Java9 extends Java8 { boolean isClassPublic = Modifier.isPublic(declaringClass.getModifiers()); if (isClassPublic && declaringModule.isExported(pn, callerModule)) { // member is public - if (Modifier.isPublic(modifiers)) { + if (Modifier.isPublic(memberModifiers)) { return !(toCheckIllegalAccess && isExportedForIllegalAccess(declaringModule, pn)); } // member is protected-static - if (Modifier.isProtected(modifiers) - && Modifier.isStatic(modifiers) + if (Modifier.isProtected(memberModifiers) + && Modifier.isStatic(memberModifiers) && isSubclassOf(callerClass, declaringClass)) { return !(toCheckIllegalAccess && isExportedForIllegalAccess(declaringModule, pn)); } @@ -358,6 +382,8 @@ public class Java9 extends Java8 { .anyMatch(e -> e.source().equals(pn) && !e.isQualified()); } + private static final String MULTIPLY = "multiply"; + private static String[] JAVA8_PACKAGES() { return new String[] { "apple.applescript", diff --git a/src/test/groovy/bugs/Groovy9103Bug.groovy b/src/test/groovy/bugs/Groovy9103Bug.groovy new file mode 100644 index 0000000..322bd36 --- /dev/null +++ b/src/test/groovy/bugs/Groovy9103Bug.groovy @@ -0,0 +1,62 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package groovy.bugs + +// TODO add JVM option `--illegal-access=deny` when all warnings fixed +class Groovy9103Bug extends GroovyTestCase { + void testProperties() { + String str = '' + assert str.properties + } + + void testBigIntegerMultiply() { + assert 2G * 1 + } + + void testClone() { + assertScript ''' + def broadcastSeq(Object value) { + value.clone() + } + + assert broadcastSeq(new Tuple1('abc')) + ''' + } + + void testClone2() { + assertScript ''' + class Value { + @Override + public Value clone() { + return new Value() + } + } + def broadcastSeq(Object value) { + value.clone() + } + + assert broadcastSeq(new Value()) + ''' + } + + void testClone3() { + Object obj = new Tuple1('abc') + assert obj.clone() + } +}
