Skip to main content
summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorMarco Stornelli2019-04-06 11:03:29 -0400
committerMarco Stornelli2019-10-12 03:11:24 -0400
commit6504d1a9179f05d0b14eae83a2bc178cde2148f2 (patch)
tree1f55fad7ed8f1026a1e34cdd9bac59c7798e52d6
parentd0d6f57a50ffa1688ede261adb1ef46d423deaaf (diff)
downloadorg.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>
-rw-r--r--codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties3
-rw-r--r--codan/org.eclipse.cdt.codan.checkers/plugin.xml9
-rw-r--r--codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/ReturnChecker.java142
-rw-r--r--codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/ReturnCheckerTest.java98
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 6447ea9b5b..153d25c41f 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 0c2e1b82f6..8a5c6f2508 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 da31be8f55..55918a2ca6 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 9e24f732d7..7487bcce5b 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();
+ }
}

Back to the top