Skip to content

Commit 3566838

Browse files
author
Zachary Blair
committed
Fixed danmar#4510 (False positive: "Possible null pointer dereference if the default parameter value is used" after init)
1 parent c3d1274 commit 3566838

3 files changed

Lines changed: 112 additions & 11 deletions

File tree

‎lib/checknullpointer.cpp‎

Lines changed: 53 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1194,6 +1194,26 @@ void CheckNullPointer::nullConstantDereference()
11941194
}
11951195
}
11961196

1197+
/**
1198+
* @brief If tok is a function call that passes in a pointer such that
1199+
* the pointer may be modified, this function will remove that
1200+
* pointer from pointerArgs.
1201+
*/
1202+
void CheckNullPointer::removeAssignedVarFromSet(const Token* tok, std::set<unsigned int>& pointerArgs)
1203+
{
1204+
// Common functions that are known NOT to modify their pointer argument
1205+
const char safeFunctions[] = "printf|sprintf|fprintf|vprintf";
1206+
1207+
// If a pointer's address is passed into a function, stop considering it
1208+
if (Token::Match(tok->previous(), "[;{}] %var% (")) {
1209+
const Token* endParen = tok->next()->link();
1210+
for (const Token* tok2 = tok->next(); tok2 != endParen; tok2 = tok2->next()) {
1211+
if (tok2->isName() && tok2->varId() > 0 && !Token::Match(tok, safeFunctions)) {
1212+
pointerArgs.erase(tok2->varId());
1213+
}
1214+
}
1215+
}
1216+
}
11971217

11981218
/**
11991219
* @brief Does one part of the check for nullPointer().
@@ -1223,8 +1243,8 @@ void CheckNullPointer::nullPointerDefaultArgument()
12231243

12241244
// Report an error if any of the default-NULL arguments are dereferenced
12251245
if (!pointerArgs.empty()) {
1226-
bool unknown = _settings->inconclusive;
12271246
for (const Token *tok = scope->classStart; tok != scope->classEnd; tok = tok->next()) {
1247+
12281248
// If we encounter a possible NULL-pointer check, skip over its body
12291249
if (Token::simpleMatch(tok, "if ( ")) {
12301250
bool dependsOnPointer = false;
@@ -1241,31 +1261,53 @@ void CheckNullPointer::nullPointerDefaultArgument()
12411261
// pointer check for the pointers referenced in its condition
12421262
const Token *endOfIf = startOfIfBlock->link();
12431263
bool isExitOrReturn =
1244-
Token::findmatch(startOfIfBlock, "exit|return", endOfIf) != NULL;
1245-
1246-
for (const Token *tok2 = tok->next(); tok2 != endOfCondition; tok2 = tok2->next()) {
1247-
if (tok2->isName() && tok2->varId() > 0 &&
1248-
pointerArgs.count(tok2->varId()) > 0) {
1264+
Token::findmatch(startOfIfBlock, "exit|return|throw", endOfIf) != NULL;
12491265

1250-
// If the if() depends on a pointer and may return, stop
1251-
// considering that pointer because it may be a NULL-pointer
1252-
// check that returns if the pointer is NULL.
1266+
if (Token::Match(tok, "if ( %var% == 0 )")) {
1267+
const unsigned int var = tok->tokAt(2)->varId();
1268+
if (var > 0 && pointerArgs.count(var) > 0) {
12531269
if (isExitOrReturn)
1254-
pointerArgs.erase(tok2->varId());
1270+
pointerArgs.erase(var);
12551271
else
12561272
dependsOnPointer = true;
12571273
}
1274+
} else {
1275+
for (const Token *tok2 = tok->next(); tok2 != endOfCondition; tok2 = tok2->next()) {
1276+
if (tok2->isName() && tok2->varId() > 0 &&
1277+
pointerArgs.count(tok2->varId()) > 0) {
1278+
1279+
// If the if() depends on a pointer and may return, stop
1280+
// considering that pointer because it may be a NULL-pointer
1281+
// check that returns if the pointer is NULL.
1282+
if (isExitOrReturn)
1283+
pointerArgs.erase(tok2->varId());
1284+
else
1285+
dependsOnPointer = true;
1286+
}
1287+
}
12581288
}
1289+
12591290
if (dependsOnPointer && endOfIf) {
12601291
for (; tok != endOfIf; tok = tok->next()) {
12611292
// If a pointer is assigned a new value, stop considering it.
12621293
if (Token::Match(tok, "%var% ="))
12631294
pointerArgs.erase(tok->varId());
1295+
else
1296+
removeAssignedVarFromSet(tok, pointerArgs);
12641297
}
12651298
continue;
12661299
}
12671300
}
12681301

1302+
// If there is a noreturn function (e.g. exit()), stop considering the rest of
1303+
// this function.
1304+
bool unknown = false;
1305+
if (Token::Match(tok, "return|throw|exit") ||
1306+
(_tokenizer->IsScopeNoReturn(tok, &unknown) && !unknown))
1307+
break;
1308+
1309+
removeAssignedVarFromSet(tok, pointerArgs);
1310+
12691311
if (tok->varId() == 0 || pointerArgs.count(tok->varId()) == 0)
12701312
continue;
12711313

@@ -1276,7 +1318,7 @@ void CheckNullPointer::nullPointerDefaultArgument()
12761318
// If a pointer dereference is preceded by an && or ||,
12771319
// they serve as a sequence point so the dereference
12781320
// may not be executed.
1279-
if (isPointerDeRef(tok, unknown) &&
1321+
if (isPointerDeRef(tok, unknown) && !unknown &&
12801322
tok->strAt(-1) != "&&" && tok->strAt(-1) != "||" &&
12811323
tok->strAt(-2) != "&&" && tok->strAt(-2) != "||")
12821324
nullPointerDefaultArgError(tok, tok->str());

‎lib/checknullpointer.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,11 @@ class CPPCHECKLIB CheckNullPointer : public Check {
152152
*/
153153
void nullPointerDefaultArgument();
154154

155+
/**
156+
* @brief Removes any variable that may be assigned from pointerArgs.
157+
*/
158+
void removeAssignedVarFromSet(const Token* tok, std::set<unsigned int>& pointerArgs);
159+
155160
/**
156161
* @brief Investigate if function call can make pointer null. If
157162
* the pointer is passed by value it can't be made a null pointer.

‎test/testnullpointer.cpp‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2110,6 +2110,13 @@ class TestNullPointer : public TestFixture {
21102110
"}");
21112111
ASSERT_EQUALS("[test.cpp:2]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", errout.str());
21122112

2113+
check("void f(int *p = 0) {\n"
2114+
" if (!p)\n"
2115+
" return;\n"
2116+
" *p = 0;\n"
2117+
"}");
2118+
ASSERT_EQUALS("", errout.str());
2119+
21132120
check("void f(char a, int *p = 0) {\n"
21142121
" *p = 0;\n"
21152122
"}");
@@ -2195,6 +2202,53 @@ class TestNullPointer : public TestFixture {
21952202
" return !p || *p;\n"
21962203
"}");
21972204
ASSERT_EQUALS("", errout.str());
2205+
2206+
2207+
// bar may initialize p but be can't know for sure without knowing
2208+
// if p is passed in by reference and is modified by bar()
2209+
check("void f(int *p = 0) {\n"
2210+
" bar(p);\n"
2211+
" *p = 0;\n"
2212+
"}");
2213+
ASSERT_EQUALS("", errout.str());
2214+
2215+
check("void f(int *p = 0) {\n"
2216+
" printf(\"%d\", p);\n"
2217+
" *p = 0;\n"
2218+
"}");
2219+
ASSERT_EQUALS("[test.cpp:3]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", errout.str());
2220+
2221+
// The init() function may or may not initialize p, but since the address
2222+
// of p is passed in, it's a good bet that p may be modified and
2223+
// so we should not report an error.
2224+
check("void f(int *p = 0) {\n"
2225+
" init(&p);\n"
2226+
" *p = 0;\n"
2227+
"}");
2228+
ASSERT_EQUALS("", errout.str());
2229+
2230+
check("void init(int* &g);\n"
2231+
"void f(int *p = 0) {\n"
2232+
" init(p);\n"
2233+
" *p = 0;\n"
2234+
"}");
2235+
ASSERT_EQUALS("", errout.str());
2236+
2237+
check("void f(int *p = 0) {\n"
2238+
" if (p == 0) {\n"
2239+
" init(&p);\n"
2240+
" }\n"
2241+
" *p = 0;\n"
2242+
"}");
2243+
ASSERT_EQUALS("", errout.str());
2244+
2245+
check("void f(int *p = 0) {\n"
2246+
" if (p == 0) {\n"
2247+
" throw SomeException;\n"
2248+
" }\n"
2249+
" *p = 0;\n"
2250+
"}");
2251+
ASSERT_EQUALS("", errout.str());
21982252
}
21992253

22002254

0 commit comments

Comments
 (0)