From a31f0bb3c0b5e08698214d68a23e7a8be1cc0206 Mon Sep 17 00:00:00 2001 From: Andy Clement Date: Thu, 5 May 2016 09:57:15 -0700 Subject: [PATCH] Fix compilation of expressions using instanceof and primitives Prior to this commit the SpEL compiler would generate bad bytecode if the left hand operand of an instanceof was a primitive or if the right hand operand was a primitive type reference. With the fixes primitives on the left hand side are now correctly boxed and special handling is in place for when the right hand side is a primitive type reference. Using a primitive type reference on the right always causes the instanceof check to return false. Additionally a guard has been added such that compilation is not allowed when the right hand side of an expression is not a type reference. If it is, for example, a variable reference that evaluates to a type reference then that cannot be expressed in bytecode so compilation is not performed. Issue: SPR-14250 --- .../spel/ast/OperatorInstanceof.java | 22 +++++++-- .../spel/SpelCompilationCoverageTests.java | 49 ++++++++++++++++++- 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OperatorInstanceof.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OperatorInstanceof.java index 5a3063b6f4..6c880bdd6b 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OperatorInstanceof.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OperatorInstanceof.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2016 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -53,8 +53,9 @@ public class OperatorInstanceof extends Operator { */ @Override public BooleanTypedValue getValueInternal(ExpressionState state) throws EvaluationException { + SpelNodeImpl rightOperand = getRightOperand(); TypedValue left = getLeftOperand().getValueInternal(state); - TypedValue right = getRightOperand().getValueInternal(state); + TypedValue right = rightOperand.getValueInternal(state); Object leftValue = left.getValue(); Object rightValue = right.getValue(); BooleanTypedValue result = null; @@ -71,7 +72,11 @@ public class OperatorInstanceof extends Operator { result = BooleanTypedValue.forValue(rightClass.isAssignableFrom(leftValue.getClass())); } this.type = rightClass; - this.exitTypeDescriptor = "Z"; + if (rightOperand instanceof TypeReference) { + // Can only generate bytecode where the right operand is a direct type reference, + // not if it is indirect (for example when right operand is a variable reference) + this.exitTypeDescriptor = "Z"; + } return result; } @@ -83,7 +88,16 @@ public class OperatorInstanceof extends Operator { @Override public void generateCode(MethodVisitor mv, CodeFlow cf) { getLeftOperand().generateCode(mv, cf); - mv.visitTypeInsn(INSTANCEOF,Type.getInternalName(this.type)); + CodeFlow.insertBoxIfNecessary(mv, cf.lastDescriptor()); + if (this.type.isPrimitive()) { + // always false - but left operand code always driven + // in case it had side effects + mv.visitInsn(POP); + mv.visitInsn(ICONST_0); // value of false + } + else { + mv.visitTypeInsn(INSTANCEOF,Type.getInternalName(this.type)); + } cf.pushDescriptor(this.exitTypeDescriptor); } diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java index 915114c6cc..cfde694a64 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2015 the original author or authors. + * Copyright 2002-2016 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -235,6 +235,53 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertEquals(true,expression.getValue(root)); } + @Test + public void operatorInstanceOf_SPR14250() throws Exception { + // primitive left operand - should get boxed, return true + expression = parse("3 instanceof T(Integer)"); + assertEquals(true,expression.getValue()); + assertCanCompile(expression); + assertEquals(true,expression.getValue()); + + // primitive left operand - should get boxed, return false + expression = parse("3 instanceof T(String)"); + assertEquals(false,expression.getValue()); + assertCanCompile(expression); + assertEquals(false,expression.getValue()); + + // double slot left operand - should get boxed, return false + expression = parse("3.0d instanceof T(Integer)"); + assertEquals(false,expression.getValue()); + assertCanCompile(expression); + assertEquals(false,expression.getValue()); + + // double slot left operand - should get boxed, return true + expression = parse("3.0d instanceof T(Double)"); + assertEquals(true,expression.getValue()); + assertCanCompile(expression); + assertEquals(true,expression.getValue()); + + // Only when the right hand operand is a direct type reference + // will it be compilable. + StandardEvaluationContext ctx = new StandardEvaluationContext(); + ctx.setVariable("foo", String.class); + expression = parse("3 instanceof #foo"); + assertEquals(false,expression.getValue(ctx)); + assertCantCompile(expression); + + // use of primitive as type for instanceof check - compilable + // but always false + expression = parse("3 instanceof T(int)"); + assertEquals(false,expression.getValue()); + assertCanCompile(expression); + assertEquals(false,expression.getValue()); + + expression = parse("3 instanceof T(long)"); + assertEquals(false,expression.getValue()); + assertCanCompile(expression); + assertEquals(false,expression.getValue()); + } + @Test public void stringLiteral() throws Exception { expression = parser.parseExpression("'abcde'");