From 4b09fcc67c52f85a6c75a561090a8d6a742393f8 Mon Sep 17 00:00:00 2001
From: Juergen Hoeller <jhoeller@vmware.com>
Date: Tue, 22 Jul 2014 16:24:39 +0200
Subject: [PATCH] Revised exitTypeDescriptor handling to avoid NPEs

Issue: SPR-12014
---
 .../expression/spel/CodeFlow.java             |  2 +-
 .../spel/ast/CompoundExpression.java          | 20 ++---
 .../expression/spel/ast/Elvis.java            | 79 ++++++++-----------
 .../spel/ast/FunctionReference.java           | 48 +++++------
 .../expression/spel/ast/MethodReference.java  | 18 ++---
 .../spel/ast/PropertyOrFieldReference.java    | 25 +++---
 .../expression/spel/ast/SpelNodeImpl.java     | 10 +--
 7 files changed, 86 insertions(+), 116 deletions(-)

diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java b/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java
index 26a47af200..98a68ef579 100644
--- a/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java
+++ b/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java
@@ -63,7 +63,7 @@ public class CodeFlow implements Opcodes {
 	 * @param descriptor type descriptor for most recently evaluated element
 	 */
 	public void pushDescriptor(String descriptor) {
-		Assert.notNull(descriptor);
+		Assert.notNull(descriptor, "Descriptor must not be null");
 		this.compilationScopes.peek().add(descriptor);
 	}
 
diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/CompoundExpression.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/CompoundExpression.java
index a540ac9c28..e14f997745 100644
--- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/CompoundExpression.java
+++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/CompoundExpression.java
@@ -85,14 +85,9 @@ public class CompoundExpression extends SpelNodeImpl {
 	public TypedValue getValueInternal(ExpressionState state) throws EvaluationException {
 		ValueRef ref = getValueRef(state);
 		TypedValue result = ref.getValue();
-		this.exitTypeDescriptor = this.children[this.children.length-1].getExitDescriptor();
+		this.exitTypeDescriptor = this.children[this.children.length - 1].getExitDescriptor();
 		return result;
 	}
-	
-	@Override
-	public String getExitDescriptor() {
-		return this.exitTypeDescriptor;
-	}
 
 	@Override
 	public void setValue(ExpressionState state, Object value) throws EvaluationException {
@@ -118,7 +113,7 @@ public class CompoundExpression extends SpelNodeImpl {
 	
 	@Override
 	public boolean isCompilable() {
-		for (SpelNodeImpl child: children) {
+		for (SpelNodeImpl child: this.children) {
 			if (!child.isCompilable()) {
 				return false;
 			}
@@ -129,16 +124,15 @@ public class CompoundExpression extends SpelNodeImpl {
 	@Override
 	public void generateCode(MethodVisitor mv,CodeFlow codeflow) {	
 		// TODO could optimize T(SomeType).staticMethod - no need to generate the T() part
-		for (int i=0;i<children.length;i++) {
-			SpelNodeImpl child = children[i];
-			if (child instanceof TypeReference && 
-				(i+1) < children.length && 
-				children[i+1] instanceof MethodReference) {
+		for (int i = 0; i < this.children.length;i++) {
+			SpelNodeImpl child = this.children[i];
+			if (child instanceof TypeReference && (i + 1) < this.children.length &&
+					this.children[i+1] instanceof MethodReference) {
 				continue;
 			}
 			child.generateCode(mv, codeflow);
 		}
-		codeflow.pushDescriptor(this.getExitDescriptor());
+		codeflow.pushDescriptor(getExitDescriptor());
 	}
 
 }
diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Elvis.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Elvis.java
index 5d9d8987a8..658ef860c3 100644
--- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Elvis.java
+++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Elvis.java
@@ -22,12 +22,14 @@ import org.springframework.expression.EvaluationException;
 import org.springframework.expression.TypedValue;
 import org.springframework.expression.spel.CodeFlow;
 import org.springframework.expression.spel.ExpressionState;
+import org.springframework.util.StringUtils;
 
 /**
  * Represents the elvis operator ?:. For an expression "a?:b" if a is not null, the value
  * of the expression is "a", if a is null then the value of the expression is "b".
  *
  * @author Andy Clement
+ * @author Juergen Hoeller
  * @since 3.0
  */
 public class Elvis extends SpelNodeImpl {
@@ -38,76 +40,38 @@ public class Elvis extends SpelNodeImpl {
 
 
 	/**
-	 * Evaluate the condition and if not null, return it. If it is null return the other
-	 * value.
+	 * Evaluate the condition and if not null, return it.
+	 * If it is null, return the other value.
 	 * @param state the expression state
-	 * @throws EvaluationException if the condition does not evaluate correctly to a
-	 *         boolean or there is a problem executing the chosen alternative
+	 * @throws EvaluationException if the condition does not evaluate correctly
+	 * to a boolean or there is a problem executing the chosen alternative
 	 */
 	@Override
 	public TypedValue getValueInternal(ExpressionState state) throws EvaluationException {
 		TypedValue value = this.children[0].getValueInternal(state);
-		if ((value.getValue() != null) && !((value.getValue() instanceof String) &&
-				((String) value.getValue()).length() == 0)) {
+		if (!StringUtils.isEmpty(value.getValue())) {
 			return value;
 		}
 		else {
 			TypedValue result = this.children[1].getValueInternal(state);
-			if (exitTypeDescriptor == null) {
-				String testDescriptor = this.children[0].exitTypeDescriptor;
-				String ifNullDescriptor = this.children[1].exitTypeDescriptor;
-				if (testDescriptor.equals(ifNullDescriptor)) {
-					this.exitTypeDescriptor = testDescriptor;
-				}
-				else {
-					this.exitTypeDescriptor = "Ljava/lang/Object";
-				}
-			}
+			computeExitTypeDescriptor();
 			return result;
 		}
 	}
 
 	@Override
 	public String toStringAST() {
-		return new StringBuilder().append(getChild(0).toStringAST()).append(" ?: ").append(
-				getChild(1).toStringAST()).toString();
-	}
-
-	private void computeExitTypeDescriptor() {
-		if (exitTypeDescriptor == null &&
-				this.children[0].getExitDescriptor()!=null &&
-				this.children[1].getExitDescriptor()!=null) {
-			String conditionDescriptor = this.children[0].exitTypeDescriptor;
-			String ifNullValueDescriptor = this.children[1].exitTypeDescriptor;
-			if (conditionDescriptor.equals(ifNullValueDescriptor)) {
-				this.exitTypeDescriptor = conditionDescriptor;
-			}
-			else if (conditionDescriptor.equals("Ljava/lang/Object") && !CodeFlow.isPrimitive(ifNullValueDescriptor)) {
-				this.exitTypeDescriptor = ifNullValueDescriptor;
-			}
-			else if (ifNullValueDescriptor.equals("Ljava/lang/Object") && !CodeFlow.isPrimitive(conditionDescriptor)) {
-				this.exitTypeDescriptor = conditionDescriptor;
-			}
-			else {
-				// Use the easiest to compute common super type
-				this.exitTypeDescriptor = "Ljava/lang/Object";
-			}
-		}
+		return getChild(0).toStringAST() + " ?: " + getChild(1).toStringAST();
 	}
 
 	@Override
 	public boolean isCompilable() {
 		SpelNodeImpl condition = this.children[0];
 		SpelNodeImpl ifNullValue = this.children[1];
-		if (!(condition.isCompilable() && ifNullValue.isCompilable())) {
-			return false;
-		}
-		return
-			condition.getExitDescriptor()!=null &&
-			ifNullValue.getExitDescriptor()!=null;
+		return (condition.isCompilable() && ifNullValue.isCompilable() &&
+				condition.getExitDescriptor() != null && ifNullValue.getExitDescriptor() != null);
 	}
 
-
 	@Override
 	public void generateCode(MethodVisitor mv, CodeFlow codeflow) {
 		// exit type descriptor can be null if both components are literal expressions
@@ -128,4 +92,25 @@ public class Elvis extends SpelNodeImpl {
 		codeflow.pushDescriptor(getExitDescriptor());
 	}
 
+	private void computeExitTypeDescriptor() {
+		if (this.exitTypeDescriptor == null && this.children[0].getExitDescriptor() != null &&
+				this.children[1].getExitDescriptor() != null) {
+			String conditionDescriptor = this.children[0].exitTypeDescriptor;
+			String ifNullValueDescriptor = this.children[1].exitTypeDescriptor;
+			if (conditionDescriptor.equals(ifNullValueDescriptor)) {
+				this.exitTypeDescriptor = conditionDescriptor;
+			}
+			else if (conditionDescriptor.equals("Ljava/lang/Object") && !CodeFlow.isPrimitive(ifNullValueDescriptor)) {
+				this.exitTypeDescriptor = ifNullValueDescriptor;
+			}
+			else if (ifNullValueDescriptor.equals("Ljava/lang/Object") && !CodeFlow.isPrimitive(conditionDescriptor)) {
+				this.exitTypeDescriptor = conditionDescriptor;
+			}
+			else {
+				// Use the easiest to compute common super type
+				this.exitTypeDescriptor = "Ljava/lang/Object";
+			}
+		}
+	}
+
 }
diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/FunctionReference.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/FunctionReference.java
index f0fe7d4e8e..55d412728c 100644
--- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/FunctionReference.java
+++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/FunctionReference.java
@@ -50,11 +50,11 @@ public class FunctionReference extends SpelNodeImpl {
 
 	private final String name;
 
-	// Captures the most recently used method for the function invocation *if*
-	// the method can safely be used for compilation (i.e. no argument conversion is
-	// going on)
+	// Captures the most recently used method for the function invocation *if* the method
+	// can safely be used for compilation (i.e. no argument conversion is going on)
 	private Method method;
 
+
 	public FunctionReference(String functionName, int pos, SpelNodeImpl... arguments) {
 		super(pos,arguments);
 		this.name = functionName;
@@ -63,29 +63,29 @@ public class FunctionReference extends SpelNodeImpl {
 
 	@Override
 	public TypedValue getValueInternal(ExpressionState state) throws EvaluationException {
-		TypedValue o = state.lookupVariable(this.name);
-		if (o == null) {
+		TypedValue value = state.lookupVariable(this.name);
+		if (value == null) {
 			throw new SpelEvaluationException(getStartPosition(), SpelMessage.FUNCTION_NOT_DEFINED, this.name);
 		}
 
 		// Two possibilities: a lambda function or a Java static method registered as a function
-		if (!(o.getValue() instanceof Method)) {
-			throw new SpelEvaluationException(SpelMessage.FUNCTION_REFERENCE_CANNOT_BE_INVOKED, this.name, o.getClass());
+		if (!(value.getValue() instanceof Method)) {
+			throw new SpelEvaluationException(SpelMessage.FUNCTION_REFERENCE_CANNOT_BE_INVOKED, this.name, value.getClass());
 		}
+
 		try {
-			return executeFunctionJLRMethod(state, (Method) o.getValue());
+			return executeFunctionJLRMethod(state, (Method) value.getValue());
 		}
-		catch (SpelEvaluationException se) {
-			se.setPosition(getStartPosition());
-			throw se;
+		catch (SpelEvaluationException ex) {
+			ex.setPosition(getStartPosition());
+			throw ex;
 		}
 	}
 
 	/**
 	 * Execute a function represented as a java.lang.reflect.Method.
-	 *
 	 * @param state the expression evaluation state
-	 * @param the java method to invoke
+	 * @param method the method to invoke
 	 * @return the return value of the invoked Java method
 	 * @throws EvaluationException if there is any problem invoking the method
 	 */
@@ -107,11 +107,10 @@ public class FunctionReference extends SpelNodeImpl {
 		// Convert arguments if necessary and remap them for varargs if required
 		if (functionArgs != null) {
 			TypeConverter converter = state.getEvaluationContext().getTypeConverter();
-			argumentConversionOccurred |= ReflectionHelper.convertAllArguments(converter, functionArgs, method);
+			argumentConversionOccurred = ReflectionHelper.convertAllArguments(converter, functionArgs, method);
 		}
 		if (method.isVarArgs()) {
-			functionArgs = ReflectionHelper.setupArgumentsForVarargsInvocation(
-					method.getParameterTypes(), functionArgs);
+			functionArgs = ReflectionHelper.setupArgumentsForVarargsInvocation(method.getParameterTypes(), functionArgs);
 		}
 
 		try {
@@ -161,19 +160,19 @@ public class FunctionReference extends SpelNodeImpl {
 	@Override
 	public boolean isCompilable() {
 		// Don't yet support non-static method compilation.
-		return method!=null && Modifier.isStatic(method.getModifiers());
+		return (this.method != null && Modifier.isStatic(this.method.getModifiers()));
 	}
 	
 	@Override 
 	public void generateCode(MethodVisitor mv,CodeFlow codeflow) {
-		String methodDeclaringClassSlashedDescriptor = method.getDeclaringClass().getName().replace('.','/');
-		String[] paramDescriptors = CodeFlow.toParamDescriptors(method);
-		for (int c = 0; c < children.length; c++) {
-			SpelNodeImpl child = children[c];
+		String methodDeclaringClassSlashedDescriptor = this.method.getDeclaringClass().getName().replace('.','/');
+		String[] paramDescriptors = CodeFlow.toParamDescriptors(this.method);
+		for (int c = 0; c < this.children.length; c++) {
+			SpelNodeImpl child = this.children[c];
 			codeflow.enterCompilationScope();
 			child.generateCode(mv, codeflow);
 			// Check if need to box it for the method reference?
-			if (CodeFlow.isPrimitive(codeflow.lastDescriptor()) && (paramDescriptors[c].charAt(0)=='L')) {
+			if (CodeFlow.isPrimitive(codeflow.lastDescriptor()) && paramDescriptors[c].charAt(0) == 'L') {
 				CodeFlow.insertBoxIfNecessary(mv, codeflow.lastDescriptor().charAt(0));
 			}
 			else if (!codeflow.lastDescriptor().equals(paramDescriptors[c])) {
@@ -182,8 +181,9 @@ public class FunctionReference extends SpelNodeImpl {
 			}
 			codeflow.exitCompilationScope();
 		}
-		mv.visitMethodInsn(INVOKESTATIC,methodDeclaringClassSlashedDescriptor,method.getName(),CodeFlow.createSignatureDescriptor(method),false);
-		codeflow.pushDescriptor(exitTypeDescriptor);
+		mv.visitMethodInsn(INVOKESTATIC, methodDeclaringClassSlashedDescriptor, this.method.getName(),
+				CodeFlow.createSignatureDescriptor(this.method),false);
+		codeflow.pushDescriptor(this.exitTypeDescriptor);
 	}
 
 }
diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java
index fdf6e46541..ff1986be56 100644
--- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java
+++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java
@@ -91,11 +91,6 @@ public class MethodReference extends SpelNodeImpl {
 		return result;
 	}
 
-	@Override
-	public String getExitDescriptor() {
-		return exitTypeDescriptor;
-	}
-
 	private TypedValue getValueInternal(EvaluationContext evaluationContext,
 			Object value, TypeDescriptor targetType, Object[] arguments) {
 
@@ -360,13 +355,13 @@ public class MethodReference extends SpelNodeImpl {
 		boolean itf = method.getDeclaringClass().isInterface();
 		String methodDeclaringClassSlashedDescriptor = method.getDeclaringClass().getName().replace('.','/');
 		if (!isStaticMethod) {
-			if (descriptor == null || !descriptor.equals(method.getDeclaringClass())) {
-				mv.visitTypeInsn(CHECKCAST, method.getDeclaringClass().getName().replace('.','/'));
+			if (descriptor == null || !descriptor.equals(methodDeclaringClassSlashedDescriptor)) {
+				mv.visitTypeInsn(CHECKCAST, methodDeclaringClassSlashedDescriptor);
 			}
 		}
 		String[] paramDescriptors = CodeFlow.toParamDescriptors(method);
-		for (int c=0;c<children.length;c++) {
-			SpelNodeImpl child = children[c];
+		for (int c = 0; c < this.children.length;c++) {
+			SpelNodeImpl child = this.children[c];
 			codeflow.enterCompilationScope();
 			child.generateCode(mv, codeflow);
 			// Check if need to box it for the method reference?
@@ -379,8 +374,9 @@ public class MethodReference extends SpelNodeImpl {
 			}
 			codeflow.exitCompilationScope();
 		}
-		mv.visitMethodInsn(isStaticMethod?INVOKESTATIC:INVOKEVIRTUAL,methodDeclaringClassSlashedDescriptor,method.getName(),CodeFlow.createSignatureDescriptor(method), itf);
-		codeflow.pushDescriptor(exitTypeDescriptor);
+		mv.visitMethodInsn(isStaticMethod ? INVOKESTATIC : INVOKEVIRTUAL,
+				methodDeclaringClassSlashedDescriptor, method.getName(), CodeFlow.createSignatureDescriptor(method), itf);
+		codeflow.pushDescriptor(this.exitTypeDescriptor);
 	}
 
 }
diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/PropertyOrFieldReference.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/PropertyOrFieldReference.java
index 94d3436e22..1aa3131d5b 100644
--- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/PropertyOrFieldReference.java
+++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/PropertyOrFieldReference.java
@@ -79,18 +79,13 @@ public class PropertyOrFieldReference extends SpelNodeImpl {
 	@Override
 	public TypedValue getValueInternal(ExpressionState state) throws EvaluationException {
 		TypedValue tv = getValueInternal(state.getActiveContextObject(), state.getEvaluationContext(), state.getConfiguration().isAutoGrowNullReferences());
-		if (cachedReadAccessor instanceof CompilablePropertyAccessor) {
-			CompilablePropertyAccessor accessor = (CompilablePropertyAccessor)cachedReadAccessor;
-			exitTypeDescriptor = CodeFlow.toDescriptor(accessor.getPropertyType());
+		if (this.cachedReadAccessor instanceof CompilablePropertyAccessor) {
+			CompilablePropertyAccessor accessor = (CompilablePropertyAccessor) this.cachedReadAccessor;
+			this.exitTypeDescriptor = CodeFlow.toDescriptor(accessor.getPropertyType());
 		}
 		return tv;
 	}
 
-	@Override
-	public String getExitDescriptor() {
-		return exitTypeDescriptor;
-	}
-
 	private TypedValue getValueInternal(TypedValue contextObject, EvaluationContext eContext,
 			boolean isAutoGrowNullReferences) throws EvaluationException {
 
@@ -290,12 +285,14 @@ public class PropertyOrFieldReference extends SpelNodeImpl {
 
 	// TODO when there is more time, remove this and use the version in AstUtils
 	/**
-	 * Determines the set of property resolvers that should be used to try and access a property on the specified target
-	 * type. The resolvers are considered to be in an ordered list, however in the returned list any that are exact
-	 * matches for the input target type (as opposed to 'general' resolvers that could work for any type) are placed at
-	 * the start of the list. In addition, there are specific resolvers that exactly name the class in question and
-	 * resolvers that name a specific class but it is a supertype of the class we have. These are put at the end of the
-	 * specific resolvers set and will be tried after exactly matching accessors but before generic accessors.
+	 * Determines the set of property resolvers that should be used to try and access a property
+	 * on the specified target type. The resolvers are considered to be in an ordered list,
+	 * however in the returned list any that are exact matches for the input target type (as
+	 * opposed to 'general' resolvers that could work for any type) are placed at the start of the
+	 * list. In addition, there are specific resolvers that exactly name the class in question
+	 * and resolvers that name a specific class but it is a supertype of the class we have.
+	 * These are put at the end of the specific resolvers set and will be tried after exactly
+	 * matching accessors but before generic accessors.
 	 * @param contextObject the object upon which property access is being attempted
 	 * @return a list of resolvers that should be tried in order to access the property
 	 */
diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/SpelNodeImpl.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/SpelNodeImpl.java
index 36363c319a..3fd02d78f2 100644
--- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/SpelNodeImpl.java
+++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/SpelNodeImpl.java
@@ -56,7 +56,7 @@ public abstract class SpelNodeImpl implements SpelNode, Opcodes {
 	 * does not include the trailing semicolon (for non array reference types). Some examples:
 	 * Ljava/lang/String, I, [I
      */
-	protected String exitTypeDescriptor;
+	protected volatile String exitTypeDescriptor;
 
 
 	public SpelNodeImpl(int pos, SpelNodeImpl... operands) {
@@ -76,7 +76,7 @@ public abstract class SpelNodeImpl implements SpelNode, Opcodes {
 		SpelNodeImpl result = null;
 		if (this.parent != null) {
 			for (SpelNodeImpl child : this.parent.children) {
-				if (this==child) {
+				if (this == child) {
 					break;
 				}
 				result = child;
@@ -92,18 +92,16 @@ public abstract class SpelNodeImpl implements SpelNode, Opcodes {
 		if (this.parent != null) {
 			SpelNodeImpl[] peers = this.parent.children;
 			for (int i = 0, max = peers.length; i < max; i++) {
-				if (peers[i] == this) {
-					if ((i + 1) >= max) {
+				if (this == peers[i]) {
+					if (i + 1 >= max) {
 						return false;
 					}
-
 					Class<?> clazz = peers[i + 1].getClass();
 					for (Class<?> desiredClazz : clazzes) {
 						if (clazz.equals(desiredClazz)) {
 							return true;
 						}
 					}
-
 					return false;
 				}
 			}