walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

Fix issue 8996: False positive duplicateCondition 

This fixes issue 8996 by improving the alias checking by using lifetime analysis. It also extends the lifetime checker to handle constructors and initializer lists for containers and arrays.
This commit is contained in:
Paul Fultz II 2019-03-19 06:25:10 +01:00 committed by Daniel Marjamäki
parent d3893a2b3f
commit 774464eabb
6 changed files with 275 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
lib/astutils.cpp
View File

@ -232,6 +232,18 @@ static bool isAliased(const Token * startTok, const Token * endTok, unsigned int
for (const Token *tok = startTok; tok != endTok; tok = tok->next()) {
if (Token::Match(tok, "= & %varid% ;", varid))
return true;
if (tok->varId() == varid)
continue;
if (tok->varId() == 0)
continue;
if (!astIsPointer(tok))
continue;
for (const ValueFlow::Value &val : tok->values()) {
if (!val.isLocalLifetimeValue())
continue;
if (val.tokvalue->varId() == varid)
return true;
}
}
return false;
}
@ -1024,7 +1036,13 @@ static void getArgumentsRecursive(const Token *tok, std::vector<const Token *> *
std::vector<const Token *> getArguments(const Token *ftok)
{
std::vector<const Token *> arguments;
getArgumentsRecursive(ftok->next()->astOperand2(), &arguments);
const Token *tok = ftok->next();
if (!Token::Match(tok, "(|{"))
tok = ftok;
const Token *startTok = tok->astOperand2();
if (!startTok && Token::simpleMatch(tok->astOperand1(), ","))
startTok = tok->astOperand1();
getArgumentsRecursive(startTok, &arguments);
return arguments;
}

4
lib/checkautovariables.cpp
View File

@ -619,8 +619,8 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token
if (Token::Match(tok->astParent(), "return|throw")) {
if (getPointerDepth(tok) < getPointerDepth(val.tokvalue))
continue;
if (tok->astParent()->str() == "return" && !astIsContainer(tok) && scope->function &&
mSettings->library.detectContainer(scope->function->retDef))
if (tok->astParent()->str() == "return" && !Token::simpleMatch(tok, "{") && !astIsContainer(tok) &&
astIsContainer(tok->astParent()))
continue;
if (isInScope(val.tokvalue->variable()->nameToken(), scope)) {
errorReturnDanglingLifetime(tok, &val);

17
lib/symboldatabase.cpp
View File

@ -4938,6 +4938,16 @@ void SymbolDatabase::setValueType(Token *tok, const ValueType &valuetype)
setValueType(parent, vt);
return;
}
if (parent->str() == "*" && Token::simpleMatch(parent->astOperand2(), "[") && valuetype.pointer > 0U) {
const Token *op1 = parent->astOperand2()->astOperand1();
while (op1 && op1->str() == "[")
op1 = op1->astOperand1();
ValueType vt(valuetype);
if (op1 && op1->variable() && op1->variable()->nameToken() == op1) {
setValueType(parent, vt);
return;
}
}
if (parent->str() == "&" && !parent->astOperand2()) {
ValueType vt(valuetype);
vt.pointer += 1U;
@ -5458,6 +5468,13 @@ void SymbolDatabase::setValueTypeInTokenList()
vt.sign = (vt.type == ValueType::Type::CHAR) ? mDefaultSignedness : ValueType::Sign::SIGNED;
}
setValueType(tok, vt);
} else if (tok->str() == "return" && tok->scope()) {
const Function *function = tok->scope()->function;
if (function) {
ValueType vt;
parsedecl(function->retDef, &vt, mDefaultSignedness, mSettings);
setValueType(tok, vt);
}
}
}

107
lib/valueflow.cpp
View File

@ -2769,8 +2769,42 @@ static bool isNotLifetimeValue(const ValueFlow::Value& val)
return !val.isLifetimeValue();
}
static const Variable *getLHSVariableRecursive(const Token *tok)
{
if (!tok)
return nullptr;
if (Token::Match(tok, "*|&|&&|[")) {
const Variable *var1 = getLHSVariableRecursive(tok->astOperand1());
if (var1 || Token::simpleMatch(tok, "["))
return var1;
const Variable *var2 = getLHSVariableRecursive(tok->astOperand2());
return var2;
}
if (!tok->variable())
return nullptr;
if (tok->variable()->nameToken() == tok)
return tok->variable();
return nullptr;
}
static const Variable *getLHSVariable(const Token *tok)
{
if (!Token::Match(tok, "%assign%"))
return nullptr;
if (!tok->astOperand1())
return nullptr;
if (tok->astOperand1()->varId() > 0 && tok->astOperand1()->variable())
return tok->astOperand1()->variable();
return getLHSVariableRecursive(tok->astOperand1());
}
static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings);
static void valueFlowLifetimeConstructor(Token *tok,
TokenList *tokenlist,
ErrorLogger *errorLogger,
const Settings *settings);
static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings)
{
const Token *parent = tok->astParent();
@ -2780,10 +2814,7 @@ static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLog
return;
// Assignment
if (parent->str() == "=" && (!parent->astParent() || Token::simpleMatch(parent->astParent(), ";"))) {
// Lhs should be a variable
if (!parent->astOperand1() || !parent->astOperand1()->varId())
return;
const Variable *var = parent->astOperand1()->variable();
const Variable *var = getLHSVariable(parent);
if (!var || (!var->isLocal() && !var->isGlobal() && !var->isArgument()))
return;
@ -2833,6 +2864,9 @@ static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLog
errorLogger,
settings);
}
// Constructor
} else if (Token::Match(parent->previous(), "=|return|%type%|%var% {")) {
valueFlowLifetimeConstructor(const_cast<Token *>(parent), tokenlist, errorLogger, settings);
// Function call
} else if (Token::Match(parent->previous(), "%name% (")) {
valueFlowLifetimeFunction(const_cast<Token *>(parent->previous()), tokenlist, errorLogger, settings);
@ -3069,6 +3103,67 @@ static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLog
}
}
static const Type *getTypeOf(const Token *tok)
{
if (Token::simpleMatch(tok, "return")) {
const Scope *scope = tok->scope();
if (!scope)
return nullptr;
const Function *function = scope->function;
if (!function)
return nullptr;
return function->retType;
} else if (Token::Match(tok, "%type%")) {
return tok->type();
} else if (Token::Match(tok, "%var%")) {
const Variable *var = tok->variable();
if (!var)
return nullptr;
return var->type();
} else if (Token::Match(tok, "%name%")) {
const Function *function = tok->function();
if (!function)
return nullptr;
return function->retType;
} else if (Token::simpleMatch(tok, "=")) {
return getTypeOf(tok->astOperand1());
}
return nullptr;
}
static void valueFlowLifetimeConstructor(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings)
{
if (!Token::Match(tok, "(|{"))
return;
if (const Type *t = getTypeOf(tok->previous())) {
const Scope *scope = t->classScope;
if (!scope)
return;
// Only support aggregate constructors for now
if (scope->numConstructors == 0 && t->derivedFrom.empty() && (t->isClassType() || t->isStructType())) {
std::vector<const Token *> args = getArguments(tok);
std::size_t i = 0;
for (const Variable &var : scope->varlist) {
if (i >= args.size())
break;
const Token *argtok = args[i];
LifetimeStore ls{argtok, "Passed to constructor of '" + t->name() + "'.", ValueFlow::Value::Object};
if (var.isReference() || var.isRValueReference()) {
ls.byRef(tok, tokenlist, errorLogger, settings);
} else {
ls.byVal(tok, tokenlist, errorLogger, settings);
}
}
}
} else if (Token::simpleMatch(tok, "{") && (astIsContainer(tok->astParent()) || astIsPointer(tok->astParent()))) {
std::vector<const Token *> args = getArguments(tok);
for (const Token *argtok : args) {
LifetimeStore ls{argtok, "Passed to initializer list.", ValueFlow::Value::Object};
ls.byVal(tok, tokenlist, errorLogger, settings);
}
}
}
struct Lambda {
explicit Lambda(const Token * tok)
: capture(nullptr), arguments(nullptr), returnTok(nullptr), bodyTok(nullptr) {
@ -3207,6 +3302,10 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger
valueFlowForwardLifetime(tok->tokAt(3), tokenlist, errorLogger, settings);
}
// Check constructors
else if (Token::Match(tok, "=|return|%type%|%var% {")) {
valueFlowLifetimeConstructor(tok->next(), tokenlist, errorLogger, settings);
}
// Check function calls
else if (Token::Match(tok, "%name% (")) {
valueFlowLifetimeFunction(tok, tokenlist, errorLogger, settings);

122
test/testautovariables.cpp
View File

@ -122,6 +122,8 @@ private:
TEST_CASE(danglingLifetimeContainer);
TEST_CASE(danglingLifetime);
TEST_CASE(danglingLifetimeFunction);
TEST_CASE(danglingLifetimeAggegrateConstructor);
TEST_CASE(danglingLifetimeInitList);
TEST_CASE(invalidLifetime);
}
@ -1874,6 +1876,126 @@ private:
ASSERT_EQUALS("", errout.str());
}
void danglingLifetimeAggegrateConstructor() {
check("struct A {\n"
" const int& x;\n"
" int y;\n"
"};\n"
"A f() {\n"
" int i = 0;\n"
" return A{i, i};\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:7] -> [test.cpp:6] -> [test.cpp:7]: (error) Returning object that points to local variable 'i' that will be invalid when returning.\n",
errout.str());
check("struct A {\n"
" const int& x;\n"
" int y;\n"
"};\n"
"A f() {\n"
" int i = 0;\n"
" return {i, i};\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:7] -> [test.cpp:6] -> [test.cpp:7]: (error) Returning object that points to local variable 'i' that will be invalid when returning.\n",
errout.str());
// TODO: Ast is missing for this case
check("struct A {\n"
" const int& x;\n"
" int y;\n"
"};\n"
"A f() {\n"
" int i = 0;\n"
" A r{i, i};\n"
" return r;\n"
"}\n");
TODO_ASSERT_EQUALS(
"[test.cpp:7] -> [test.cpp:6] -> [test.cpp:7]: (error) Returning object that points to local variable 'i' that will be invalid when returning.\n",
"",
errout.str());
check("struct A {\n"
" const int& x;\n"
" int y;\n"
"};\n"
"A f() {\n"
" int i = 0;\n"
" A r = {i, i};\n"
" return r;\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:7] -> [test.cpp:6] -> [test.cpp:8]: (error) Returning object that points to local variable 'i' that will be invalid when returning.\n",
errout.str());
check("struct A {\n"
" const int& x;\n"
" int y;\n"
"};\n"
"A f(int& x) {\n"
" int i = 0;\n"
" return A{i, x};\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:7] -> [test.cpp:6] -> [test.cpp:7]: (error) Returning object that points to local variable 'i' that will be invalid when returning.\n",
errout.str());
check("struct A {\n"
" const int& x;\n"
" int y;\n"
"};\n"
"A f(int& x) {\n"
" int i = 0;\n"
" return A{x, i};\n"
"}\n");
ASSERT_EQUALS("", errout.str());
check("struct A {\n"
" const int& x;\n"
" int y;\n"
"};\n"
"A f(int& x) {\n"
" return A{x, x};\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
void danglingLifetimeInitList() {
check("std::vector<int*> f() {\n"
" int i = 0;\n"
" std::vector<int*> v = {&i, &i};\n"
" return v;\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:3] -> [test.cpp:3] -> [test.cpp:2] -> [test.cpp:4]: (error) Returning object that points to local variable 'i' that will be invalid when returning.\n",
errout.str());
// TODO: Ast is missing for this case
check("std::vector<int*> f() {\n"
" int i = 0;\n"
" std::vector<int*> v{&i, &i};\n"
" return v;\n"
"}\n");
TODO_ASSERT_EQUALS(
"[test.cpp:3] -> [test.cpp:3] -> [test.cpp:2] -> [test.cpp:4]: (error) Returning object that points to local variable 'i' that will be invalid when returning.\n",
"",
errout.str());
check("std::vector<int*> f() {\n"
" int i = 0;\n"
" return {&i, &i};\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:3] -> [test.cpp:3] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning object that points to local variable 'i' that will be invalid when returning.\n",
errout.str());
check("std::vector<int*> f(int& x) {\n"
" return {&x, &x};\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
void invalidLifetime() {
check("void foo(int a) {\n"
" std::function<void()> f;\n"

12
test/testcondition.cpp
View File

@ -2959,6 +2959,18 @@ private:
" if(x == 1) {}\n"
"}\n");
ASSERT_EQUALS("", errout.str());
// #8996
check("void g(int** v);\n"
"void f() {\n"
" int a = 0;\n"
" int b = 0;\n"
" int* d[] = {&a, &b};\n"
" g(d);\n"
" if (a) {}\n"
" if (b) {}\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
void checkInvalidTestForOverflow() {