From 03241037373a727444cd7fda2ab23825a6eb84d6 Mon Sep 17 00:00:00 2001 From: Jack Conradson Date: Thu, 14 Jun 2018 18:30:37 -0700 Subject: [PATCH] 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. --- .../java/org/elasticsearch/painless/Def.java | 3 ++- .../org/elasticsearch/painless/Definition.java | 17 +++++++++++++++-- .../org/elasticsearch/painless/FunctionRef.java | 7 +++++++ .../elasticsearch/painless/LambdaBootstrap.java | 12 +++++++++--- .../elasticsearch/painless/WriterConstants.java | 4 ++-- .../painless/node/ECapturingFunctionRef.java | 3 ++- .../painless/node/EFunctionRef.java | 3 ++- .../elasticsearch/painless/node/ELambda.java | 3 ++- .../painless/BasicExpressionTests.java | 5 +++++ .../painless/FunctionRefTests.java | 5 +++++ 10 files changed, 51 insertions(+), 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 index 661af1b6c9137..988a31a24ee27 100644 --- 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 index f97df128f15e5..75575d6f12568 100644 --- 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 index 66cf78e857220..0b698dd244192 100644 --- 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 ("".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 index 7a2ec9da34e29..3fc8554b271e2 100644 --- 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 index 9150e2609b700..18d7d94492e67 100644 --- 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/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ECapturingFunctionRef.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ECapturingFunctionRef.java index 724679d3f8538..e6f2f7ebf91f9 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ECapturingFunctionRef.java +++ b/modules/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 index 636623004c982..c82b1003a55f1 100644 --- 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 index c37ff435f566f..a7213e75ca485 100644 --- 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 index 97e1f01fdfc94..6ff727d987cdd 100644 --- 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 index 7c49d042108ef..fd47db6b83d41 100644 --- 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);");