You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by su...@apache.org on 2022/08/28 14:49:08 UTC
[groovy] 01/01: Fix accessing `clone` illegally
This is an automated email from the ASF dual-hosted git repository.
sunlan pushed a commit to branch danielsun/fix-clone-array-based-on-groovy10730pr
in repository https://gitbox.apache.org/repos/asf/groovy.git
commit 3493e08878012f9ef795ed1994d35315eae53e96
Author: Daniel Sun <su...@apache.org>
AuthorDate: Sun Aug 28 22:24:38 2022 +0800
Fix accessing `clone` illegally
---
src/main/java/groovy/lang/MetaClassImpl.java | 22 +-----
.../org/codehaus/groovy/runtime/ArrayUtil.java | 30 ++++++++
.../org/codehaus/groovy/runtime/InvokerHelper.java | 4 ++
.../org/codehaus/groovy/vmplugin/v8/Selector.java | 11 +++
src/test/groovy/bugs/Groovy9103.groovy | 9 +++
src/test/groovy/inspect/InspectorTest.java | 14 +++-
.../groovy/transform/ImmutableTransformTest.groovy | 8 +--
.../groovy/text/StreamingTemplateEngineTest.groovy | 80 ++++++++++++----------
8 files changed, 112 insertions(+), 66 deletions(-)
diff --git a/src/main/java/groovy/lang/MetaClassImpl.java b/src/main/java/groovy/lang/MetaClassImpl.java
index c32102989d..14ca8081fb 100644
--- a/src/main/java/groovy/lang/MetaClassImpl.java
+++ b/src/main/java/groovy/lang/MetaClassImpl.java
@@ -1131,7 +1131,7 @@ public class MetaClassImpl implements MetaClass, MutableMetaClass {
if (receiverClass.isArray()) {
if (methodName.equals("clone") && arguments.length == 0) {
try {
- return (Object[]) MethodHandleHolder.CLONE_ARRAY_METHOD_HANDLE.invokeExact((Object[]) object);
+ return (Object[]) ArrayUtil.getCloneArrayMethodHandle().invokeExact((Object[]) object);
} catch (Throwable t) {
throw new GroovyRuntimeException(t);
}
@@ -1164,26 +1164,6 @@ public class MetaClassImpl implements MetaClass, MutableMetaClass {
}
}
- private static class MethodHandleHolder {
- private static final MethodHandle CLONE_ARRAY_METHOD_HANDLE;
- static {
- final Class<ArrayUtil> arrayUtilClass = ArrayUtil.class;
- Method cloneArrayMethod;
- try {
- cloneArrayMethod = arrayUtilClass.getDeclaredMethod("cloneArray", Object[].class);
- } catch (NoSuchMethodException e) {
- throw new GroovyBugError("Failed to find `cloneArray` method in class `" + arrayUtilClass.getName() + "`", e);
- }
-
- try {
- CLONE_ARRAY_METHOD_HANDLE = MethodHandles.lookup().in(arrayUtilClass).unreflect(cloneArrayMethod);
- } catch (IllegalAccessException e) {
- throw new GroovyBugError("Failed to create method handle for " + cloneArrayMethod);
- }
- }
- private MethodHandleHolder() {}
- }
-
private static final ClassValue<Map<String, Set<Method>>> SPECIAL_METHODS_MAP = new ClassValue<Map<String, Set<Method>>>() {
@Override
protected Map<String, Set<Method>> computeValue(final Class<?> type) {
diff --git a/src/main/java/org/codehaus/groovy/runtime/ArrayUtil.java b/src/main/java/org/codehaus/groovy/runtime/ArrayUtil.java
index 72bcd83c51..d35a4d80ea 100644
--- a/src/main/java/org/codehaus/groovy/runtime/ArrayUtil.java
+++ b/src/main/java/org/codehaus/groovy/runtime/ArrayUtil.java
@@ -18,6 +18,12 @@
*/
package org.codehaus.groovy.runtime;
+import org.codehaus.groovy.GroovyBugError;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Method;
+
/**
* This is a generated class used internally during the writing of bytecode within the CallSiteWriter logic.
* This is not a class exposed to users, as is the case with almost all classes in the org.codehaus.groovy packages.
@@ -55,6 +61,30 @@ package org.codehaus.groovy.runtime;
public class ArrayUtil {
private static final Object[] EMPTY = new Object[0];
+ public static MethodHandle getCloneArrayMethodHandle() {
+ return MethodHandleHolder.CLONE_ARRAY_METHOD_HANDLE;
+ }
+
+ private static class MethodHandleHolder {
+ private static final MethodHandle CLONE_ARRAY_METHOD_HANDLE;
+ static {
+ final Class<ArrayUtil> arrayUtilClass = ArrayUtil.class;
+ Method cloneArrayMethod;
+ try {
+ cloneArrayMethod = arrayUtilClass.getDeclaredMethod("cloneArray", Object[].class);
+ } catch (NoSuchMethodException e) {
+ throw new GroovyBugError("Failed to find `cloneArray` method in class `" + arrayUtilClass.getName() + "`", e);
+ }
+
+ try {
+ CLONE_ARRAY_METHOD_HANDLE = MethodHandles.lookup().in(arrayUtilClass).unreflect(cloneArrayMethod);
+ } catch (IllegalAccessException e) {
+ throw new GroovyBugError("Failed to create method handle for " + cloneArrayMethod);
+ }
+ }
+ private MethodHandleHolder() {}
+ }
+
public static <T> T[] cloneArray(T[] array) {
return array.clone();
}
diff --git a/src/main/java/org/codehaus/groovy/runtime/InvokerHelper.java b/src/main/java/org/codehaus/groovy/runtime/InvokerHelper.java
index 66926668b3..e39de8d48c 100644
--- a/src/main/java/org/codehaus/groovy/runtime/InvokerHelper.java
+++ b/src/main/java/org/codehaus/groovy/runtime/InvokerHelper.java
@@ -586,6 +586,10 @@ public class InvokerHelper {
// it's an instance; check if it's a Java one
if (!(object instanceof GroovyObject)) {
+ if (null != object && object.getClass().isArray() && "clone".equals(methodName) && (null == arguments || arguments.getClass().isArray() && 0 == ((Object[]) arguments).length)) {
+ return ArrayUtil.cloneArray((Object[]) object);
+ }
+
return invokePojoMethod(object, methodName, arguments);
}
diff --git a/src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java b/src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java
index 2352a1c21b..c5d75a4968 100644
--- a/src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java
+++ b/src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java
@@ -38,6 +38,7 @@ import org.codehaus.groovy.reflection.CachedMethod;
import org.codehaus.groovy.reflection.ClassInfo;
import org.codehaus.groovy.reflection.GeneratedMetaMethod;
import org.codehaus.groovy.reflection.stdclasses.CachedSAMClass;
+import org.codehaus.groovy.runtime.ArrayUtil;
import org.codehaus.groovy.runtime.GeneratedClosure;
import org.codehaus.groovy.runtime.GroovyCategorySupport;
import org.codehaus.groovy.runtime.GroovyCategorySupport.CategoryMethod;
@@ -673,9 +674,19 @@ public abstract class Selector {
}
}
+ private static final Method OBJECT_CLONE_METHOD;
+ static {
+ try {
+ OBJECT_CLONE_METHOD = Object.class.getDeclaredMethod("clone");
+ } catch (NoSuchMethodException e) {
+ throw new GroovyBugError(e); // should never happen
+ }
+ }
private MethodHandle correctClassForNameAndUnReflectOtherwise(Method m) throws IllegalAccessException {
if (m.getDeclaringClass() == Class.class && m.getName().equals("forName") && m.getParameterTypes().length == 1) {
return MethodHandles.insertArguments(CLASS_FOR_NAME, 1, true, sender.getClassLoader());
+ } else if (null != args && args.getClass().isArray() && OBJECT_CLONE_METHOD.equals(m)) {
+ return ArrayUtil.getCloneArrayMethodHandle();
} else {
return LOOKUP.unreflect(m);
}
diff --git a/src/test/groovy/bugs/Groovy9103.groovy b/src/test/groovy/bugs/Groovy9103.groovy
index ab1be1a0c5..721344f0c6 100644
--- a/src/test/groovy/bugs/Groovy9103.groovy
+++ b/src/test/groovy/bugs/Groovy9103.groovy
@@ -69,4 +69,13 @@ final class Groovy9103 {
Object obj = new Tuple1('abc')
assert obj.clone()
}
+
+ @Test
+ void testClone4() {
+ assertScript '''
+ int[] nums = [1, 2, 3]
+ int[] cloned = nums.clone()
+ assert nums == cloned
+ '''
+ }
}
diff --git a/src/test/groovy/inspect/InspectorTest.java b/src/test/groovy/inspect/InspectorTest.java
index 0bcc38530e..cc789e2117 100644
--- a/src/test/groovy/inspect/InspectorTest.java
+++ b/src/test/groovy/inspect/InspectorTest.java
@@ -21,7 +21,6 @@ package groovy.inspect;
import groovy.lang.GroovyShell;
import groovy.lang.MetaProperty;
import groovy.lang.PropertyValue;
-import org.codehaus.groovy.runtime.FormatHelper;
import org.jmock.Mock;
import org.jmock.cglib.MockObjectTestCase;
@@ -29,10 +28,19 @@ import java.io.ByteArrayOutputStream;
import java.io.PrintStream;
import java.io.Serializable;
import java.lang.reflect.Field;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Comparator;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Set;
+import java.util.TreeSet;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
+import static groovy.test.GroovyAssert.isAtLeastJdk;
+
public class InspectorTest extends MockObjectTestCase implements Serializable {
public String someField = "only for testing";
public static final String SOME_CONST = "only for testing";
@@ -139,6 +147,8 @@ public class InspectorTest extends MockObjectTestCase implements Serializable {
// TODO: if our code can never access inspect in this way, it would be better
// to move this to a boundary class and then we wouldn't need this test
public void testInspectUninspectableProperty() {
+ if (isAtLeastJdk("16.0")) return;
+
Object dummyInstance = new Object();
Inspector inspector = getTestableInspector(dummyInstance);
Class[] paramTypes = {Object.class, MetaProperty.class};
diff --git a/src/test/org/codehaus/groovy/transform/ImmutableTransformTest.groovy b/src/test/org/codehaus/groovy/transform/ImmutableTransformTest.groovy
index f108715511..2095952132 100644
--- a/src/test/org/codehaus/groovy/transform/ImmutableTransformTest.groovy
+++ b/src/test/org/codehaus/groovy/transform/ImmutableTransformTest.groovy
@@ -139,8 +139,7 @@ class ImmutableTransformTest extends GroovyShellTestCase {
@Test
void testCloneableFieldNotCloneableObject() {
- def cls = shouldFail(CloneNotSupportedException) {
- def objects = evaluate("""
+ shouldFail(isAtLeastJdk('16.0') ? IllegalAccessException : CloneNotSupportedException, '''
import groovy.transform.Immutable
class Dolly {
@@ -154,10 +153,7 @@ class ImmutableTransformTest extends GroovyShellTestCase {
def dolly = new Dolly(name: "The Sheep")
[dolly, new Lab(name: "Area 51", clone: dolly)]
- """)
- }
-
- assert cls == 'Dolly'
+ ''')
}
@Test
diff --git a/subprojects/groovy-templates/src/test/groovy/groovy/text/StreamingTemplateEngineTest.groovy b/subprojects/groovy-templates/src/test/groovy/groovy/text/StreamingTemplateEngineTest.groovy
index 78b0ed0b47..94e1273ac4 100644
--- a/subprojects/groovy-templates/src/test/groovy/groovy/text/StreamingTemplateEngineTest.groovy
+++ b/subprojects/groovy-templates/src/test/groovy/groovy/text/StreamingTemplateEngineTest.groovy
@@ -23,6 +23,8 @@ import org.junit.Test
import java.util.concurrent.ConcurrentHashMap
+import static groovy.test.GroovyAssert.assertScript
+
class StreamingTemplateEngineTest {
TemplateEngine engine
Map binding
@@ -463,48 +465,52 @@ class StreamingTemplateEngineTest {
@Test
void reuseClassLoader1() {
- final reuseOption = 'groovy.StreamingTemplateEngine.reuseClassLoader'
- System.setProperty(reuseOption, 'true')
+ assertScript '''
+ final reuseOption = 'groovy.StreamingTemplateEngine.reuseClassLoader'
+ System.setProperty(reuseOption, 'true')
- try {
- // reload class to initialize static field from the beginning
- def steClass = reloadClass('groovy.text.StreamingTemplateEngine')
-
- GroovyClassLoader gcl = new GroovyClassLoader()
- def engine = steClass.newInstance(gcl)
- assert 'Hello, Daniel' == engine.createTemplate('Hello, ${name}').make([name: 'Daniel']).toString()
- assert gcl.loadedClasses.length > 0
- def cloned = gcl.loadedClasses.clone()
- assert 'Hello, Paul' == engine.createTemplate('Hello, ${name}').make([name: 'Paul']).toString()
- assert cloned == gcl.loadedClasses
- } finally {
- System.clearProperty(reuseOption)
- }
+ try {
+ // reload class to initialize static field from the beginning
+ def steClass = groovy.text.StreamingTemplateEngineTest.reloadClass('groovy.text.StreamingTemplateEngine')
+
+ GroovyClassLoader gcl = new GroovyClassLoader()
+ def engine = steClass.newInstance(gcl)
+ assert 'Hello, Daniel' == engine.createTemplate('Hello, ${name}').make([name: 'Daniel']).toString()
+ assert gcl.loadedClasses.length > 0
+ def cloned = gcl.loadedClasses.clone()
+ assert 'Hello, Paul' == engine.createTemplate('Hello, ${name}').make([name: 'Paul']).toString()
+ assert cloned == gcl.loadedClasses
+ } finally {
+ System.clearProperty(reuseOption)
+ }
+ '''
}
@Test
void reuseClassLoader2() {
- final reuseOption = 'groovy.StreamingTemplateEngine.reuseClassLoader'
- System.setProperty(reuseOption, 'true')
-
- try {
- // reload class to initialize static field from the beginning
- def steClass = reloadClass('groovy.text.StreamingTemplateEngine')
-
- GroovyClassLoader gcl = new GroovyClassLoader()
- def engine = steClass.newInstance(gcl)
- assert 'Hello, Daniel' == engine.createTemplate('Hello, ${name}').make([name: 'Daniel']).toString()
- assert gcl.loadedClasses.length > 0
- def cloned = gcl.loadedClasses.clone()
- engine = steClass.newInstance(gcl)
- assert 'Hello, Paul' == engine.createTemplate('Hello, ${name}').make([name: 'Paul']).toString()
- assert cloned == gcl.loadedClasses
- } finally {
- System.clearProperty(reuseOption)
- }
- }
-
- private static Class reloadClass(String className) {
+ assertScript '''
+ final reuseOption = 'groovy.StreamingTemplateEngine.reuseClassLoader'
+ System.setProperty(reuseOption, 'true')
+
+ try {
+ // reload class to initialize static field from the beginning
+ def steClass = groovy.text.StreamingTemplateEngineTest.reloadClass('groovy.text.StreamingTemplateEngine')
+
+ GroovyClassLoader gcl = new GroovyClassLoader()
+ def engine = steClass.newInstance(gcl)
+ assert 'Hello, Daniel' == engine.createTemplate('Hello, ${name}').make([name: 'Daniel']).toString()
+ assert gcl.loadedClasses.length > 0
+ def cloned = gcl.loadedClasses.clone()
+ engine = steClass.newInstance(gcl)
+ assert 'Hello, Paul' == engine.createTemplate('Hello, ${name}').make([name: 'Paul']).toString()
+ assert cloned == gcl.loadedClasses
+ } finally {
+ System.clearProperty(reuseOption)
+ }
+ '''
+ }
+
+ static Class reloadClass(String className) {
def clazz =
new GroovyClassLoader() {
private final Map<String, Class> loadedClasses = new ConcurrentHashMap<String, Class>()