Painless: Fix Bug with Duplicate PainlessClasses (#32110)

jdconrad · web-flow · commit 1c63eb108164 · 2018-07-17T10:33:38.000-07:00
When building the PainlessMethods and PainlessFields they stored a reference to a 
PainlessClass. This reference was prior to "freezing" the PainlessClass so the data was 
both incomplete and mutable. This has been replaced with a target java class instead 
since the PainlessClass is accessible through a java class now and it requires no special 
modifications to get around a chicken and egg issue.
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Def.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Def.java
@@ -334,8 +334,8 @@ static MethodHandle lookupReference(PainlessLookup painlessLookup, MethodHandles
          }
          int arity = interfaceMethod.arguments.size();
          PainlessMethod implMethod = lookupMethodInternal(painlessLookup, receiverClass, name, arity);
-        return lookupReferenceInternal(painlessLookup, methodHandlesLookup, interfaceType, implMethod.owner.name,
-                implMethod.name, receiverClass);
+        return lookupReferenceInternal(painlessLookup, methodHandlesLookup, interfaceType,
+                PainlessLookupUtility.anyTypeToPainlessTypeName(implMethod.target), implMethod.name, receiverClass);
      }
 
      /** Returns a method handle to an implementation of clazz, given method reference signature. */
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/FunctionRef.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/FunctionRef.java
@@ -102,22 +102,22 @@ public FunctionRef(Class<?> expected, PainlessMethod interfaceMethod, PainlessMe
         interfaceMethodType = interfaceMethod.getMethodType().dropParameterTypes(0, 1);
 
         // the Painless$Script class can be inferred if owner is null
-        if (delegateMethod.owner == null) {
+        if (delegateMethod.target == null) {
             delegateClassName = CLASS_NAME;
             isDelegateInterface = false;
         } else if (delegateMethod.augmentation != null) {
             delegateClassName = delegateMethod.augmentation.getName();
             isDelegateInterface = delegateMethod.augmentation.isInterface();
         } else {
-            delegateClassName = delegateMethod.owner.clazz.getName();
-            isDelegateInterface = delegateMethod.owner.clazz.isInterface();
+            delegateClassName = delegateMethod.target.getName();
+            isDelegateInterface = delegateMethod.target.isInterface();
         }
 
         if ("<init>".equals(delegateMethod.name)) {
             delegateInvokeType = H_NEWINVOKESPECIAL;
         } else if (Modifier.isStatic(delegateMethod.modifiers)) {
             delegateInvokeType = H_INVOKESTATIC;
-        } else if (delegateMethod.owner.clazz.isInterface()) {
+        } else if (delegateMethod.target.isInterface()) {
             delegateInvokeType = H_INVOKEINTERFACE;
         } else {
             delegateInvokeType = H_INVOKEVIRTUAL;
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessField.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessField.java
@@ -23,18 +23,18 @@
 
 public final class PainlessField {
     public final String name;
-    public final PainlessClass owner;
+    public final Class<?> target;
     public final Class<?> clazz;
     public final String javaName;
     public final int modifiers;
     public final MethodHandle getter;
     public final MethodHandle setter;
 
-    PainlessField(String name, String javaName, PainlessClass owner, Class<?> clazz, int modifiers,
+    PainlessField(String name, String javaName, Class<?> target, Class<?> clazz, int modifiers,
                   MethodHandle getter, MethodHandle setter) {
         this.name = name;
         this.javaName = javaName;
-        this.owner = owner;
+        this.target = target;
         this.clazz = clazz;
         this.modifiers = modifiers;
         this.getter = getter;
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookupBuilder.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookupBuilder.java
@@ -310,7 +310,7 @@ private void addConstructor(String ownerStructName, WhitelistConstructor whiteli
             }
 
             painlessConstructor = methodCache.computeIfAbsent(buildMethodCacheKey(ownerStruct.name, "<init>", painlessParametersTypes),
-                key -> new PainlessMethod("<init>", ownerStruct, null, void.class, painlessParametersTypes,
+                key -> new PainlessMethod("<init>", ownerStruct.clazz, null, void.class, painlessParametersTypes,
                     asmConstructor, javaConstructor.getModifiers(), javaHandle));
             ownerStruct.constructors.put(painlessMethodKey, painlessConstructor);
         } else if (painlessConstructor.arguments.equals(painlessParametersTypes) == false){
@@ -419,7 +419,7 @@ private void addMethod(ClassLoader whitelistClassLoader, String ownerStructName,
 
                 painlessMethod = methodCache.computeIfAbsent(
                     buildMethodCacheKey(ownerStruct.name, whitelistMethod.javaMethodName, painlessParametersTypes),
-                    key -> new PainlessMethod(whitelistMethod.javaMethodName, ownerStruct, null, painlessReturnClass,
+                    key -> new PainlessMethod(whitelistMethod.javaMethodName, ownerStruct.clazz, null, painlessReturnClass,
                         painlessParametersTypes, asmMethod, javaMethod.getModifiers(), javaMethodHandle));
                 ownerStruct.staticMethods.put(painlessMethodKey, painlessMethod);
             } else if ((painlessMethod.name.equals(whitelistMethod.javaMethodName) && painlessMethod.rtn == painlessReturnClass &&
@@ -445,7 +445,7 @@ private void addMethod(ClassLoader whitelistClassLoader, String ownerStructName,
 
                 painlessMethod = methodCache.computeIfAbsent(
                     buildMethodCacheKey(ownerStruct.name, whitelistMethod.javaMethodName, painlessParametersTypes),
-                    key -> new PainlessMethod(whitelistMethod.javaMethodName, ownerStruct, javaAugmentedClass, painlessReturnClass,
+                    key -> new PainlessMethod(whitelistMethod.javaMethodName, ownerStruct.clazz, javaAugmentedClass, painlessReturnClass,
                         painlessParametersTypes, asmMethod, javaMethod.getModifiers(), javaMethodHandle));
                 ownerStruct.methods.put(painlessMethodKey, painlessMethod);
             } else if ((painlessMethod.name.equals(whitelistMethod.javaMethodName) && painlessMethod.rtn.equals(painlessReturnClass) &&
@@ -501,7 +501,7 @@ private void addField(String ownerStructName, WhitelistField whitelistField) {
                 painlessField = fieldCache.computeIfAbsent(
                     buildFieldCacheKey(ownerStruct.name, whitelistField.javaFieldName, painlessFieldClass.getName()),
                     key -> new PainlessField(whitelistField.javaFieldName, javaField.getName(),
-                        ownerStruct, painlessFieldClass, javaField.getModifiers(), null, null));
+                        ownerStruct.clazz, painlessFieldClass, javaField.getModifiers(), null, null));
                 ownerStruct.staticMembers.put(whitelistField.javaFieldName, painlessField);
             } else if (painlessField.clazz != painlessFieldClass) {
                 throw new IllegalArgumentException("illegal duplicate static fields [" + whitelistField.javaFieldName + "] " +
@@ -530,7 +530,7 @@ private void addField(String ownerStructName, WhitelistField whitelistField) {
                 painlessField = fieldCache.computeIfAbsent(
                     buildFieldCacheKey(ownerStruct.name, whitelistField.javaFieldName, painlessFieldClass.getName()),
                     key -> new PainlessField(whitelistField.javaFieldName, javaField.getName(),
-                        ownerStruct, painlessFieldClass, javaField.getModifiers(), javaMethodHandleGetter, javaMethodHandleSetter));
+                        ownerStruct.clazz, painlessFieldClass, javaField.getModifiers(), javaMethodHandleGetter, javaMethodHandleSetter));
                 ownerStruct.members.put(whitelistField.javaFieldName, painlessField);
             } else if (painlessField.clazz != painlessFieldClass) {
                 throw new IllegalArgumentException("illegal duplicate member fields [" + whitelistField.javaFieldName + "] " +
@@ -615,8 +615,8 @@ private void copyStruct(String struct, List<String> children) {
 
             for (PainlessField field : child.members.values()) {
                 if (owner.members.get(field.name) == null) {
-                    owner.members.put(field.name,
-                        new PainlessField(field.name, field.javaName, owner, field.clazz, field.modifiers, field.getter, field.setter));
+                    owner.members.put(field.name, new PainlessField(
+                            field.name, field.javaName, owner.clazz, field.clazz, field.modifiers, field.getter, field.setter));
                 }
             }
         }
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookupUtility.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookupUtility.java
@@ -158,7 +158,7 @@ public static Class<?> painlessTypeNameToPainlessType(String painlessTypeName, M
                     painlessTypeName.charAt(arrayIndex++) == ']') {
                     ++arrayDimensions;
                 } else {
-                    throw new IllegalArgumentException("invalid painless type [" + painlessTypeName + "].");
+                    throw new IllegalArgumentException("painless type [" + painlessTypeName + "] not found");
                 }
             }
 
@@ -192,7 +192,7 @@ public static Class<?> painlessTypeNameToPainlessType(String painlessTypeName, M
             try {
                 return Class.forName(javaDescriptor);
             } catch (ClassNotFoundException cnfe) {
-                throw new IllegalStateException("painless type [" + painlessTypeName + "] not found", cnfe);
+                throw new IllegalArgumentException("painless type [" + painlessTypeName + "] not found", cnfe);
             }
         }
 
@@ -207,7 +207,7 @@ public static void validatePainlessType(Class<?> painlessType, Collection<Class<
         }
 
         if (javaClasses.contains(painlessType) == false) {
-            throw new IllegalStateException("painless type [" + painlessTypeName + "] not found");
+            throw new IllegalArgumentException("painless type [" + painlessTypeName + "] not found");
         }
     }
 
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessMethod.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessMethod.java
@@ -21,6 +21,7 @@
 
 import org.elasticsearch.painless.MethodWriter;
 import org.objectweb.asm.Opcodes;
+import org.objectweb.asm.Type;
 
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodType;
@@ -30,19 +31,19 @@
 
 public class PainlessMethod {
     public final String name;
-    public final PainlessClass owner;
+    public final Class<?> target;
     public final Class<?> augmentation;
     public final Class<?> rtn;
     public final List<Class<?>> arguments;
     public final org.objectweb.asm.commons.Method method;
     public final int modifiers;
     public final MethodHandle handle;
 
-    public PainlessMethod(String name, PainlessClass owner, Class<?> augmentation, Class<?> rtn, List<Class<?>> arguments,
+    public PainlessMethod(String name, Class<?> target, Class<?> augmentation, Class<?> rtn, List<Class<?>> arguments,
                           org.objectweb.asm.commons.Method method, int modifiers, MethodHandle handle) {
         this.name = name;
         this.augmentation = augmentation;
-        this.owner = owner;
+        this.target = target;
         this.rtn = rtn;
         this.arguments = Collections.unmodifiableList(arguments);
         this.method = method;
@@ -85,11 +86,11 @@ public MethodType getMethodType() {
             for (int i = 0; i < arguments.size(); i++) {
                 params[i] = PainlessLookupUtility.painlessDefTypeToJavaObjectType(arguments.get(i));
             }
-            returnValue = owner.clazz;
+            returnValue = target;
         } else {
             // virtual/interface method: add receiver class
             params = new Class<?>[1 + arguments.size()];
-            params[0] = owner.clazz;
+            params[0] = target;
             for (int i = 0; i < arguments.size(); i++) {
                 params[i + 1] = PainlessLookupUtility.painlessDefTypeToJavaObjectType(arguments.get(i));
             }
@@ -106,8 +107,8 @@ public void write(MethodWriter writer) {
             clazz = augmentation;
             type = org.objectweb.asm.Type.getType(augmentation);
         } else {
-            clazz = owner.clazz;
-            type = owner.type;
+            clazz = target;
+            type = Type.getType(target);
         }
 
         if (Modifier.isStatic(modifiers)) {
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EListInit.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EListInit.java
@@ -26,6 +26,7 @@
 import org.elasticsearch.painless.lookup.PainlessMethod;
 import org.elasticsearch.painless.lookup.PainlessMethodKey;
 import org.elasticsearch.painless.lookup.def;
+import org.objectweb.asm.Type;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -90,7 +91,7 @@ void write(MethodWriter writer, Globals globals) {
 
         writer.newInstance(MethodWriter.getType(actual));
         writer.dup();
-        writer.invokeConstructor(constructor.owner.type, constructor.method);
+        writer.invokeConstructor(Type.getType(constructor.target), constructor.method);
 
         for (AExpression value : values) {
             writer.dup();
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EMapInit.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EMapInit.java
@@ -26,6 +26,7 @@
 import org.elasticsearch.painless.lookup.PainlessMethod;
 import org.elasticsearch.painless.lookup.PainlessMethodKey;
 import org.elasticsearch.painless.lookup.def;
+import org.objectweb.asm.Type;
 
 import java.util.HashMap;
 import java.util.List;
@@ -109,7 +110,7 @@ void write(MethodWriter writer, Globals globals) {
 
         writer.newInstance(MethodWriter.getType(actual));
         writer.dup();
-        writer.invokeConstructor(constructor.owner.type, constructor.method);
+        writer.invokeConstructor(Type.getType(constructor.target), constructor.method);
 
         for (int index = 0; index < keys.size(); ++index) {
             AExpression key = keys.get(index);
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ENewObj.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ENewObj.java
@@ -26,6 +26,7 @@
 import org.elasticsearch.painless.lookup.PainlessClass;
 import org.elasticsearch.painless.lookup.PainlessMethod;
 import org.elasticsearch.painless.lookup.PainlessMethodKey;
+import org.objectweb.asm.Type;
 
 import java.util.List;
 import java.util.Objects;
@@ -104,7 +105,7 @@ void write(MethodWriter writer, Globals globals) {
             argument.write(writer, globals);
         }
 
-        writer.invokeConstructor(constructor.owner.type, constructor.method);
+        writer.invokeConstructor(Type.getType(constructor.target), constructor.method);
     }
 
     @Override
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/PSubField.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/PSubField.java
@@ -25,6 +25,7 @@
 import org.elasticsearch.painless.MethodWriter;
 import org.elasticsearch.painless.lookup.PainlessField;
 import org.elasticsearch.painless.lookup.PainlessLookupUtility;
+import org.objectweb.asm.Type;
 
 import java.lang.reflect.Modifier;
 import java.util.Objects;
@@ -63,9 +64,9 @@ void write(MethodWriter writer, Globals globals) {
         writer.writeDebugInfo(location);
 
         if (java.lang.reflect.Modifier.isStatic(field.modifiers)) {
-            writer.getStatic(field.owner.type, field.javaName, MethodWriter.getType(field.clazz));
+            writer.getStatic(Type.getType(field.target), field.javaName, MethodWriter.getType(field.clazz));
         } else {
-            writer.getField(field.owner.type, field.javaName, MethodWriter.getType(field.clazz));
+            writer.getField(Type.getType(field.target), field.javaName, MethodWriter.getType(field.clazz));
         }
     }
 
@@ -94,9 +95,9 @@ void load(MethodWriter writer, Globals globals) {
         writer.writeDebugInfo(location);
 
         if (java.lang.reflect.Modifier.isStatic(field.modifiers)) {
-            writer.getStatic(field.owner.type, field.javaName, MethodWriter.getType(field.clazz));
+            writer.getStatic(Type.getType(field.target), field.javaName, MethodWriter.getType(field.clazz));
         } else {
-            writer.getField(field.owner.type, field.javaName, MethodWriter.getType(field.clazz));
+            writer.getField(Type.getType(field.target), field.javaName, MethodWriter.getType(field.clazz));
         }
     }
 
@@ -105,9 +106,9 @@ void store(MethodWriter writer, Globals globals) {
         writer.writeDebugInfo(location);
 
         if (java.lang.reflect.Modifier.isStatic(field.modifiers)) {
-            writer.putStatic(field.owner.type, field.javaName, MethodWriter.getType(field.clazz));
+            writer.putStatic(Type.getType(field.target), field.javaName, MethodWriter.getType(field.clazz));
         } else {
-            writer.putField(field.owner.type, field.javaName, MethodWriter.getType(field.clazz));
+            writer.putField(Type.getType(field.target), field.javaName, MethodWriter.getType(field.clazz));
         }
     }
 
diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/PainlessDocGenerator.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/PainlessDocGenerator.java

Original file line number	Diff line number	Diff line change
`@@ -334,8 +334,8 @@ static MethodHandle lookupReference(PainlessLookup painlessLookup, MethodHandles`
`334`	`334`	`}`
`335`	`335`	`int arity = interfaceMethod.arguments.size();`
`336`	`336`	`PainlessMethod implMethod = lookupMethodInternal(painlessLookup, receiverClass, name, arity);`
`337`		`- return lookupReferenceInternal(painlessLookup, methodHandlesLookup, interfaceType, implMethod.owner.name,`
`338`		`- implMethod.name, receiverClass);`
	`337`	`+ return lookupReferenceInternal(painlessLookup, methodHandlesLookup, interfaceType,`
	`338`	`+ PainlessLookupUtility.anyTypeToPainlessTypeName(implMethod.target), implMethod.name, receiverClass);`
`339`	`339`	`}`
`340`	`340`
`341`	`341`	`/** Returns a method handle to an implementation of clazz, given method reference signature. */`
Original file line number	Diff line number	Diff line change
`@@ -158,7 +158,7 @@ public static Class<?> painlessTypeNameToPainlessType(String painlessTypeName, M`
`158`	`158`	`painlessTypeName.charAt(arrayIndex++) == ']') {`
`159`	`159`	`++arrayDimensions;`
`160`	`160`	`} else {`
`161`		`- throw new IllegalArgumentException("invalid painless type [" + painlessTypeName + "].");`
	`161`	`+ throw new IllegalArgumentException("painless type [" + painlessTypeName + "] not found");`
`162`	`162`	`}`
`163`	`163`	`}`
`164`	`164`
`@@ -192,7 +192,7 @@ public static Class<?> painlessTypeNameToPainlessType(String painlessTypeName, M`
`192`	`192`	`try {`
`193`	`193`	`return Class.forName(javaDescriptor);`
`194`	`194`	`} catch (ClassNotFoundException cnfe) {`
`195`		`- throw new IllegalStateException("painless type [" + painlessTypeName + "] not found", cnfe);`
	`195`	`+ throw new IllegalArgumentException("painless type [" + painlessTypeName + "] not found", cnfe);`
`196`	`196`	`}`
`197`	`197`	`}`
`198`	`198`
`@@ -207,7 +207,7 @@ public static void validatePainlessType(Class<?> painlessType, Collection<Class<`
`207`	`207`	`}`
`208`	`208`
`209`	`209`	`if (javaClasses.contains(painlessType) == false) {`
`210`		`- throw new IllegalStateException("painless type [" + painlessTypeName + "] not found");`
	`210`	`+ throw new IllegalArgumentException("painless type [" + painlessTypeName + "] not found");`
`211`	`211`	`}`
`212`	`212`	`}`
`213`	`213`
Original file line number	Diff line number	Diff line change
`@@ -26,6 +26,7 @@`
`26`	`26`	`import org.elasticsearch.painless.lookup.PainlessClass;`
`27`	`27`	`import org.elasticsearch.painless.lookup.PainlessMethod;`
`28`	`28`	`import org.elasticsearch.painless.lookup.PainlessMethodKey;`
	`29`	`+import org.objectweb.asm.Type;`
`29`	`30`
`30`	`31`	`import java.util.List;`
`31`	`32`	`import java.util.Objects;`
`@@ -104,7 +105,7 @@ void write(MethodWriter writer, Globals globals) {`
`104`	`105`	`argument.write(writer, globals);`
`105`	`106`	`}`
`106`	`107`
`107`		`- writer.invokeConstructor(constructor.owner.type, constructor.method);`
	`108`	`+ writer.invokeConstructor(Type.getType(constructor.target), constructor.method);`
`108`	`109`	`}`
`109`	`110`
`110`	`111`	`@Override`
Original file line number	Diff line number	Diff line change
`@@ -25,6 +25,7 @@`
`25`	`25`	`import org.elasticsearch.painless.MethodWriter;`
`26`	`26`	`import org.elasticsearch.painless.lookup.PainlessField;`
`27`	`27`	`import org.elasticsearch.painless.lookup.PainlessLookupUtility;`
	`28`	`+import org.objectweb.asm.Type;`
`28`	`29`
`29`	`30`	`import java.lang.reflect.Modifier;`
`30`	`31`	`import java.util.Objects;`
`@@ -63,9 +64,9 @@ void write(MethodWriter writer, Globals globals) {`
`63`	`64`	`writer.writeDebugInfo(location);`
`64`	`65`
`65`	`66`	`if (java.lang.reflect.Modifier.isStatic(field.modifiers)) {`
`66`		`- writer.getStatic(field.owner.type, field.javaName, MethodWriter.getType(field.clazz));`
	`67`	`+ writer.getStatic(Type.getType(field.target), field.javaName, MethodWriter.getType(field.clazz));`
`67`	`68`	`} else {`
`68`		`- writer.getField(field.owner.type, field.javaName, MethodWriter.getType(field.clazz));`
	`69`	`+ writer.getField(Type.getType(field.target), field.javaName, MethodWriter.getType(field.clazz));`
`69`	`70`	`}`
`70`	`71`	`}`
`71`	`72`
`@@ -94,9 +95,9 @@ void load(MethodWriter writer, Globals globals) {`
`94`	`95`	`writer.writeDebugInfo(location);`
`95`	`96`
`96`	`97`	`if (java.lang.reflect.Modifier.isStatic(field.modifiers)) {`
`97`		`- writer.getStatic(field.owner.type, field.javaName, MethodWriter.getType(field.clazz));`
	`98`	`+ writer.getStatic(Type.getType(field.target), field.javaName, MethodWriter.getType(field.clazz));`
`98`	`99`	`} else {`
`99`		`- writer.getField(field.owner.type, field.javaName, MethodWriter.getType(field.clazz));`
	`100`	`+ writer.getField(Type.getType(field.target), field.javaName, MethodWriter.getType(field.clazz));`
`100`	`101`	`}`
`101`	`102`	`}`
`102`	`103`
`@@ -105,9 +106,9 @@ void store(MethodWriter writer, Globals globals) {`
`105`	`106`	`writer.writeDebugInfo(location);`
`106`	`107`
`107`	`108`	`if (java.lang.reflect.Modifier.isStatic(field.modifiers)) {`
`108`		`- writer.putStatic(field.owner.type, field.javaName, MethodWriter.getType(field.clazz));`
	`109`	`+ writer.putStatic(Type.getType(field.target), field.javaName, MethodWriter.getType(field.clazz));`
`109`	`110`	`} else {`
`110`		`- writer.putField(field.owner.type, field.javaName, MethodWriter.getType(field.clazz));`
	`111`	`+ writer.putField(Type.getType(field.target), field.javaName, MethodWriter.getType(field.clazz));`
`111`	`112`	`}`
`112`	`113`	`}`
`113`	`114`