Skip to content

Commit f3f7e6d

Browse files
danmardanmar
committed
value flow: replacing executionpath checking of null pointers
1 parent 43db1ee commit f3f7e6d

5 files changed

Lines changed: 102 additions & 18 deletions

File tree

‎lib/checknullpointer.cpp‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -593,13 +593,17 @@ void CheckNullPointer::nullPointerByDeRefAndChec()
593593
// Pointer dereference.
594594
bool unknown = false;
595595
if (!isPointerDeRef(tok,unknown)) {
596-
if (_settings->inconclusive && unknown && value->condition)
597-
nullPointerError(tok, tok->str(), value->condition, true);
596+
if (_settings->inconclusive && unknown) {
597+
if (value->condition == NULL)
598+
nullPointerError(tok, tok->str(), true);
599+
else
600+
nullPointerError(tok, tok->str(), value->condition, true);
601+
}
598602
continue;
599603
}
600604

601605
if (value->condition == NULL)
602-
nullPointerError(tok, tok->str());
606+
nullPointerError(tok, tok->str(), value->inconclusive);
603607
else if (_settings->isEnabled("warning"))
604608
nullPointerError(tok, tok->str(), value->condition, value->inconclusive);
605609
}
@@ -1245,9 +1249,9 @@ void CheckNullPointer::nullPointerError(const Token *tok)
12451249
reportError(tok, Severity::error, "nullPointer", "Null pointer dereference");
12461250
}
12471251

1248-
void CheckNullPointer::nullPointerError(const Token *tok, const std::string &varname)
1252+
void CheckNullPointer::nullPointerError(const Token *tok, const std::string &varname, bool inconclusive)
12491253
{
1250-
reportError(tok, Severity::error, "nullPointer", "Possible null pointer dereference: " + varname);
1254+
reportError(tok, Severity::error, "nullPointer", "Possible null pointer dereference: " + varname, inconclusive);
12511255
}
12521256

12531257
void CheckNullPointer::nullPointerError(const Token *tok, const std::string &varname, const Token* nullCheck, bool inconclusive)

‎lib/checknullpointer.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ class CPPCHECKLIB CheckNullPointer : public Check {
9999
void executionPaths();
100100

101101
void nullPointerError(const Token *tok); // variable name unknown / doesn't exist
102-
void nullPointerError(const Token *tok, const std::string &varname);
102+
void nullPointerError(const Token *tok, const std::string &varname, bool inconclusive=false);
103103
void nullPointerError(const Token *tok, const std::string &varname, const Token* nullcheck, bool inconclusive = false);
104104
void nullPointerDefaultArgError(const Token *tok, const std::string &varname);
105105
private:

‎lib/valueflow.cpp‎

Lines changed: 50 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -452,17 +452,54 @@ static void valueFlowAfterAssign(TokenList *tokenlist, ErrorLogger *errorLogger,
452452
const Variable *var = tok->astOperand1()->variable();
453453
if (!var || !var->isLocal())
454454
continue;
455+
const Token * endToken = 0;
456+
for (const Token *tok2 = var->typeStartToken(); tok2; tok2 = tok2->previous()) {
457+
if (tok2->str() == "{") {
458+
endToken = tok2->link();
459+
break;
460+
}
461+
}
455462

456463
// Lhs values..
457464
if (!tok->astOperand2() || tok->astOperand2()->values.empty())
458465
continue;
459466
std::list<ValueFlow::Value> values = tok->astOperand2()->values;
460467

461-
for (Token *tok2 = tok; tok2; tok2 = tok2->next()) {
462-
if (Token::Match(tok2, "[{}]"))
463-
break;
468+
unsigned int number_of_if = 0;
469+
470+
for (Token *tok2 = tok; tok2 && tok2 != endToken; tok2 = tok2->next()) {
464471
if (Token::Match(tok2, "sizeof|typeof|typeid ("))
465472
tok2 = tok2->linkAt(1);
473+
474+
// conditional block of code that assigns variable..
475+
if (Token::Match(tok2, "%var% (") && Token::Match(tok2->linkAt(1), ") {")) {
476+
Token * const start = tok2->linkAt(1)->next();
477+
Token * const end = start->link();
478+
// TODO: don't check noreturn scopes
479+
if (number_of_if > 0U && Token::findmatch(start, "%varid%", end, varid)) {
480+
if (settings->debugwarnings)
481+
bailout(tokenlist, errorLogger, tok2, "variable " + var->nameToken()->str() + " is assigned in conditional code");
482+
break;
483+
}
484+
if (Token::findmatch(start, "++|--| %varid% ++|--|=", end, varid)) {
485+
if (number_of_if == 0 &&
486+
Token::simpleMatch(tok2, "if (") &&
487+
!(Token::simpleMatch(end, "} else {") &&
488+
(Token::findmatch(end, "%varid%", end->linkAt(2), varid) ||
489+
Token::findmatch(end, "return|continue|break", end->linkAt(2))))) {
490+
++number_of_if;
491+
tok2 = end;
492+
} else {
493+
if (settings->debugwarnings)
494+
bailout(tokenlist, errorLogger, tok2, "variable " + var->nameToken()->str() + " is assigned in conditional code");
495+
break;
496+
}
497+
}
498+
}
499+
500+
else if (tok2->str() == "}")
501+
++number_of_if;
502+
466503
if (tok2->varId() == varid) {
467504
// bailout: assignment
468505
if (Token::Match(tok2->previous(), "!!* %var% =")) {
@@ -471,6 +508,16 @@ static void valueFlowAfterAssign(TokenList *tokenlist, ErrorLogger *errorLogger,
471508
break;
472509
}
473510

511+
// skip if variable is conditionally used in ?: expression
512+
if (const Token *parent = skipValueInConditionalExpression(tok2)) {
513+
if (settings->debugwarnings)
514+
bailout(tokenlist,
515+
errorLogger,
516+
tok2,
517+
"no simplification of " + tok2->str() + " within " + (Token::Match(parent,"[?:]") ? "?:" : parent->str()) + " expression");
518+
continue;
519+
}
520+
474521
{
475522
std::list<ValueFlow::Value>::const_iterator it;
476523
for (it = values.begin(); it != values.end(); ++it)

‎test/testnullpointer.cpp‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ class TestNullPointer : public TestFixture {
113113
"TestSimplifyTokens instead.\nstr1="+str1+"\nstr2="+str2).c_str());
114114

115115
checkNullPointer.nullConstantDereference();
116-
checkNullPointer.executionPaths();
116+
//checkNullPointer.executionPaths();
117117
}
118118

119119

@@ -881,8 +881,7 @@ class TestNullPointer : public TestFixture {
881881
ASSERT_EQUALS("[test.cpp:2]: (error) Null pointer dereference\n", errout.str());
882882

883883
{
884-
const char code[] = "static void foo(int x)\n"
885-
"{\n"
884+
const char code[] = "static void foo() {\n"
886885
" Foo<int> *abc = 0;\n"
887886
" abc->a();\n"
888887
"}\n";
@@ -893,7 +892,7 @@ class TestNullPointer : public TestFixture {
893892

894893
// inconclusive=true => error
895894
check(code, true);
896-
ASSERT_EQUALS("[test.cpp:4]: (error) Possible null pointer dereference: abc\n", errout.str());
895+
ASSERT_EQUALS("[test.cpp:3]: (error, inconclusive) Possible null pointer dereference: abc\n", errout.str());
897896
}
898897

899898
check("static void foo() {\n"
@@ -1058,7 +1057,7 @@ class TestNullPointer : public TestFixture {
10581057
// No false positive:
10591058
check("void foo() {\n"
10601059
" int n;\n"
1061-
" int *argv32;\n"
1060+
" int *argv32 = p;\n"
10621061
" if (x) {\n"
10631062
" n = 0;\n"
10641063
" argv32 = 0;\n"
@@ -1099,7 +1098,7 @@ class TestNullPointer : public TestFixture {
10991098
"\n"
11001099
" *p = 0;\n"
11011100
"}");
1102-
ASSERT_EQUALS("[test.cpp:11]: (error) Possible null pointer dereference: p\n", errout.str());
1101+
TODO_ASSERT_EQUALS("[test.cpp:11]: (error) Possible null pointer dereference: p\n", "", errout.str());
11031102
}
11041103

11051104
void nullpointer7() {
@@ -1255,7 +1254,7 @@ class TestNullPointer : public TestFixture {
12551254
" if (x) p = q;\n"
12561255
" if (y ? p->x : p->y) { }\n"
12571256
"}");
1258-
TODO_ASSERT_EQUALS("error", "", errout.str());
1257+
ASSERT_EQUALS("[test.cpp:4]: (error) Possible null pointer dereference: p\n", errout.str());
12591258
}
12601259

12611260
void nullpointer21() { // #4038 - fp: if (x) p=q; else return;
@@ -1875,7 +1874,7 @@ class TestNullPointer : public TestFixture {
18751874
" int* iVal = 0;\n"
18761875
" sscanf(dummy, \"%d%d\", foo(iVal), iVal);\n"
18771876
"}");
1878-
ASSERT_EQUALS("[test.cpp:3]: (error) Possible null pointer dereference: iVal\n", errout.str());
1877+
ASSERT_EQUALS("", errout.str());
18791878

18801879
check("void f(char* dummy) {\n"
18811880
" sscanf(dummy, \"%*d%u\", 0);\n"
@@ -1979,8 +1978,11 @@ class TestNullPointer : public TestFixture {
19791978
"[test.cpp:4]: (error) Null pointer dereference\n"
19801979
"[test.cpp:5]: (error) Null pointer dereference\n"
19811980
"[test.cpp:6]: (error) Null pointer dereference\n"
1981+
/* TODO: handle std::string
19821982
"[test.cpp:9]: (error) Possible null pointer dereference: p\n"
1983-
"[test.cpp:10]: (error) Possible null pointer dereference: p\n", errout.str());
1983+
"[test.cpp:10]: (error) Possible null pointer dereference: p\n"
1984+
*/
1985+
, errout.str());
19841986

19851987
check("void f(std::string s1, const std::string& s2, const std::string* s3) {\n"
19861988
" void* p = 0;\n"

‎test/testvalueflow.cpp‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -476,6 +476,37 @@ class TestValueFlow : public TestFixture {
476476
" a = 2 + x;\n"
477477
"}";
478478
ASSERT_EQUALS(true, testValueOfX(code, 3U, 123));
479+
480+
// if/else
481+
code = "void f() {\n"
482+
" int x = 123;\n"
483+
" if (condition) return;\n"
484+
" a = 2 + x;\n"
485+
"}";
486+
ASSERT_EQUALS(true, testValueOfX(code, 4U, 123));
487+
488+
code = "void f() {\n"
489+
" int x = 1;\n"
490+
" if (condition) x = 2;\n"
491+
" a = 2 + x;\n"
492+
"}";
493+
ASSERT_EQUALS(true, testValueOfX(code, 4U, 1));
494+
495+
code = "void f() {\n"
496+
" int x = 123;\n"
497+
" if (condition1) x = 456;\n"
498+
" if (condition2) x = 789;\n"
499+
" a = 2 + x;\n"
500+
"}";
501+
ASSERT_EQUALS(false, testValueOfX(code, 4U, 123));
502+
503+
code = "void f() {\n"
504+
" int x = 1;\n"
505+
" if (condition1) x = 2;\n"
506+
" else return;\n"
507+
" a = 2 + x;\n"
508+
"}";
509+
ASSERT_EQUALS(false, testValueOfX(code, 5U, 1));
479510
}
480511

481512
void valueFlowForLoop() {

0 commit comments

Comments
 (0)