Skip to content

Commit 166bd2b

Browse files
pfultz2danmar
pfultz2
authored and
danmar
committed
Fix issue 2153: valueFlowAfterCondition: struct member (#2228)
* Fix issue 2153: valueFlowAfterCondition: struct member * Fix null pointer dereference * Formatting * Check for another null pointer * Initialize variables * Remove redundant condition * Format * Add missing initialization to copy constructor * Format
1 parent b4af8bd commit 166bd2b

9 files changed

Lines changed: 193 additions & 31 deletions

‎lib/astutils.cpp‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,8 @@ bool astIsPointer(const Token *tok)
138138
return tok && tok->valueType() && tok->valueType()->pointer;
139139
}
140140

141+
bool astIsSmartPointer(const Token* tok) { return tok && tok->valueType() && tok->valueType()->smartPointerTypeToken; }
142+
141143
bool astIsIterator(const Token *tok)
142144
{
143145
return tok && tok->valueType() && tok->valueType()->type == ValueType::Type::ITERATOR;
@@ -335,7 +337,7 @@ bool isAliased(const Variable *var)
335337
return isAliased(start, var->scope()->bodyEnd, var->declarationId());
336338
}
337339

338-
static bool exprDependsOnThis(const Token *expr, nonneg int depth)
340+
bool exprDependsOnThis(const Token* expr, nonneg int depth)
339341
{
340342
if (!expr)
341343
return false;
@@ -353,7 +355,12 @@ static bool exprDependsOnThis(const Token *expr, nonneg int depth)
353355
nestedIn = nestedIn->nestedIn;
354356
}
355357
return nestedIn == expr->function()->nestedIn;
358+
} else if (Token::Match(expr, "%var%") && expr->variable()) {
359+
const Variable* var = expr->variable();
360+
return (var->isPrivate() || var->isPublic() || var->isProtected());
356361
}
362+
if (Token::simpleMatch(expr, "."))
363+
return exprDependsOnThis(expr->astOperand1(), depth);
357364
return exprDependsOnThis(expr->astOperand1(), depth) || exprDependsOnThis(expr->astOperand2(), depth);
358365
}
359366

@@ -381,7 +388,7 @@ static const Token * followVariableExpression(const Token * tok, bool cpp, const
381388
if (!varTok)
382389
return tok;
383390
// Bailout. If variable value depends on value of "this".
384-
if (exprDependsOnThis(varTok, 0))
391+
if (exprDependsOnThis(varTok))
385392
return tok;
386393
// Skip array access
387394
if (Token::simpleMatch(varTok, "["))
@@ -1743,10 +1750,10 @@ struct FwdAnalysis::Result FwdAnalysis::checkRecursive(const Token *expr, const
17431750
if (exprVarIds.find(tok->varId()) != exprVarIds.end()) {
17441751
const Token *parent = tok;
17451752
bool other = false;
1746-
bool same = tok->astParent() && isSameExpression(mCpp, false, expr, tok, mLibrary, false, false, nullptr);
1747-
while (!same && Token::Match(parent->astParent(), "*|.|::|[|%cop%")) {
1753+
bool same = tok->astParent() && isSameExpression(mCpp, false, expr, tok, mLibrary, true, false, nullptr);
1754+
while (!same && Token::Match(parent->astParent(), "*|.|::|[|(|%cop%")) {
17481755
parent = parent->astParent();
1749-
if (parent && isSameExpression(mCpp, false, expr, parent, mLibrary, false, false, nullptr)) {
1756+
if (parent && isSameExpression(mCpp, false, expr, parent, mLibrary, true, false, nullptr)) {
17501757
same = true;
17511758
if (mWhat == What::ValueFlow) {
17521759
KnownAndToken v;

‎lib/astutils.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,8 @@ bool astIsBool(const Token *tok);
6767

6868
bool astIsPointer(const Token *tok);
6969

70+
bool astIsSmartPointer(const Token* tok);
71+
7072
bool astIsIterator(const Token *tok);
7173

7274
bool astIsContainer(const Token *tok);
@@ -92,6 +94,8 @@ const Token* astParentSkipParens(const Token* tok);
9294

9395
bool precedes(const Token * tok1, const Token * tok2);
9496

97+
bool exprDependsOnThis(const Token* expr, nonneg int depth = 0);
98+
9599
bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2, const Library& library, bool pure, bool followVar, ErrorPath* errors=nullptr);
96100

97101
bool isEqualKnownValue(const Token * const tok1, const Token * const tok2);

‎lib/checkio.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -956,7 +956,7 @@ void CheckIO::checkFormatString(const Token * const tok,
956956
if (argListTok->tokType() != Token::eString &&
957957
argInfo.isKnownType() && !argInfo.isArrayOrPointer()) {
958958
if (!Token::Match(argInfo.typeToken, "char|wchar_t")) {
959-
if (!(!argInfo.isArrayOrPointer() && argInfo.element))
959+
if (!argInfo.element)
960960
invalidPrintfArgTypeError_s(tok, numFormat, &argInfo);
961961
}
962962
}

‎lib/checknullpointer.cpp‎

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,31 @@ void CheckNullPointer::nullPointerLinkedList()
318318
}
319319
}
320320

321+
static bool isNullablePointer(const Token* tok, const Settings* settings)
322+
{
323+
if (!tok)
324+
return false;
325+
if (astIsPointer(tok))
326+
return true;
327+
if (astIsSmartPointer(tok))
328+
return true;
329+
// TODO: Move this logic into ValueType
330+
if (Token::simpleMatch(tok, "."))
331+
return isNullablePointer(tok->astOperand2(), settings);
332+
if (const Variable* var = tok->variable()) {
333+
return (var->isPointer() || var->isSmartPointer());
334+
}
335+
if (Token::Match(tok->previous(), "%name% (")) {
336+
if (const Function* f = tok->previous()->function()) {
337+
if (f->retDef) {
338+
ValueType vt = ValueType::parseDecl(f->retDef, settings);
339+
return vt.smartPointerTypeToken || vt.pointer > 0;
340+
}
341+
}
342+
}
343+
return false;
344+
}
345+
321346
void CheckNullPointer::nullPointerByDeRefAndChec()
322347
{
323348
const bool printInconclusive = (mSettings->inconclusive);
@@ -328,11 +353,10 @@ void CheckNullPointer::nullPointerByDeRefAndChec()
328353
continue;
329354
}
330355

331-
const Variable *var = tok->variable();
332-
if (!var || tok == var->nameToken())
356+
if (Token::Match(tok, "%num%|%char%|%str%"))
333357
continue;
334358

335-
if (!var->isPointer() && !var->isSmartPointer())
359+
if (!isNullablePointer(tok, mSettings))
336360
continue;
337361

338362
// Can pointer be NULL?
@@ -347,11 +371,11 @@ void CheckNullPointer::nullPointerByDeRefAndChec()
347371
bool unknown = false;
348372
if (!isPointerDeRef(tok,unknown)) {
349373
if (unknown)
350-
nullPointerError(tok, tok->str(), value, true);
374+
nullPointerError(tok, tok->expressionString(), value, true);
351375
continue;
352376
}
353377

354-
nullPointerError(tok, tok->str(), value, value->isInconclusive());
378+
nullPointerError(tok, tok->expressionString(), value, value->isInconclusive());
355379
}
356380
}
357381

‎lib/checkuninitvar.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1101,7 +1101,7 @@ bool CheckUninitVar::isVariableUsage(const Token *vartok, bool pointer, Alloc al
11011101
if (alloc == NO_ALLOC && vartok->next() && vartok->next()->isOp() && !vartok->next()->isAssignmentOp())
11021102
return true;
11031103

1104-
if (alloc == NO_ALLOC && vartok->next()->isAssignmentOp() && vartok->next()->str() != "=")
1104+
if (alloc == NO_ALLOC && vartok->next() && vartok->next()->isAssignmentOp() && vartok->next()->str() != "=")
11051105
return true;
11061106

11071107
if (vartok->strAt(1) == "]")

‎lib/symboldatabase.cpp‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5324,6 +5324,12 @@ static const Token * parsedecl(const Token *type, ValueType * const valuetype, V
53245324
type = type->next();
53255325
}
53265326
continue;
5327+
} else if (settings->library.isSmartPointer(type)) {
5328+
const Token* argTok = Token::findsimplematch(type, "<");
5329+
if (!argTok)
5330+
continue;
5331+
valuetype->smartPointerTypeToken = argTok->next();
5332+
valuetype->smartPointerType = argTok->next()->type();
53275333
} else if (Token::Match(type, "%name% :: %name%")) {
53285334
std::string typestr;
53295335
const Token *end = type;

‎lib/symboldatabase.h‎

Lines changed: 63 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1128,15 +1128,73 @@ class CPPCHECKLIB ValueType {
11281128
nonneg int constness; ///< bit 0=data, bit 1=*, bit 2=**
11291129
const Scope *typeScope; ///< if the type definition is seen this point out the type scope
11301130
const ::Type *smartPointerType; ///< Smart pointer type
1131+
const Token* smartPointerTypeToken; ///< Smart pointer type token
11311132
const Library::Container *container; ///< If the type is a container defined in a cfg file, this is the used container
11321133
const Token *containerTypeToken; ///< The container type token. the template argument token that defines the container element type.
11331134
std::string originalTypeName; ///< original type name as written in the source code. eg. this might be "uint8_t" when type is CHAR.
11341135

1135-
ValueType() : sign(UNKNOWN_SIGN), type(UNKNOWN_TYPE), bits(0), pointer(0U), constness(0U), typeScope(nullptr), smartPointerType(nullptr), container(nullptr), containerTypeToken(nullptr) {}
1136-
ValueType(const ValueType &vt) : sign(vt.sign), type(vt.type), bits(vt.bits), pointer(vt.pointer), constness(vt.constness), typeScope(vt.typeScope), smartPointerType(vt.smartPointerType), container(vt.container), containerTypeToken(vt.containerTypeToken), originalTypeName(vt.originalTypeName) {}
1137-
ValueType(enum Sign s, enum Type t, nonneg int p) : sign(s), type(t), bits(0), pointer(p), constness(0U), typeScope(nullptr), smartPointerType(nullptr), container(nullptr), containerTypeToken(nullptr) {}
1138-
ValueType(enum Sign s, enum Type t, nonneg int p, nonneg int c) : sign(s), type(t), bits(0), pointer(p), constness(c), typeScope(nullptr), smartPointerType(nullptr), container(nullptr), containerTypeToken(nullptr) {}
1139-
ValueType(enum Sign s, enum Type t, nonneg int p, nonneg int c, const std::string &otn) : sign(s), type(t), bits(0), pointer(p), constness(c), typeScope(nullptr), smartPointerType(nullptr), container(nullptr), containerTypeToken(nullptr), originalTypeName(otn) {}
1136+
ValueType()
1137+
: sign(UNKNOWN_SIGN),
1138+
type(UNKNOWN_TYPE),
1139+
bits(0),
1140+
pointer(0U),
1141+
constness(0U),
1142+
typeScope(nullptr),
1143+
smartPointerType(nullptr),
1144+
smartPointerTypeToken(nullptr),
1145+
container(nullptr),
1146+
containerTypeToken(nullptr)
1147+
{}
1148+
ValueType(const ValueType& vt)
1149+
: sign(vt.sign),
1150+
type(vt.type),
1151+
bits(vt.bits),
1152+
pointer(vt.pointer),
1153+
constness(vt.constness),
1154+
typeScope(vt.typeScope),
1155+
smartPointerType(vt.smartPointerType),
1156+
smartPointerTypeToken(vt.smartPointerTypeToken),
1157+
container(vt.container),
1158+
containerTypeToken(vt.containerTypeToken),
1159+
originalTypeName(vt.originalTypeName)
1160+
{}
1161+
ValueType(enum Sign s, enum Type t, nonneg int p)
1162+
: sign(s),
1163+
type(t),
1164+
bits(0),
1165+
pointer(p),
1166+
constness(0U),
1167+
typeScope(nullptr),
1168+
smartPointerType(nullptr),
1169+
smartPointerTypeToken(nullptr),
1170+
container(nullptr),
1171+
containerTypeToken(nullptr)
1172+
{}
1173+
ValueType(enum Sign s, enum Type t, nonneg int p, nonneg int c)
1174+
: sign(s),
1175+
type(t),
1176+
bits(0),
1177+
pointer(p),
1178+
constness(c),
1179+
typeScope(nullptr),
1180+
smartPointerType(nullptr),
1181+
smartPointerTypeToken(nullptr),
1182+
container(nullptr),
1183+
containerTypeToken(nullptr)
1184+
{}
1185+
ValueType(enum Sign s, enum Type t, nonneg int p, nonneg int c, const std::string& otn)
1186+
: sign(s),
1187+
type(t),
1188+
bits(0),
1189+
pointer(p),
1190+
constness(c),
1191+
typeScope(nullptr),
1192+
smartPointerType(nullptr),
1193+
smartPointerTypeToken(nullptr),
1194+
container(nullptr),
1195+
containerTypeToken(nullptr),
1196+
originalTypeName(otn)
1197+
{}
11401198
ValueType &operator=(const ValueType &other) = delete;
11411199

11421200
static ValueType parseDecl(const Token *type, const Settings *settings);

‎lib/valueflow.cpp‎

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3552,7 +3552,7 @@ static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLog
35523552
// Variable
35533553
} else if (tok->variable()) {
35543554
const Variable *var = tok->variable();
3555-
if (!var->typeStartToken() && !var->typeStartToken()->scope())
3555+
if (!var->typeStartToken() || !var->typeStartToken()->scope())
35563556
return;
35573557
const Token *endOfVarScope = var->typeStartToken()->scope()->bodyEnd;
35583558

@@ -4388,6 +4388,9 @@ struct ValueFlowConditionHandler {
43884388
for (const Scope *scope : symboldatabase->functionScopes) {
43894389
std::set<unsigned> aliased;
43904390
for (Token *tok = const_cast<Token *>(scope->bodyStart); tok != scope->bodyEnd; tok = tok->next()) {
4391+
if (Token::Match(tok, "if|while|for ("))
4392+
continue;
4393+
43914394
if (Token::Match(tok, "= & %var% ;"))
43924395
aliased.insert(tok->tokAt(2)->varId());
43934396
const Token* top = tok->astTop();
@@ -4403,21 +4406,22 @@ struct ValueFlowConditionHandler {
44034406
if (cond.true_values.empty() || cond.false_values.empty())
44044407
continue;
44054408

4409+
if (exprDependsOnThis(cond.vartok))
4410+
continue;
44064411
std::vector<const Variable*> vars = getExprVariables(cond.vartok, tokenlist, symboldatabase, settings);
4407-
if (std::any_of(vars.begin(), vars.end(), [](const Variable* var) {
4408-
return !var || !(var->isLocal() || var->isGlobal() || var->isArgument());
4409-
}))
4410-
continue;
4411-
if (std::any_of(vars.begin(), vars.end(), [&](const Variable* var) {
4412-
return aliased.find(var->declarationId()) != aliased.end();
4413-
})) {
4414-
if (settings->debugwarnings)
4415-
bailout(tokenlist,
4416-
errorLogger,
4417-
cond.vartok,
4418-
"variable is aliased so we just skip all valueflow after condition");
4412+
if (std::any_of(vars.begin(), vars.end(), [](const Variable* var) { return !var; }))
44194413
continue;
4420-
}
4414+
if (!vars.empty() && (vars.front()))
4415+
if (std::any_of(vars.begin(), vars.end(), [&](const Variable* var) {
4416+
return var && aliased.find(var->declarationId()) != aliased.end();
4417+
})) {
4418+
if (settings->debugwarnings)
4419+
bailout(tokenlist,
4420+
errorLogger,
4421+
cond.vartok,
4422+
"variable is aliased so we just skip all valueflow after condition");
4423+
continue;
4424+
}
44214425

44224426
if (Token::Match(tok->astParent(), "%oror%|&&")) {
44234427
Token *parent = tok->astParent();

‎test/testnullpointer.cpp‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,10 @@ class TestNullPointer : public TestFixture {
7878
TEST_CASE(nullpointer36); // #9264
7979
TEST_CASE(nullpointer37); // #9315
8080
TEST_CASE(nullpointer38);
81+
TEST_CASE(nullpointer39); // #2153
82+
TEST_CASE(nullpointer40);
83+
TEST_CASE(nullpointer41);
84+
TEST_CASE(nullpointer42);
8185
TEST_CASE(nullpointer_addressOf); // address of
8286
TEST_CASE(nullpointerSwitch); // #2626
8387
TEST_CASE(nullpointer_cast); // #4692
@@ -1474,6 +1478,61 @@ class TestNullPointer : public TestFixture {
14741478
ASSERT_EQUALS("", errout.str());
14751479
}
14761480

1481+
void nullpointer39()
1482+
{
1483+
check("struct A { int * x; };\n"
1484+
"void f(struct A *a) {\n"
1485+
" if (a->x == NULL) {}\n"
1486+
" *(a->x);\n"
1487+
"}\n");
1488+
ASSERT_EQUALS(
1489+
"[test.cpp:3] -> [test.cpp:4]: (warning) Either the condition 'a->x==NULL' is redundant or there is possible null pointer dereference: a->x.\n",
1490+
errout.str());
1491+
}
1492+
1493+
void nullpointer40()
1494+
{
1495+
check("struct A { std::unique_ptr<int> x; };\n"
1496+
"void f(struct A *a) {\n"
1497+
" if (a->x == nullptr) {}\n"
1498+
" *(a->x);\n"
1499+
"}\n");
1500+
ASSERT_EQUALS(
1501+
"[test.cpp:3] -> [test.cpp:4]: (warning) Either the condition 'a->x==nullptr' is redundant or there is possible null pointer dereference: a->x.\n",
1502+
errout.str());
1503+
}
1504+
1505+
void nullpointer41()
1506+
{
1507+
check("struct A { int * g() const; };\n"
1508+
"void f(struct A *a) {\n"
1509+
" if (a->g() == nullptr) {}\n"
1510+
" *(a->g());\n"
1511+
"}\n");
1512+
ASSERT_EQUALS(
1513+
"[test.cpp:3] -> [test.cpp:4]: (warning) Either the condition 'a->g()==nullptr' is redundant or there is possible null pointer dereference: a->g().\n",
1514+
errout.str());
1515+
1516+
check("struct A { int * g(); };\n"
1517+
"void f(struct A *a) {\n"
1518+
" if (a->g() == nullptr) {}\n"
1519+
" *(a->g());\n"
1520+
"}\n");
1521+
ASSERT_EQUALS("", errout.str());
1522+
}
1523+
1524+
void nullpointer42()
1525+
{
1526+
check("struct A { std::unique_ptr<int> g() const; };\n"
1527+
"void f(struct A *a) {\n"
1528+
" if (a->g() == nullptr) {}\n"
1529+
" *(a->g());\n"
1530+
"}\n");
1531+
ASSERT_EQUALS(
1532+
"[test.cpp:3] -> [test.cpp:4]: (warning) Either the condition 'a->g()==nullptr' is redundant or there is possible null pointer dereference: a->g().\n",
1533+
errout.str());
1534+
}
1535+
14771536
void nullpointer_addressOf() { // address of
14781537
check("void f() {\n"
14791538
" struct X *x = 0;\n"

0 commit comments

Comments
 (0)