diff options
author | Marco Stornelli | 2019-04-06 15:03:29 +0000 |
---|---|---|
committer | Marco Stornelli | 2019-10-12 07:11:24 +0000 |
commit | 6504d1a9179f05d0b14eae83a2bc178cde2148f2 (patch) | |
tree | 1f55fad7ed8f1026a1e34cdd9bac59c7798e52d6 | |
parent | d0d6f57a50ffa1688ede261adb1ef46d423deaaf (diff) | |
download | org.eclipse.cdt-6504d1a9179f05d0b14eae83a2bc178cde2148f2.tar.gz org.eclipse.cdt-6504d1a9179f05d0b14eae83a2bc178cde2148f2.tar.xz org.eclipse.cdt-6504d1a9179f05d0b14eae83a2bc178cde2148f2.zip |
Bug 546173 - Add a check for returning of local variable addresses
Change-Id: Ief17af55c20b6e075381fa22a9208b7dfa67ec0b
Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
4 files changed, 249 insertions, 3 deletions
diff --git a/codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties b/codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties index 6447ea9b5b3..153d25c41ff 100644 --- a/codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties +++ b/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}'' diff --git a/codan/org.eclipse.cdt.codan.checkers/plugin.xml b/codan/org.eclipse.cdt.codan.checkers/plugin.xml index 0c2e1b82f68..8a5c6f25084 100644 --- a/codan/org.eclipse.cdt.codan.checkers/plugin.xml +++ b/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" diff --git a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/ReturnChecker.java b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/ReturnChecker.java index da31be8f559..55918a2ca65 100644 --- a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/ReturnChecker.java +++ b/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; diff --git a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/ReturnCheckerTest.java b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/ReturnCheckerTest.java index 9e24f732d79..7487bcce5bb 100644 --- a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/ReturnCheckerTest.java +++ b/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(); + } } |