Bug 546173 - Add a check for returning of local variable addresses

Change-Id: Ief17af55c20b6e075381fa22a9208b7dfa67ec0b
Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>

codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties

@@ -48,6 +48,9 @@ problem.name.UnusedReturnValue = Unused return value
 problem.description.NoReturn = No return statement in a function which is declared to return value
 problem.messagePattern.NoReturn = No return, in function returning non-void
 problem.name.NoReturn = No return
+problem.description.LocalVarReturn = Returning the address of a local variable
+problem.messagePattern.LocalVarReturn = Returning the address of local variable ''{0}''
+problem.name.LocalVarReturn = Returning the address of a local variable
 checker.name.FormatString = Format String
 problem.description.FormatString = Finds statements lead to format string vulnerability (e.g. 'char[5] str; scanf("%10s", str);'\u0020
 problem.messagePattern.FormatString = Format string vulnerability in ''{0}''

codan/org.eclipse.cdt.codan.checkers/plugin.xml

@@ -119,6 +119,15 @@
                messagePattern="%problem.messagePattern.NoReturn"
                name="%problem.name.NoReturn">
          </problem>
+         <problem
+               category="org.eclipse.cdt.codan.core.categories.ProgrammingProblems"
+               defaultEnabled="false"
+               defaultSeverity="Warning"
+               description="%problem.description.LocalVarReturn"
+               id="org.eclipse.cdt.codan.checkers.localvarreturn"
+               messagePattern="%problem.messagePattern.LocalVarReturn"
+               name="%problem.name.LocalVarReturn">
+         </problem>
       </checker>
       <checker
             class="org.eclipse.cdt.codan.internal.checkers.ProblemBindingChecker"

codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/ReturnChecker.java

@@ -17,7 +17,9 @@ package org.eclipse.cdt.codan.internal.checkers;
 
 import java.util.Collection;
 import java.util.Iterator;
+import java.util.Stack;
 
+import org.eclipse.cdt.codan.checkers.CodanCheckersActivator;
 import org.eclipse.cdt.codan.core.cxx.CxxAstUtils;
 import org.eclipse.cdt.codan.core.cxx.model.AbstractAstFunctionChecker;
 import org.eclipse.cdt.codan.core.model.IProblem;
@@ -28,32 +30,51 @@ import org.eclipse.cdt.codan.core.model.cfg.IControlFlowGraph;
 import org.eclipse.cdt.codan.core.model.cfg.IExitNode;
 import org.eclipse.cdt.codan.internal.core.cfg.ControlFlowGraph;
 import org.eclipse.cdt.core.dom.ast.ASTVisitor;
+import org.eclipse.cdt.core.dom.ast.DOMException;
+import org.eclipse.cdt.core.dom.ast.EScopeKind;
+import org.eclipse.cdt.core.dom.ast.IASTCastExpression;
 import org.eclipse.cdt.core.dom.ast.IASTCompoundStatement;
+import org.eclipse.cdt.core.dom.ast.IASTConditionalExpression;
 import org.eclipse.cdt.core.dom.ast.IASTDeclSpecifier;
 import org.eclipse.cdt.core.dom.ast.IASTDeclaration;
+import org.eclipse.cdt.core.dom.ast.IASTDeclarator;
 import org.eclipse.cdt.core.dom.ast.IASTDoStatement;
 import org.eclipse.cdt.core.dom.ast.IASTExpression;
+import org.eclipse.cdt.core.dom.ast.IASTFieldReference;
 import org.eclipse.cdt.core.dom.ast.IASTForStatement;
 import org.eclipse.cdt.core.dom.ast.IASTFunctionDefinition;
 import org.eclipse.cdt.core.dom.ast.IASTGotoStatement;
+import org.eclipse.cdt.core.dom.ast.IASTIdExpression;
 import org.eclipse.cdt.core.dom.ast.IASTIfStatement;
 import org.eclipse.cdt.core.dom.ast.IASTInitializerClause;
 import org.eclipse.cdt.core.dom.ast.IASTLabelStatement;
+import org.eclipse.cdt.core.dom.ast.IASTPointerOperator;
 import org.eclipse.cdt.core.dom.ast.IASTReturnStatement;
 import org.eclipse.cdt.core.dom.ast.IASTSimpleDeclSpecifier;
 import org.eclipse.cdt.core.dom.ast.IASTStatement;
 import org.eclipse.cdt.core.dom.ast.IASTSwitchStatement;
+import org.eclipse.cdt.core.dom.ast.IASTTypeId;
+import org.eclipse.cdt.core.dom.ast.IASTUnaryExpression;
 import org.eclipse.cdt.core.dom.ast.IASTWhileStatement;
 import org.eclipse.cdt.core.dom.ast.IBasicType;
 import org.eclipse.cdt.core.dom.ast.IBinding;
+import org.eclipse.cdt.core.dom.ast.IFunction;
+import org.eclipse.cdt.core.dom.ast.IParameter;
+import org.eclipse.cdt.core.dom.ast.IPointerType;
+import org.eclipse.cdt.core.dom.ast.IScope;
 import org.eclipse.cdt.core.dom.ast.IType;
+import org.eclipse.cdt.core.dom.ast.IVariable;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTFunctionDefinition;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTLambdaExpression;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTReferenceOperator;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTTryBlockStatement;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPConstructor;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPField;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPMethod;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPReferenceType;
 import org.eclipse.cdt.core.parser.util.CharArrayUtils;
 import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.CPPTemplates;
+import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.SemanticUtil;
 
 /**
  * The checker suppose to find issue related to mismatched return value/function
@@ -62,16 +83,115 @@ import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.CPPTemplates;
  * <li>Function declared as returning void has non-void return
  * <li>Function declared as returning non-void has no return (requires control flow graph)
  */
+@SuppressWarnings("restriction")
 public class ReturnChecker extends AbstractAstFunctionChecker {
 	public static final String PARAM_IMPLICIT = "implicit"; //$NON-NLS-1$
 	public static final String RET_NO_VALUE_ID = "org.eclipse.cdt.codan.checkers.noreturn"; //$NON-NLS-1$
 	public static final String RET_ERR_VALUE_ID = "org.eclipse.cdt.codan.checkers.errreturnvalue"; //$NON-NLS-1$
 	public static final String RET_NORET_ID = "org.eclipse.cdt.codan.checkers.errnoreturn"; //$NON-NLS-1$
+	public static final String RET_LOCAL_ID = "org.eclipse.cdt.codan.checkers.localvarreturn"; //$NON-NLS-1$
 
 	private IType cachedReturnType = null;
 
+	private enum RetType {
+		BY_REF, BY_PTR
+	}
+
+	private class ReturnTypeAnalyzer {
+		private RetType retType;
+		private Stack<Integer> innermostOp;
+
+		public ReturnTypeAnalyzer(RetType t) {
+			retType = t;
+			innermostOp = new Stack<>();
+		}
+
+		public RetType getType() {
+			return retType;
+		}
+
+		public void visit(IASTInitializerClause expr) {
+			if (expr instanceof IASTCastExpression) {
+				visit((IASTCastExpression) expr);
+			} else if (expr instanceof IASTConditionalExpression) {
+				visit((IASTConditionalExpression) expr);
+			} else if (expr instanceof IASTIdExpression) {
+				visit((IASTIdExpression) expr);
+			} else if (expr instanceof IASTUnaryExpression) {
+				visit((IASTUnaryExpression) expr);
+			} else if (expr instanceof IASTFieldReference) {
+				visit((IASTFieldReference) expr);
+			}
+		}
+
+		private void visit(IASTFieldReference expr) {
+			visit(expr.getFieldOwner());
+		}
+
+		private void visit(IASTCastExpression expr) {
+			IASTTypeId id = expr.getTypeId();
+			IASTDeclarator declarator = id.getAbstractDeclarator();
+			IASTPointerOperator[] ptr = declarator.getPointerOperators();
+			if (ptr.length > 0 && ptr[0] instanceof ICPPASTReferenceOperator) {
+				innermostOp.push(IASTUnaryExpression.op_amper);
+			}
+			visit(expr.getOperand());
+			if (ptr.length > 0 && ptr[0] instanceof ICPPASTReferenceOperator) {
+				innermostOp.pop();
+			}
+		}
+
+		private void visit(IASTConditionalExpression expr) {
+			visit(expr.getPositiveResultExpression());
+			visit(expr.getNegativeResultExpression());
+		}
+
+		private void visit(IASTIdExpression expr) {
+			IBinding binding = expr.getName().resolveBinding();
+			if (binding instanceof IVariable && !(binding instanceof IParameter) && !(binding instanceof ICPPField)) {
+				Integer op = null;
+				if (!innermostOp.empty())
+					op = innermostOp.peek();
+				IType t = ((IVariable) binding).getType();
+				if (((IVariable) binding).isStatic()) {
+					return;
+				}
+				t = SemanticUtil.getNestedType(t, SemanticUtil.TDEF);
+				if (retType == RetType.BY_REF && !(t instanceof ICPPReferenceType)) {
+					if (t instanceof IPointerType && op != null && op == IASTUnaryExpression.op_star) {
+						return;
+					}
+					try {
+						IScope scope = binding.getScope();
+						if (scope.getKind() == EScopeKind.eLocal) {
+							reportProblem(RET_LOCAL_ID, expr, binding.getName());
+						}
+					} catch (DOMException e) {
+						CodanCheckersActivator.log(e);
+					}
+				} else if (retType == RetType.BY_PTR && op != null && op == IASTUnaryExpression.op_amper) {
+					try {
+						IScope scope = binding.getScope();
+						if (scope.getKind() == EScopeKind.eLocal) {
+							reportProblem(RET_LOCAL_ID, expr, binding.getName());
+						}
+					} catch (DOMException e) {
+						CodanCheckersActivator.log(e);
+					}
+				}
+			}
+		}
+
+		private void visit(IASTUnaryExpression expr) {
+			innermostOp.push(expr.getOperator());
+			visit(expr.getOperand());
+			innermostOp.pop();
+		}
+	}
+
 	class ReturnStmpVisitor extends ASTVisitor {
 		private final IASTFunctionDefinition func;
+		private final ReturnTypeAnalyzer analyzer;
 		boolean hasret;
 
 		ReturnStmpVisitor(IASTFunctionDefinition func) {
@@ -80,6 +200,24 @@ public class ReturnChecker extends AbstractAstFunctionChecker {
 			shouldVisitExpressions = true;
 			this.func = func;
 			this.hasret = false;
+			IBinding binding = func.getDeclarator().getName().resolveBinding();
+			if (binding instanceof IFunction) {
+				IType retType = SemanticUtil.getNestedType(((IFunction) binding).getType().getReturnType(),
+						SemanticUtil.TDEF);
+				if (retType instanceof ICPPReferenceType) {
+					analyzer = new ReturnTypeAnalyzer(RetType.BY_REF);
+				} else if (retType instanceof IPointerType) {
+					analyzer = new ReturnTypeAnalyzer(RetType.BY_PTR);
+				} else
+					analyzer = null;
+			} else
+				analyzer = null;
+		}
+
+		RetType getType() {
+			if (analyzer != null)
+				return analyzer.getType();
+			return null;
 		}
 
 		@Override
@@ -110,6 +248,8 @@ public class ReturnChecker extends AbstractAstFunctionChecker {
 					if (checkImplicitReturn(RET_NO_VALUE_ID) || isExplicitReturn(func)) {
 						if (returnValue == null)
 							reportProblem(RET_NO_VALUE_ID, ret);
+						else if (analyzer != null)
+							analyzer.visit(returnValue);
 					}
 				} else if (returnKind == ReturnTypeKind.Void) {
 					if (returnValue instanceof IASTExpression) {
@@ -190,7 +330,6 @@ public class ReturnChecker extends AbstractAstFunctionChecker {
 		return false;
 	}
 
-	@SuppressWarnings("restriction")
 	// TODO: Any reason not to just expose getDeadNodes() in IControlFlowGraph?
 	public Collection<IBasicBlock> getDeadBlocks(IASTFunctionDefinition func) {
 		IControlFlowGraph graph = getModelCache().getControlFlowGraph(func);
@@ -263,7 +402,6 @@ public class ReturnChecker extends AbstractAstFunctionChecker {
 	 * @param func the function to check
 	 * @return {@code true} if the function has a non void return type
 	 */
-	@SuppressWarnings("restriction")
 	private ReturnTypeKind getReturnTypeKind(IASTFunctionDefinition func) {
 		if (isConstructorDestructor(func))
 			return ReturnTypeKind.Void;

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

@@ -26,7 +26,8 @@ public class ReturnCheckerTest extends CheckerTestCase {
 	@Override
 	public void setUp() throws Exception {
 		super.setUp();
-		enableProblems(ReturnChecker.RET_NORET_ID, ReturnChecker.RET_ERR_VALUE_ID, ReturnChecker.RET_NO_VALUE_ID);
+		enableProblems(ReturnChecker.RET_NORET_ID, ReturnChecker.RET_ERR_VALUE_ID, ReturnChecker.RET_NO_VALUE_ID,
+				ReturnChecker.RET_LOCAL_ID);
 	}
 
 	@Override
@@ -527,4 +528,99 @@ public class ReturnCheckerTest extends CheckerTestCase {
 	public void testNoReturnWithGoto_Bug492878() throws Exception {
 		checkSampleAboveCpp();
 	}
+
+	//	int& bar() {
+	//		int a = 0;
+	//		return a; //error here
+	//	}
+	public void testReturnByRef_Bug546173() throws Exception {
+		checkSampleAboveCpp();
+	}
+
+	//	int* bar() {
+	//		int a = 0;
+	//		return &a; //error here
+	//	}
+	public void testReturnByPtr_Bug546173() throws Exception {
+		checkSampleAboveCpp();
+	}
+
+	//	int& bar() {
+	//		int a = 0;
+	//		return reinterpret_cast<int&>(a); //error here
+	//	}
+	public void testReturnByCastRef_Bug546173() throws Exception {
+		checkSampleAboveCpp();
+	}
+
+	//	int* bar() {
+	//		int a = 0;
+	//		return reinterpret_cast<int*>(a);
+	//	}
+	public void testReturnByCastPtr_Bug546173() throws Exception {
+		checkSampleAboveCpp();
+	}
+
+	//	int* bar() {
+	//		int a = 0, b = 0;
+	//		bool cond = true;
+	//		return cond ? &a : //error here
+	//					  &b; //error here
+	//	}
+	public void testReturnByTernary_Bug546173() throws Exception {
+		checkSampleAboveCpp();
+	}
+
+	//	struct S { int a; }
+	//	int& bar() {
+	//		struct S s;
+	//		return s.a; //error here
+	//	}
+	public void testReturnLocalStructField_Bug546173() throws Exception {
+		checkSampleAboveCpp();
+	}
+
+	//	class Test {
+	//	private:
+	//		int field;
+	//	public:
+	//		int& bar() {
+	//			return field;
+	//		}
+	//	}
+	public void testReturnClassField_Bug546173() throws Exception {
+		checkSampleAboveCpp();
+	}
+
+	//	class Test {
+	//	private:
+	//		int field;
+	//	public:
+	//		void foo(double*);
+	//		void (Test::*bar())(double*) {
+	//			 return &Test::foo;
+	//		}
+	//	}
+	public void testReturnClassMethod_Bug546173() throws Exception {
+		checkSampleAboveCpp();
+	}
+
+	//int& foo() {
+	//	int* a = new int;
+	//	return *a;
+	//}
+	public void testReturnRefUsingDerefPtr_Bug546173() throws Exception {
+		checkSampleAboveCpp();
+	}
+
+	//void foo() {
+	//	int local;
+	//	auto s = [&local]() {
+	//	    return &local;  // ok
+	//	};
+	//	int* ptr = s();
+	//}
+	public void testReturnLambda_Bug546173() throws Exception {
+		checkSampleAboveCpp();
+	}
 }