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

Fix issue 8785: ValueFlow: Track pointer alias 

This fixes the issue by making `ProgramMemory` keep track of values based on the conditions.

It also removes the `deadpointer` check since it duplicates the `invalidLifetime` check.
This commit is contained in:
Paul Fultz II 2019-09-03 17:15:58 +02:00 committed by Daniel Marjamäki
parent 1faf932206
commit 1afd56e964
5 changed files with 239 additions and 182 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

33
lib/checkuninitvar.cpp
View File

@ -1374,39 +1374,6 @@ void CheckUninitVar::valueFlowUninit()
}
}
void CheckUninitVar::deadPointer()
{
const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
// check every executable scope
for (const Scope &scope : symbolDatabase->scopeList) {
if (!scope.isExecutable())
continue;
// Dead pointers..
for (const Token* tok = scope.bodyStart; tok != scope.bodyEnd; tok = tok->next()) {
if (tok->variable() &&
tok->variable()->isPointer() &&
isVariableUsage(tok, true, NO_ALLOC)) {
const Token *alias = tok->getValueTokenDeadPointer();
if (alias) {
deadPointerError(tok,alias);
}
}
}
}
}
void CheckUninitVar::deadPointerError(const Token *pointer, const Token *alias)
{
const std::string strpointer(pointer ? pointer->str() : std::string("pointer"));
const std::string stralias(alias ? alias->expressionString() : std::string("&x"));
reportError(pointer,
Severity::error,
"deadpointer",
"$symbol:" + strpointer + "\nDead pointer usage. Pointer '$symbol' is dead if it has been assigned '" + stralias + "' at line " + MathLib::toString(alias ? alias->linenr() : 0U) + ".", CWE825, false);
}
std::string CheckUninitVar::MyFileInfo::toString() const
{
return CTU::toString(unsafeUsage);

9
lib/checkuninitvar.h
View File

@ -64,7 +64,6 @@ public:
void runChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) OVERRIDE {
CheckUninitVar checkUninitVar(tokenizer, settings, errorLogger);
checkUninitVar.check();
checkUninitVar.deadPointer();
checkUninitVar.valueFlowUninit();
}
@ -82,10 +81,6 @@ public:
bool isMemberVariableAssignment(const Token *tok, const std::string &membervar) const;
bool isMemberVariableUsage(const Token *tok, bool isPointer, Alloc alloc, const std::string &membervar) const;
/** ValueFlow-based checking for dead pointer usage */
void deadPointer();
void deadPointerError(const Token *pointer, const Token *alias);
/** ValueFlow-based checking for uninitialized variables */
void valueFlowUninit();
@ -134,7 +129,6 @@ private:
c.uninitdataError(nullptr, "varname");
c.uninitvarError(nullptr, "varname");
c.uninitStructMemberError(nullptr, "a.b");
c.deadPointerError(nullptr, nullptr);
}
static std::string myName() {
@ -144,8 +138,7 @@ private:
std::string classInfo() const OVERRIDE {
return "Uninitialized variables\n"
"- using uninitialized local variables\n"
"- using allocated data before it has been initialized\n"
"- using dead pointer\n";
"- using allocated data before it has been initialized\n";
}
};
/// @}

221
lib/valueflow.cpp
View File

@ -91,6 +91,7 @@
#include "path.h"
#include <algorithm>
#include <cassert>
#include <cstddef>
#include <functional>
#include <iterator>
@ -145,6 +146,16 @@ namespace {
bool empty() const {
return values.empty();
}
void replace(const ProgramMemory &pm) {
for (auto&& p:pm.values)
values[p.first] = p.second;
}
void insert(const ProgramMemory &pm) {
for (auto&& p:pm.values)
values.insert(p);
}
};
}
@ -218,32 +229,142 @@ static bool conditionIsTrue(const Token *condition, const ProgramMemory &program
return !error && result == 1;
}
/**
* Get program memory by looking backwards from given token.
*/
static ProgramMemory getProgramMemory(const Token *tok, nonneg int varid, const ValueFlow::Value &value)
static void setConditionalValues(const Token *tok,
bool invert,
MathLib::bigint value,
ValueFlow::Value &true_value,
ValueFlow::Value &false_value)
{
if (Token::Match(tok, "==|!=|>=|<=")) {
true_value = ValueFlow::Value{tok, value};
false_value = ValueFlow::Value{tok, value};
return;
}
const char *greaterThan = ">";
const char *lessThan = "<";
if (invert)
std::swap(greaterThan, lessThan);
if (Token::simpleMatch(tok, greaterThan)) {
true_value = ValueFlow::Value{tok, value + 1};
false_value = ValueFlow::Value{tok, value};
} else if (Token::simpleMatch(tok, lessThan)) {
true_value = ValueFlow::Value{tok, value - 1};
false_value = ValueFlow::Value{tok, value};
}
}
static const Token *parseCompareInt(const Token *tok, ValueFlow::Value &true_value, ValueFlow::Value &false_value)
{
if (!tok->astOperand1() || !tok->astOperand2())
return nullptr;
if (Token::Match(tok, "%comp%")) {
if (tok->astOperand1()->hasKnownIntValue()) {
setConditionalValues(tok, true, tok->astOperand1()->values().front().intvalue, true_value, false_value);
return tok->astOperand2();
} else if (tok->astOperand2()->hasKnownIntValue()) {
setConditionalValues(tok, false, tok->astOperand2()->values().front().intvalue, true_value, false_value);
return tok->astOperand1();
}
}
return nullptr;
}
static void programMemoryParseCondition(ProgramMemory& pm, const Token* tok, const Token* endTok, const Settings* settings, bool then)
{
if (Token::Match(tok, "==|>=|<=|<|>|!=")) {
if (then && !Token::Match(tok, "==|>=|<="))
return;
if (!then && !Token::Match(tok, "<|>|!="))
return;
ValueFlow::Value truevalue;
ValueFlow::Value falsevalue;
const Token* vartok = parseCompareInt(tok, truevalue, falsevalue);
if (!vartok)
return;
if (vartok->varId() == 0)
return;
if (!truevalue.isIntValue())
return;
if (isVariableChanged(tok->next(), endTok, vartok->varId(), false, settings, true))
return;
pm.setIntValue(vartok->varId(), then ? truevalue.intvalue : falsevalue.intvalue);
} else if (Token::Match(tok, "%var%")) {
if (tok->varId() == 0)
return;
if (then && !astIsPointer(tok) && !astIsBool(tok))
return;
if (isVariableChanged(tok->next(), endTok, tok->varId(), false, settings, true))
return;
pm.setIntValue(tok->varId(), then);
} else if (Token::simpleMatch(tok, "!")) {
programMemoryParseCondition(pm, tok->astOperand1(), endTok, settings, !then);
} else if (then && Token::simpleMatch(tok, "&&")) {
programMemoryParseCondition(pm, tok->astOperand1(), endTok, settings, then);
programMemoryParseCondition(pm, tok->astOperand2(), endTok, settings, then);
} else if (!then && Token::simpleMatch(tok, "||")) {
programMemoryParseCondition(pm, tok->astOperand1(), endTok, settings, then);
programMemoryParseCondition(pm, tok->astOperand2(), endTok, settings, then);
}
}
static void fillProgramMemoryFromConditions(ProgramMemory& pm, const Scope* scope, const Token* endTok, const Settings* settings)
{
if (!scope)
return;
if (!scope->isLocal())
return;
assert(scope != scope->nestedIn);
fillProgramMemoryFromConditions(pm, scope->nestedIn, endTok, settings);
if (scope->type == Scope::eIf || scope->type == Scope::eWhile || scope->type == Scope::eElse) {
const Token * bodyStart = scope->bodyStart;
if (scope->type == Scope::eElse) {
if (!Token::simpleMatch(bodyStart->tokAt(-2), "} else {"))
return;
bodyStart = bodyStart->linkAt(-2);
}
const Token * condEndTok = bodyStart->previous();
if (!Token::simpleMatch(condEndTok, ") {"))
return;
const Token * condStartTok = condEndTok->link();
if (!condStartTok)
return;
if (!Token::Match(condStartTok->previous(), "if|while ("))
return;
const Token * condTok = condStartTok->astOperand2();
programMemoryParseCondition(pm, condTok, endTok, settings, scope->type != Scope::eElse);
}
}
static void fillProgramMemoryFromConditions(ProgramMemory& pm, const Token* tok, const Settings* settings)
{
fillProgramMemoryFromConditions(pm, tok->scope(), tok, settings);
}
static void fillProgramMemoryFromAssignments(ProgramMemory& pm, const Token* tok, const ProgramMemory& state, std::unordered_map<nonneg int, ValueFlow::Value> vars)
{
ProgramMemory programMemory;
programMemory.setValue(varid, value);
if (value.varId)
programMemory.setIntValue(value.varId, value.varvalue);
const ProgramMemory programMemory1(programMemory);
int indentlevel = 0;
for (const Token *tok2 = tok; tok2; tok2 = tok2->previous()) {
if (Token::Match(tok2, "[;{}] %varid% = %var% ;", varid)) {
const Token *vartok = tok2->tokAt(3);
programMemory.setValue(vartok->varId(), value);
} else if (Token::Match(tok2, "[;{}] %var% =") ||
Token::Match(tok2, "[;{}] const| %type% %var% (")) {
bool setvar = false;
if (Token::Match(tok2, "[;{}] %var% = %var% ;")) {
for (auto&& p:vars) {
if (p.first != tok2->next()->varId())
continue;
const Token *vartok = tok2->tokAt(3);
pm.setValue(vartok->varId(), p.second);
setvar = true;
}
}
if (!setvar && (Token::Match(tok2, "[;{}] %var% =") ||
Token::Match(tok2, "[;{}] const| %type% %var% ("))) {
const Token *vartok = tok2->next();
while (vartok->next()->isName())
vartok = vartok->next();
if (!programMemory.hasValue(vartok->varId())) {
if (!pm.hasValue(vartok->varId())) {
MathLib::bigint result = 0;
bool error = false;
execute(vartok->next()->astOperand2(), &programMemory, &result, &error);
execute(vartok->next()->astOperand2(), &pm, &result, &error);
if (!error)
programMemory.setIntValue(vartok->varId(), result);
pm.setIntValue(vartok->varId(), result);
}
}
@ -255,15 +376,32 @@ static ProgramMemory getProgramMemory(const Token *tok, nonneg int varid, const
if (tok2->str() == "}") {
const Token *cond = tok2->link();
cond = Token::simpleMatch(cond->previous(), ") {") ? cond->linkAt(-1) : nullptr;
if (cond && conditionIsFalse(cond->astOperand2(), programMemory1))
if (cond && conditionIsFalse(cond->astOperand2(), state))
tok2 = cond->previous();
else if (cond && conditionIsTrue(cond->astOperand2(), programMemory1)) {
else if (cond && conditionIsTrue(cond->astOperand2(), state)) {
++indentlevel;
continue;
} else
break;
}
}
}
/**
* Get program memory by looking backwards from given token.
*/
static ProgramMemory getProgramMemory(const Token *tok, nonneg int varid, const ValueFlow::Value &value)
{
ProgramMemory programMemory;
if (value.tokvalue)
fillProgramMemoryFromConditions(programMemory, value.tokvalue, nullptr);
if (value.condition)
fillProgramMemoryFromConditions(programMemory, value.condition, nullptr);
programMemory.setValue(varid, value);
if (value.varId)
programMemory.setIntValue(value.varId, value.varvalue);
const ProgramMemory state = programMemory;
fillProgramMemoryFromAssignments(programMemory, tok, state, {{varid, value}});
return programMemory;
}
@ -2269,7 +2407,8 @@ static bool valueFlowForward(Token * const startToken,
falsevalues.push_back(v);
continue;
}
const ProgramMemory &programMemory = getProgramMemory(tok2, varid, v);
// TODO: Compute program from tokvalue first
ProgramMemory programMemory = getProgramMemory(tok2, varid, v);
const bool isTrue = conditionIsTrue(condTok, programMemory);
const bool isFalse = conditionIsFalse(condTok, programMemory);
@ -3819,6 +3958,8 @@ static void valueFlowForwardAssign(Token * const tok,
settings);
values.remove_if(std::mem_fn(&ValueFlow::Value::isTokValue));
}
for (ValueFlow::Value& value:values)
value.tokvalue = tok;
valueFlowForward(const_cast<Token *>(nextExpression), endOfVarScope, var, var->declarationId(), values, constValue, false, tokenlist, errorLogger, settings);
}
@ -4117,46 +4258,6 @@ struct ValueFlowConditionHandler {
}
};
static void setConditionalValues(const Token *tok,
bool invert,
MathLib::bigint value,
ValueFlow::Value &true_value,
ValueFlow::Value &false_value)
{
if (Token::Match(tok, "==|!=|>=|<=")) {
true_value = ValueFlow::Value{tok, value};
false_value = ValueFlow::Value{tok, value};
return;
}
const char *greaterThan = ">";
const char *lessThan = "<";
if (invert)
std::swap(greaterThan, lessThan);
if (Token::simpleMatch(tok, greaterThan)) {
true_value = ValueFlow::Value{tok, value + 1};
false_value = ValueFlow::Value{tok, value};
} else if (Token::simpleMatch(tok, lessThan)) {
true_value = ValueFlow::Value{tok, value - 1};
false_value = ValueFlow::Value{tok, value};
}
}
static const Token *parseCompareInt(const Token *tok, ValueFlow::Value &true_value, ValueFlow::Value &false_value)
{
if (!tok->astOperand1() || !tok->astOperand2())
return nullptr;
if (Token::Match(tok, "%comp%")) {
if (tok->astOperand1()->hasKnownIntValue()) {
setConditionalValues(tok, true, tok->astOperand1()->values().front().intvalue, true_value, false_value);
return tok->astOperand2();
} else if (tok->astOperand2()->hasKnownIntValue()) {
setConditionalValues(tok, false, tok->astOperand2()->values().front().intvalue, true_value, false_value);
return tok->astOperand1();
}
}
return nullptr;
}
static void valueFlowAfterCondition(TokenList *tokenlist,
SymbolDatabase *symboldatabase,
ErrorLogger *errorLogger,

77
test/testautovariables.cpp
View File

@ -127,6 +127,7 @@ private:
TEST_CASE(danglingLifetimeInitList);
TEST_CASE(danglingLifetimeImplicitConversion);
TEST_CASE(invalidLifetime);
TEST_CASE(deadPointer);
}
@ -2192,6 +2193,82 @@ private:
ASSERT_EQUALS("", errout.str());
}
void deadPointer() {
check("void f() {\n"
" int *p = p1;\n"
" if (cond) {\n"
" int x;\n"
" p = &x;\n"
" }\n"
" *p = 0;\n"
"}");
ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:4] -> [test.cpp:7]: (error) Using pointer to local variable 'x' that is out of scope.\n", errout.str());
// FP: don't warn in subfunction
check("void f(struct KEY *key) {\n"
" key->x = 0;\n"
"}\n"
"\n"
"int main() {\n"
" struct KEY *tmp = 0;\n"
" struct KEY k;\n"
"\n"
" if (condition) {\n"
" tmp = &k;\n"
" } else {\n"
" }\n"
" f(tmp);\n"
"}");
ASSERT_EQUALS("", errout.str());
// Don't warn about references (#6399)
check("void f() {\n"
" wxAuiToolBarItem* former_hover = NULL;\n"
" for (i = 0, count = m_items.GetCount(); i < count; ++i) {\n"
" wxAuiToolBarItem& item = m_items.Item(i);\n"
" former_hover = &item;\n"
" }\n"
" if (former_hover != pitem)\n"
" dosth();\n"
"}");
ASSERT_EQUALS("", errout.str());
check("void f() {\n"
" wxAuiToolBarItem* former_hover = NULL;\n"
" for (i = 0, count = m_items.GetCount(); i < count; ++i) {\n"
" wxAuiToolBarItem item = m_items.Item(i);\n"
" former_hover = &item;\n"
" }\n"
" if (former_hover != pitem)\n"
" dosth();\n"
"}");
ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:4] -> [test.cpp:7]: (error) Using pointer to local variable 'item' that is out of scope.\n", errout.str());
// #6575
check("void trp_deliver_signal() {\n"
" union {\n"
" Uint32 theData[25];\n"
" EventReport repData;\n"
" };\n"
" EventReport * rep = &repData;\n"
" rep->setEventType(NDB_LE_Connected);\n"
"}");
ASSERT_EQUALS("", errout.str());
// #8785
check("int f(bool a, bool b) {\n"
" int *iPtr = 0;\n"
" if(b) {\n"
" int x = 42;\n"
" iPtr = &x;\n"
" }\n"
" if(b && a)\n"
" return *iPtr;\n"
" return 0;\n"
"}\n");
ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:4] -> [test.cpp:8]: (error) Using pointer to local variable 'x' that is out of scope.\n", errout.str());
}
};
REGISTER_TEST(TestAutoVariables)

81
test/testuninitvar.cpp
View File

@ -87,9 +87,6 @@ private:
TEST_CASE(isVariableUsageDeref); // *p
// dead pointer
TEST_CASE(deadPointer);
// whole program analysis
TEST_CASE(ctu);
}
@ -3934,21 +3931,6 @@ private:
ASSERT_EQUALS("", errout.str());
}
void checkDeadPointer(const char code[]) {
// Clear the error buffer..
errout.str("");
// Tokenize..
Tokenizer tokenizer(&settings, this);
std::istringstream istr(code);
tokenizer.tokenize(istr, "test.cpp");
// Check code..
CheckUninitVar check(&tokenizer, &settings, this);
check.deadPointer();
}
void valueFlowUninit(const char code[]) {
// Clear the error buffer..
errout.str("");
@ -4513,69 +4495,6 @@ private:
ASSERT_EQUALS("", errout.str());
}
void deadPointer() {
checkDeadPointer("void f() {\n"
" int *p = p1;\n"
" if (cond) {\n"
" int x;\n"
" p = &x;\n"
" }\n"
" *p = 0;\n"
"}");
ASSERT_EQUALS("[test.cpp:7]: (error) Dead pointer usage. Pointer 'p' is dead if it has been assigned '&x' at line 5.\n", errout.str());
// FP: don't warn in subfunction
checkDeadPointer("void f(struct KEY *key) {\n"
" key->x = 0;\n"
"}\n"
"\n"
"int main() {\n"
" struct KEY *tmp = 0;\n"
" struct KEY k;\n"
"\n"
" if (condition) {\n"
" tmp = &k;\n"
" } else {\n"
" }\n"
" f(tmp);\n"
"}");
ASSERT_EQUALS("", errout.str());
// Don't warn about references (#6399)
checkDeadPointer("void f() {\n"
" wxAuiToolBarItem* former_hover = NULL;\n"
" for (i = 0, count = m_items.GetCount(); i < count; ++i) {\n"
" wxAuiToolBarItem& item = m_items.Item(i);\n"
" former_hover = &item;\n"
" }\n"
" if (former_hover != pitem)\n"
" dosth();\n"
"}");
ASSERT_EQUALS("", errout.str());
checkDeadPointer("void f() {\n"
" wxAuiToolBarItem* former_hover = NULL;\n"
" for (i = 0, count = m_items.GetCount(); i < count; ++i) {\n"
" wxAuiToolBarItem item = m_items.Item(i);\n"
" former_hover = &item;\n"
" }\n"
" if (former_hover != pitem)\n"
" dosth();\n"
"}");
ASSERT_EQUALS("[test.cpp:7]: (error) Dead pointer usage. Pointer 'former_hover' is dead if it has been assigned '&item' at line 5.\n", errout.str());
// #6575
checkDeadPointer("void trp_deliver_signal() {\n"
" union {\n"
" Uint32 theData[25];\n"
" EventReport repData;\n"
" };\n"
" EventReport * rep = &repData;\n"
" rep->setEventType(NDB_LE_Connected);\n"
"}");
ASSERT_EQUALS("", errout.str());
}
void ctu(const char code[]) {
// Clear the error buffer..
errout.str("");