Painless: Fix bug for static method calls on interfaces (#31348)

Static method calls on interfaces were not being called correctly which was causing JVM crashes. This change fixes the issue.
elastic · Jun 15, 2018 · 0324103 · 0324103
1 parent d6d0727
commit 0324103
Show file tree

Hide file tree

Showing 10 changed files with 51 additions and 11 deletions.
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
@@ -376,7 +376,8 @@ private static MethodHandle lookupReferenceInternal(Definition definition, Looku
              ref.delegateClassName,
              ref.delegateInvokeType,
              ref.delegateMethodName,
-             ref.delegateMethodType
+             ref.delegateMethodType,
+             ref.isDelegateInterface ? 1 : 0
          );
          return callSite.dynamicInvoker().asType(MethodType.methodType(clazz.clazz, captures));
      }

diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Definition.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Definition.java
@@ -20,6 +20,7 @@
 package org.elasticsearch.painless;
 
 import org.elasticsearch.painless.spi.Whitelist;
+import org.objectweb.asm.Opcodes;
 
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodHandles;
@@ -202,16 +203,28 @@ public MethodType getMethodType() {
 
         public void write(MethodWriter writer) {
             final org.objectweb.asm.Type type;
+            final Class<?> clazz;
             if (augmentation != null) {
                 assert java.lang.reflect.Modifier.isStatic(modifiers);
+                clazz = augmentation;
                 type = org.objectweb.asm.Type.getType(augmentation);
             } else {
+                clazz = owner.clazz;
                 type = owner.type;
             }
 
             if (java.lang.reflect.Modifier.isStatic(modifiers)) {
-                writer.invokeStatic(type, method);
-            } else if (java.lang.reflect.Modifier.isInterface(owner.clazz.getModifiers())) {
+                // invokeStatic assumes that the owner class is not an interface, so this is a
+                // special case for interfaces where the interface method boolean needs to be set to
+                // true to reference the appropriate class constant when calling a static interface
+                // method since java 8 did not check, but java 9 and 10 do
+                if (java.lang.reflect.Modifier.isInterface(clazz.getModifiers())) {
+                    writer.visitMethodInsn(Opcodes.INVOKESTATIC,
+                            type.getInternalName(), name, getMethodType().toMethodDescriptorString(), true);
+                } else {
+                    writer.invokeStatic(type, method);
+                }
+            } else if (java.lang.reflect.Modifier.isInterface(clazz.getModifiers())) {
                 writer.invokeInterface(type, method);
             } else {
                 writer.invokeVirtual(type, method);

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
@@ -66,6 +66,9 @@ public class FunctionRef {
     /** delegate method type method as type */
     public final Type delegateType;
 
+    /** whether a call is made on a delegate interface */
+    public final boolean isDelegateInterface;
+
     /**
      * Creates a new FunctionRef, which will resolve {@code type::call} from the whitelist.
      * @param definition the whitelist against which this script is being compiled
@@ -97,10 +100,13 @@ public FunctionRef(Class<?> expected, Method interfaceMethod, Method delegateMet
         // the Painless$Script class can be inferred if owner is null
         if (delegateMethod.owner == 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();
         }
 
         if ("<init>".equals(delegateMethod.name)) {
@@ -139,6 +145,7 @@ public FunctionRef(Class<?> expected,
         delegateInvokeType = H_INVOKESTATIC;
         this.delegateMethodName = delegateMethodName;
         this.delegateMethodType = delegateMethodType.dropParameterTypes(0, numCaptures);
+        isDelegateInterface = false;
 
         this.interfaceMethod = null;
         delegateMethod = null;

diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java
@@ -188,6 +188,10 @@ private Capture(int count, Class<?> type) {
      * @param delegateMethodName The name of the method to be called in the Painless script class
      * @param delegateMethodType The type of method call in the Painless script class without
      *                           the captured types
+     * @param isDelegateInterface If the method to be called is owned by an interface where
+     *                            if the value is '1' if the delegate is an interface and '0'
+     *                            otherwise; note this is an int because the bootstrap method
+     *                            cannot convert constants to boolean
      * @return A {@link CallSite} linked to a factory method for creating a lambda class
      * that implements the expected functional interface
      * @throws LambdaConversionException Thrown when an illegal type conversion occurs at link time
@@ -200,7 +204,8 @@ public static CallSite lambdaBootstrap(
             String delegateClassName,
             int delegateInvokeType,
             String delegateMethodName,
-            MethodType delegateMethodType)
+            MethodType delegateMethodType,
+            int isDelegateInterface)
             throws LambdaConversionException {
         Loader loader = (Loader)lookup.lookupClass().getClassLoader();
         String lambdaClassName = Type.getInternalName(lookup.lookupClass()) + "$$Lambda" + loader.newLambdaIdentifier();
@@ -225,7 +230,7 @@ public static CallSite lambdaBootstrap(
 
         generateInterfaceMethod(cw, factoryMethodType, lambdaClassType, interfaceMethodName,
             interfaceMethodType, delegateClassType, delegateInvokeType,
-            delegateMethodName, delegateMethodType, captures);
+            delegateMethodName, delegateMethodType, isDelegateInterface == 1, captures);
 
         endLambdaClass(cw);
 
@@ -369,6 +374,7 @@ private static void generateInterfaceMethod(
             int delegateInvokeType,
             String delegateMethodName,
             MethodType delegateMethodType,
+            boolean isDelegateInterface,
             Capture[] captures)
             throws LambdaConversionException {
 
@@ -434,7 +440,7 @@ private static void generateInterfaceMethod(
         Handle delegateHandle =
             new Handle(delegateInvokeType, delegateClassType.getInternalName(),
                 delegateMethodName, delegateMethodType.toMethodDescriptorString(),
-                delegateInvokeType == H_INVOKEINTERFACE);
+                isDelegateInterface);
         iface.invokeDynamic(delegateMethodName, Type.getMethodType(interfaceMethodType
                 .toMethodDescriptorString()).getDescriptor(), DELEGATE_BOOTSTRAP_HANDLE,
                 delegateHandle);

diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/WriterConstants.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/WriterConstants.java
@@ -141,8 +141,8 @@ public final class WriterConstants {
 
     /** invokedynamic bootstrap for lambda expression/method references */
     public static final MethodType LAMBDA_BOOTSTRAP_TYPE =
-            MethodType.methodType(CallSite.class, MethodHandles.Lookup.class, String.class,
-                                  MethodType.class, MethodType.class, String.class, int.class, String.class, MethodType.class);
+            MethodType.methodType(CallSite.class, MethodHandles.Lookup.class, String.class, MethodType.class,
+                    MethodType.class, String.class, int.class, String.class, MethodType.class, int.class);
     public static final Handle LAMBDA_BOOTSTRAP_HANDLE =
             new Handle(Opcodes.H_INVOKESTATIC, Type.getInternalName(LambdaBootstrap.class),
                 "lambdaBootstrap", LAMBDA_BOOTSTRAP_TYPE.toMethodDescriptorString(), false);

diff --git a/...es/lang-painless/src/main/java/org/elasticsearch/painless/node/ECapturingFunctionRef.java b/...es/lang-painless/src/main/java/org/elasticsearch/painless/node/ECapturingFunctionRef.java
@@ -121,7 +121,8 @@ void write(MethodWriter writer, Globals globals) {
                 ref.delegateClassName,
                 ref.delegateInvokeType,
                 ref.delegateMethodName,
-                ref.delegateType
+                ref.delegateType,
+                ref.isDelegateInterface ? 1 : 0
             );
         }
     }

diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EFunctionRef.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EFunctionRef.java
@@ -112,7 +112,8 @@ void write(MethodWriter writer, Globals globals) {
                 ref.delegateClassName,
                 ref.delegateInvokeType,
                 ref.delegateMethodName,
-                ref.delegateType
+                ref.delegateType,
+                ref.isDelegateInterface ? 1 : 0
             );
         } else {
             // TODO: don't do this: its just to cutover :)

diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ELambda.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ELambda.java
@@ -222,7 +222,8 @@ void write(MethodWriter writer, Globals globals) {
                 ref.delegateClassName,
                 ref.delegateInvokeType,
                 ref.delegateMethodName,
-                ref.delegateType
+                ref.delegateType,
+                ref.isDelegateInterface ? 1 : 0
             );
         } else {
             // placeholder

diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicExpressionTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicExpressionTests.java
@@ -264,6 +264,11 @@ public void testNullSafeDeref() {
 //        assertEquals(null, exec("def a = ['thing': 'bar']; a.other?.cat?.dog = 'wombat'; return a.other?.cat?.dog"));
     }
 
+    // test to ensure static interface methods are called correctly
+    public void testStaticInterfaceMethod() {
+        assertEquals(4, exec("def values = [1, 4, 3, 2]; values.sort(Comparator.comparing(p -> p)); return values[3]"));
+    }
+
     private void assertMustBeNullable(String script) {
         Exception e = expectScriptThrows(IllegalArgumentException.class, false, () -> exec(script));
         assertEquals("Result of null safe operator must be nullable", e.getMessage());

diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/FunctionRefTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/FunctionRefTests.java
@@ -184,6 +184,11 @@ public void testInterfaceDefaultMethodDef() {
                                  "def map = new HashMap(); f(map::getOrDefault)"));
     }
 
+    public void testInterfaceStaticMethod() {
+        assertEquals(-1, exec("Supplier get(Supplier supplier) { return supplier }" +
+                              "Supplier s = get(Comparator::naturalOrder); s.get().compare(1, 2)"));
+    }
+
     public void testMethodMissing() {
         Exception e = expectScriptThrows(IllegalArgumentException.class, () -> {
             exec("List l = [2, 1]; l.sort(Integer::bogus); return l.get(0);");