Bug 452325 - Protect against IVariable.getInitialValue() calling itself

recursively along any code path

Change-Id: Idbf46ca82b431e72bf6b2598427430c37ef65cc4
Signed-off-by: Nathan Ridge <zeratul976@hotmail.com>
Reviewed-on: https://git.eclipse.org/r/37220
Tested-by: Hudson CI
Reviewed-by: Elena Laskavaia <elaskavaia.cdt@gmail.com>
Reviewed-by: Sergey Prigogin <eclipse.sprigogin@gmail.com>
Tested-by: Sergey Prigogin <eclipse.sprigogin@gmail.com>

codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/internal/model/cfg/ControlFlowGraphBuilder.java

@@ -533,7 +533,7 @@ public class ControlFlowGraphBuilder {
 		if (node instanceof ICfgData) {
 			IASTNode ast = (IASTNode) ((ICfgData) node).getData();
 			if (ast instanceof IASTExpression) {
-				IValue dvalue = Value.create((IASTExpression) ast, 5);
+				IValue dvalue = Value.create((IASTExpression) ast);
 				Long numericalValue = dvalue.numericalValue();
 				if (numericalValue == null)
 					return false;

codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/ReturnCheckerTest.java

@@ -406,4 +406,13 @@ public class ReturnCheckerTest extends CheckerTestCase {
 		loadCodeAndRunCpp(getAboveComment());
 		checkNoErrors();
 	}
+
+	//	int foo() {
+	//	    int waldo = waldo();
+	//	    if (waldo);
+	//	}
+	public void testSelfReferencingVariable_452325() throws Exception {
+		// Just check that codan runs without any exceptions being thrown.
+		loadCodeAndRunCpp(getAboveComment());
+	}
 }

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/ASTEnumerator.java

@@ -151,7 +151,7 @@ public abstract class ASTEnumerator extends ASTNode implements IASTEnumerator, I
 				IValue val;
 				IASTExpression expr= etor.getValue();
 				if (expr != null) {
-					val= Value.create(expr, Value.MAX_RECURSION_DEPTH);
+					val= Value.create(expr);
 					previousExplicitValue = val;
 					delta = 1;
 					if (fixedType == null) {

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/IInternalVariable.java

@@ -1,25 +0,0 @@
-/*******************************************************************************
- * Copyright (c) 2008 Wind River Systems, Inc. and others.
- * All rights reserved. This program and the accompanying materials
- * are made available under the terms of the Eclipse Public License v1.0
- * which accompanies this distribution, and is available at
- * http://www.eclipse.org/legal/epl-v10.html
- *
- * Contributors:
- *     Markus Schorn - initial API and implementation
- *******************************************************************************/ 
-package org.eclipse.cdt.internal.core.dom.parser;
-
-import org.eclipse.cdt.core.dom.ast.IValue;
-import org.eclipse.cdt.core.dom.ast.IVariable;
-
-/**
- * Internal interface for bindings in the ast that have values.
- */
-public interface IInternalVariable extends IVariable {
-	/**
-	 * Returns the value of the variable, or <code>null</code>. 
-	 * If the recursion depth is reached {@link Value#UNKNOWN} will be returned.
-	 */
-	IValue getInitialValue(int maxRecursionDepth);
-}

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/Value.java

@@ -421,8 +421,8 @@ public class Value implements IValue {
 	/**
 	 * Creates the value for an expression.
 	 */
-	public static IValue create(IASTExpression expr, int maxRecursionDepth) {
+	public static IValue create(IASTExpression expr) {
-		Number val= evaluate(expr, maxRecursionDepth);
+		Number val= evaluate(expr);
 		if (val == VALUE_CANNOT_BE_DETERMINED)
 			return UNKNOWN;
 		if (val != null)
@@ -457,38 +457,38 @@ public class Value implements IValue {
 	 * Returns a {@code Number} for numerical values or {@code null}, otherwise.
 	 * @throws UnknownValueException
 	 */
-	private static Number evaluate(IASTExpression exp, int maxdepth) {
+	private static Number evaluate(IASTExpression exp) {
-		if (maxdepth < 0 || exp == null)
+		if (exp == null)
 			return VALUE_CANNOT_BE_DETERMINED;
 
 		if (exp instanceof IASTArraySubscriptExpression) {
 			return VALUE_CANNOT_BE_DETERMINED;
 		}
 		if (exp instanceof IASTBinaryExpression) {
-			return evaluateBinaryExpression((IASTBinaryExpression) exp, maxdepth);
+			return evaluateBinaryExpression((IASTBinaryExpression) exp);
 		}
 		if (exp instanceof IASTCastExpression) { // must be ahead of unary
-			return evaluate(((IASTCastExpression) exp).getOperand(), maxdepth);
+			return evaluate(((IASTCastExpression) exp).getOperand());
 		}
 		if (exp instanceof IASTUnaryExpression) {
-			return evaluateUnaryExpression((IASTUnaryExpression) exp, maxdepth);
+			return evaluateUnaryExpression((IASTUnaryExpression) exp);
 		}
 		if (exp instanceof IASTConditionalExpression) {
 			IASTConditionalExpression cexpr= (IASTConditionalExpression) exp;
-			Number v= evaluate(cexpr.getLogicalConditionExpression(), maxdepth);
+			Number v= evaluate(cexpr.getLogicalConditionExpression());
 			if (v == null || v == VALUE_CANNOT_BE_DETERMINED)
 				return v;
 			if (v.longValue() == 0) {
-				return evaluate(cexpr.getNegativeResultExpression(), maxdepth);
+				return evaluate(cexpr.getNegativeResultExpression());
 			}
 			final IASTExpression pe = cexpr.getPositiveResultExpression();
 			if (pe == null) // gnu-extension allows to omit the positive expression.
 				return v;
-			return evaluate(pe, maxdepth);
+			return evaluate(pe);
 		}
 		if (exp instanceof IASTIdExpression) {
 			IBinding b= ((IASTIdExpression) exp).getName().resolvePreBinding();
-			return evaluateBinding(b, maxdepth);
+			return evaluateBinding(b);
 		}
 		if (exp instanceof IASTLiteralExpression) {
 			IASTLiteralExpression litEx= (IASTLiteralExpression) exp;
@@ -541,7 +541,7 @@ public class Value implements IValue {
 	/**
 	 * Extract a value off a binding.
 	 */
-	private static Number evaluateBinding(IBinding b, int maxdepth) {
+	private static Number evaluateBinding(IBinding b) {
 		if (b instanceof IType) {
 			return VALUE_CANNOT_BE_DETERMINED;
 		}
@@ -554,9 +554,7 @@ public class Value implements IValue {
 		}
 
 		IValue value= null;
-		if (b instanceof IInternalVariable) {
+		if (b instanceof IVariable) {
-			value= ((IInternalVariable) b).getInitialValue(maxdepth - 1);
-		} else if (b instanceof IVariable) {
 			value= ((IVariable) b).getInitialValue();
 		} else if (b instanceof IEnumerator) {
 			value= ((IEnumerator) b).getValue();
@@ -568,7 +566,7 @@ public class Value implements IValue {
 		return VALUE_CANNOT_BE_DETERMINED;
 	}
 
-	private static Number evaluateUnaryExpression(IASTUnaryExpression exp, int maxdepth) {
+	private static Number evaluateUnaryExpression(IASTUnaryExpression exp) {
 		final int unaryOp= exp.getOperator();
 
 		if (unaryOp == IASTUnaryExpression.op_sizeof) {
@@ -591,7 +589,7 @@ public class Value implements IValue {
 			return VALUE_CANNOT_BE_DETERMINED;
 		}
 
-		final Number value= evaluate(exp.getOperand(), maxdepth);
+		final Number value= evaluate(exp.getOperand());
 		if (value == null || value == VALUE_CANNOT_BE_DETERMINED)
 			return value;
 		return applyUnaryOperator(unaryOp, value.longValue());
@@ -621,7 +619,7 @@ public class Value implements IValue {
 		return VALUE_CANNOT_BE_DETERMINED;
 	}
 
-	private static Number evaluateBinaryExpression(IASTBinaryExpression exp, int maxdepth) {
+	private static Number evaluateBinaryExpression(IASTBinaryExpression exp) {
 		final int op= exp.getOperator();
 		switch (op) {
 		case IASTBinaryExpression.op_equals:
@@ -634,10 +632,10 @@ public class Value implements IValue {
 			break;
 		}
 
-		final Number o1= evaluate(exp.getOperand1(), maxdepth);
+		final Number o1= evaluate(exp.getOperand1());
 		if (o1 == null || o1 == VALUE_CANNOT_BE_DETERMINED)
 			return o1;
-		final Number o2= evaluate(exp.getOperand2(), maxdepth);
+		final Number o2= evaluate(exp.getOperand2());
 		if (o2 == null || o2 == VALUE_CANNOT_BE_DETERMINED)
 			return o2;
 

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/c/CArrayType.java

@@ -136,7 +136,7 @@ public class CArrayType implements ICArrayType, ITypeContainer, ISerializableTyp
     	if (sizeExpression == null)
     		return value= null;
 
-    	return value= Value.create(sizeExpression, Value.MAX_RECURSION_DEPTH);
+    	return value= Value.create(sizeExpression);
     }
 
     @Override

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/c/CVariable.java

@@ -11,6 +11,9 @@
  *******************************************************************************/
 package org.eclipse.cdt.internal.core.dom.parser.c;
 
+import java.util.HashSet;
+import java.util.Set;
+
 import org.eclipse.cdt.core.dom.ILinkage;
 import org.eclipse.cdt.core.dom.ast.IASTDeclSpecifier;
 import org.eclipse.cdt.core.dom.ast.IASTDeclaration;
@@ -26,19 +29,30 @@ import org.eclipse.cdt.core.dom.ast.IBinding;
 import org.eclipse.cdt.core.dom.ast.IScope;
 import org.eclipse.cdt.core.dom.ast.IType;
 import org.eclipse.cdt.core.dom.ast.IValue;
+import org.eclipse.cdt.core.dom.ast.IVariable;
 import org.eclipse.cdt.core.parser.util.ArrayUtil;
 import org.eclipse.cdt.internal.core.dom.Linkage;
 import org.eclipse.cdt.internal.core.dom.parser.ASTQueries;
-import org.eclipse.cdt.internal.core.dom.parser.IInternalVariable;
 import org.eclipse.cdt.internal.core.dom.parser.Value;
 import org.eclipse.core.runtime.PlatformObject;
 
 /**
  * Binding for a global or a local variable, serves as base class for fields.
  */
-public class CVariable extends PlatformObject implements IInternalVariable, ICInternalBinding {
+public class CVariable extends PlatformObject implements ICInternalBinding, IVariable {
 	private IASTName[] declarations = null;
 	private IType type = null;
+	
+	/**
+	 * The set of CVariable objects for which initial value computation is in progress on each thread.
+	 * This is used to guard against recursion during initial value computation.
+	 */
+	private static final ThreadLocal<Set<CVariable>> fInitialValueInProgress = new ThreadLocal<Set<CVariable>>() {
+		@Override
+		protected Set<CVariable> initialValue() {
+			return new HashSet<>();
+		}	
+	};
 
 	public CVariable(IASTName name) {
 		declarations = new IASTName[] { name };
@@ -144,24 +158,27 @@ public class CVariable extends PlatformObject implements IInternalVariable, ICIn
 
 	@Override
 	public IValue getInitialValue() {
-		return getInitialValue(Value.MAX_RECURSION_DEPTH);
+		Set<CVariable> recursionProtectionSet = fInitialValueInProgress.get();
-	}
+		if (!recursionProtectionSet.add(this)) {
-
+			return Value.UNKNOWN;
-	@Override
+		}
-	public IValue getInitialValue(int maxDepth) {
+		try {
-		if (declarations != null) {
+			if (declarations != null) {
-			for (IASTName decl : declarations) {
+				for (IASTName decl : declarations) {
-				if (decl == null)
+					if (decl == null)
-					break;
+						break;
-				final IValue val = getInitialValue(decl, maxDepth);
+					final IValue val = getInitialValue(decl);
-				if (val != null)
+					if (val != null)
-					return val;
+						return val;
+				}
 			}
+		} finally {
+			recursionProtectionSet.remove(this);
 		}
 		return null;
 	}
 
-	private IValue getInitialValue(IASTName name, int maxDepth) {
+	private IValue getInitialValue(IASTName name) {
 		IASTDeclarator dtor = findDeclarator(name);
 		if (dtor != null) {
 			IASTInitializer init = dtor.getInitializer();
@@ -169,7 +186,7 @@ public class CVariable extends PlatformObject implements IInternalVariable, ICIn
 				final IASTInitializerClause initClause = ((IASTEqualsInitializer) init)
 						.getInitializerClause();
 				if (initClause instanceof IASTExpression) {
-					return Value.create((IASTExpression) initClause, maxDepth);
+					return Value.create((IASTExpression) initClause);
 				}
 			}
 			if (init != null)

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPASTTemplateId.java

@@ -143,7 +143,7 @@ public class CPPASTTemplateId extends CPPASTNameBase implements ICPPASTTemplateI
     			buf.append(arg.getRawSignature());
     			cleanupWhitespace= true;
     		} else if (arg instanceof IASTExpression) {
-    			IValue value= Value.create((IASTExpression) arg, Value.MAX_RECURSION_DEPTH);
+    			IValue value= Value.create((IASTExpression) arg);
     			if (value != Value.UNKNOWN && !Value.isDependentValue(value)) {
         			buf.append(value.getSignature());
     			} else {

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPArrayType.java

@@ -84,7 +84,7 @@ public class CPPArrayType implements IArrayType, ITypeContainer, ISerializableTy
     	if (sizeExpression == null)
     		return value= null;
 
-    	return value= Value.create(sizeExpression, Value.MAX_RECURSION_DEPTH);
+    	return value= Value.create(sizeExpression);
     }
 
     @Override

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPBasicType.java

@@ -69,7 +69,7 @@ public class CPPBasicType implements ICPPBasicType, ISerializableType {
 		}
 		fModifiers= qualifiers;
 		if (expression instanceof ICPPASTInitializerClause) {
-			fAssociatedValue = Value.create(expression, Value.MAX_RECURSION_DEPTH).numericalValue();
+			fAssociatedValue = Value.create(expression).numericalValue();
 		}
 	}
 

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPParameter.java

@@ -35,7 +35,6 @@ import org.eclipse.cdt.internal.core.dom.Linkage;
 import org.eclipse.cdt.internal.core.dom.parser.ASTNode;
 import org.eclipse.cdt.internal.core.dom.parser.ASTQueries;
 import org.eclipse.cdt.internal.core.dom.parser.ProblemBinding;
-import org.eclipse.cdt.internal.core.dom.parser.Value;
 import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.CPPVisitor;
 import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.SemanticUtil;
 import org.eclipse.core.runtime.PlatformObject;
@@ -246,7 +245,7 @@ public class CPPParameter extends PlatformObject implements ICPPParameter, ICPPI
 	public IValue getDefaultValue() {
 		IASTInitializer init = getInitializer();
 		if (init != null) {
-			return SemanticUtil.getValueOfInitializer(init, getType(), Value.MAX_RECURSION_DEPTH);
+			return SemanticUtil.getValueOfInitializer(init, getType());
 		}
 		return null;
 	}

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPTemplateNonTypeParameter.java

@@ -93,7 +93,7 @@ public class CPPTemplateNonTypeParameter extends CPPTemplateParameter
 		if (d == null)
 			return null;
 		
-		IValue val= Value.create(d, Value.MAX_RECURSION_DEPTH);
+		IValue val= Value.create(d);
 		IType t= getType();
 		return new CPPTemplateNonTypeArgument(val, t);
 	}

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPVariable.java

@@ -13,6 +13,9 @@
  *******************************************************************************/
 package org.eclipse.cdt.internal.core.dom.parser.cpp;
 
+import java.util.HashSet;
+import java.util.Set;
+
 import org.eclipse.cdt.core.dom.ILinkage;
 import org.eclipse.cdt.core.dom.ast.DOMException;
 import org.eclipse.cdt.core.dom.ast.IASTDeclSpecifier;
@@ -34,18 +37,28 @@ import org.eclipse.cdt.core.dom.ast.cpp.ICPPVariable;
 import org.eclipse.cdt.core.parser.util.ArrayUtil;
 import org.eclipse.cdt.internal.core.dom.Linkage;
 import org.eclipse.cdt.internal.core.dom.parser.ASTNode;
-import org.eclipse.cdt.internal.core.dom.parser.IInternalVariable;
 import org.eclipse.cdt.internal.core.dom.parser.Value;
 import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.CPPVisitor;
 import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.SemanticUtil;
 import org.eclipse.core.runtime.PlatformObject;
 
-public class CPPVariable extends PlatformObject implements ICPPVariable, ICPPInternalBinding, IInternalVariable {
+public class CPPVariable extends PlatformObject implements ICPPVariable, ICPPInternalBinding {
 	private IASTName fDefinition;
 	private IASTName fDeclarations[];
 	private IType fType;
 	private boolean fAllResolved;
 	
+	/**
+	 * The set of CPPVariable objects for which initial value computation is in progress on each thread.
+	 * This is used to guard against recursion during initial value computation.
+	 */
+	private static final ThreadLocal<Set<CPPVariable>> fInitialValueInProgress = new ThreadLocal<Set<CPPVariable>>() {
+		@Override
+		protected Set<CPPVariable> initialValue() {
+			return new HashSet<>();
+		}	
+	};
+	
 	public CPPVariable(IASTName name) {
 	    boolean isDef = name != null && name.isDefinition();
 	    if (name instanceof ICPPASTQualifiedName) {
@@ -306,34 +319,37 @@ public class CPPVariable extends PlatformObject implements ICPPVariable, ICPPInt
 	
 	@Override
 	public IValue getInitialValue() {
-		return getInitialValue(Value.MAX_RECURSION_DEPTH);
+		Set<CPPVariable> recursionProtectionSet = fInitialValueInProgress.get();
-	}
+		if (!recursionProtectionSet.add(this)) {
-	
+			return Value.UNKNOWN;
-	@Override
-	public IValue getInitialValue(int maxDepth) {
-		if (fDefinition != null) {
-			final IValue val= getInitialValue(fDefinition, maxDepth);
-			if (val != null)
-				return val;
 		}
-		if (fDeclarations != null) {
+		try {
-			for (IASTName decl : fDeclarations) {
+			if (fDefinition != null) {
-				if (decl == null)
+				final IValue val= getInitialValue(fDefinition);
-					break;
-				final IValue val= getInitialValue(decl, maxDepth);
 				if (val != null)
 					return val;
 			}
-		}		
+			if (fDeclarations != null) {
+				for (IASTName decl : fDeclarations) {
+					if (decl == null)
+						break;
+					final IValue val= getInitialValue(decl);
+					if (val != null)
+						return val;
+				}
+			}		
+		} finally {
+			recursionProtectionSet.remove(this);
+		}
 		return null;
 	}
 	
-	private IValue getInitialValue(IASTName name, int maxDepth) {
+	private IValue getInitialValue(IASTName name) {
 		IASTDeclarator dtor= findDeclarator(name);
 		if (dtor != null) {
 			IASTInitializer init= dtor.getInitializer();
 			if (init != null) {
-				return SemanticUtil.getValueOfInitializer(init, getType(), maxDepth);
+				return SemanticUtil.getValueOfInitializer(init, getType());
 			}
 		}
 		return null;

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/EvalBinding.java

@@ -39,7 +39,6 @@ import org.eclipse.cdt.core.dom.ast.cpp.ICPPTemplateParameter;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPTemplateParameterMap;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPTypeSpecialization;
 import org.eclipse.cdt.core.index.IIndexBinding;
-import org.eclipse.cdt.internal.core.dom.parser.IInternalVariable;
 import org.eclipse.cdt.internal.core.dom.parser.ISerializableEvaluation;
 import org.eclipse.cdt.internal.core.dom.parser.ITypeMarshalBuffer;
 import org.eclipse.cdt.internal.core.dom.parser.ProblemType;
@@ -317,9 +316,7 @@ public class EvalBinding extends CPPDependentEvaluation {
 
 		IValue value= null;
  		// No need to call getBinding() since a function parameter never has an initial value.
-		if (fBinding instanceof IInternalVariable) {
+		if (fBinding instanceof IVariable) {
-			value= ((IInternalVariable) fBinding).getInitialValue(Value.MAX_RECURSION_DEPTH);
-		} else if (fBinding instanceof IVariable) {
 			value= ((IVariable) fBinding).getInitialValue();
 		} else if (fBinding instanceof IEnumerator) {
 			value= ((IEnumerator) fBinding).getValue();

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/SemanticUtil.java

@@ -791,9 +791,8 @@ public class SemanticUtil {
 	 *
 	 * @param init the initializer's AST node
 	 * @param type the type of the variable
-	 * @param maxDepth maximum recursion depth
 	 */
-	public static IValue getValueOfInitializer(IASTInitializer init, IType type, int maxDepth) {
+	public static IValue getValueOfInitializer(IASTInitializer init, IType type) {
 		IASTInitializerClause clause= null;
 		if (init instanceof IASTEqualsInitializer) {
 			clause= ((IASTEqualsInitializer) init).getInitializerClause();
@@ -815,7 +814,7 @@ public class SemanticUtil {
 			}
 		}
 		if (clause instanceof IASTExpression) {
-			return Value.create((IASTExpression) clause, maxDepth);
+			return Value.create((IASTExpression) clause);
 		}
 		return Value.UNKNOWN;
 	}