From 39f4374446d6b3d24b2cec790c6ff8a0663f7181 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Fri, 26 Apr 2019 04:30:09 -0500 Subject: [PATCH] Improve diagnostics with null smart pointers (#1805) * Warn when dereferencing null smart pointers * Improve tracking of smart pointer values * Use library isSmartPointer --- lib/checknullpointer.cpp | 5 +- lib/symboldatabase.cpp | 1 + lib/symboldatabase.h | 33 ++++---- lib/valueflow.cpp | 162 +++++++++++++++++++++++++++------------ test/testnullpointer.cpp | 75 ++++++++++++++++++ 5 files changed, 212 insertions(+), 64 deletions(-) diff --git a/lib/checknullpointer.cpp b/lib/checknullpointer.cpp index 9a94c813c..c63e2dbfa 100644 --- a/lib/checknullpointer.cpp +++ b/lib/checknullpointer.cpp @@ -329,7 +329,10 @@ void CheckNullPointer::nullPointerByDeRefAndChec() } const Variable *var = tok->variable(); - if (!var || !var->isPointer() || tok == var->nameToken()) + if (!var || tok == var->nameToken()) + continue; + + if (!var->isPointer() && !var->isSmartPointer()) continue; // Can pointer be NULL? diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index b331de2cb..f7127795d 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -1659,6 +1659,7 @@ void Variable::evaluate(const Settings* settings) setFlag(fIsClass, !lib->podtype(strtype) && !mTypeStartToken->isStandardType() && !isEnumType() && !isPointer() && !isReference()); setFlag(fIsStlType, Token::simpleMatch(mTypeStartToken, "std ::")); setFlag(fIsStlString, isStlType() && (Token::Match(mTypeStartToken->tokAt(2), "string|wstring|u16string|u32string !!::") || (Token::simpleMatch(mTypeStartToken->tokAt(2), "basic_string <") && !Token::simpleMatch(mTypeStartToken->linkAt(3), "> ::")))); + setFlag(fIsSmartPointer, lib->isSmartPointer(mTypeStartToken)); } if (mAccess == Argument) { tok = mNameToken; diff --git a/lib/symboldatabase.h b/lib/symboldatabase.h index 862d715dd..faa78abfb 100644 --- a/lib/symboldatabase.h +++ b/lib/symboldatabase.h @@ -177,20 +177,21 @@ public: class CPPCHECKLIB Variable { /** @brief flags mask used to access specific bit. */ enum { - fIsMutable = (1 << 0), /** @brief mutable variable */ - fIsStatic = (1 << 1), /** @brief static variable */ - fIsConst = (1 << 2), /** @brief const variable */ - fIsExtern = (1 << 3), /** @brief extern variable */ - fIsClass = (1 << 4), /** @brief user defined type */ - fIsArray = (1 << 5), /** @brief array variable */ - fIsPointer = (1 << 6), /** @brief pointer variable */ - fIsReference = (1 << 7), /** @brief reference variable */ - fIsRValueRef = (1 << 8), /** @brief rvalue reference variable */ - fHasDefault = (1 << 9), /** @brief function argument with default value */ - fIsStlType = (1 << 10), /** @brief STL type ('std::') */ - fIsStlString = (1 << 11), /** @brief std::string|wstring|basic_string<T>|u16string|u32string */ - fIsFloatType = (1 << 12), /** @brief Floating point type */ - fIsVolatile = (1 << 13) /** @brief volatile */ + fIsMutable = (1 << 0), /** @brief mutable variable */ + fIsStatic = (1 << 1), /** @brief static variable */ + fIsConst = (1 << 2), /** @brief const variable */ + fIsExtern = (1 << 3), /** @brief extern variable */ + fIsClass = (1 << 4), /** @brief user defined type */ + fIsArray = (1 << 5), /** @brief array variable */ + fIsPointer = (1 << 6), /** @brief pointer variable */ + fIsReference = (1 << 7), /** @brief reference variable */ + fIsRValueRef = (1 << 8), /** @brief rvalue reference variable */ + fHasDefault = (1 << 9), /** @brief function argument with default value */ + fIsStlType = (1 << 10), /** @brief STL type ('std::') */ + fIsStlString = (1 << 11), /** @brief std::string|wstring|basic_string<T>|u16string|u32string */ + fIsFloatType = (1 << 12), /** @brief Floating point type */ + fIsVolatile = (1 << 13), /** @brief volatile */ + fIsSmartPointer = (1 << 14) /** @brief std::shared_ptr|unique_ptr */ }; /** @@ -555,6 +556,10 @@ public: return getFlag(fIsStlString); } + bool isSmartPointer() const { + return getFlag(fIsSmartPointer); + } + /** * Checks if the variable is of any of the STL types passed as arguments ('std::') * E.g.: diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 67e7e9bc3..d220cfa68 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -3496,6 +3496,67 @@ static void valueFlowAfterMove(TokenList *tokenlist, SymbolDatabase* symboldatab } } +static void valueFlowForwardAssign(Token * const tok, + const Variable * const var, + std::list values, + const bool constValue, + const bool init, + TokenList * const tokenlist, + ErrorLogger * const errorLogger, + const Settings * const settings) +{ + const Token * const endOfVarScope = var->typeStartToken()->scope()->bodyEnd; + if (std::any_of(values.begin(), values.end(), std::mem_fn(&ValueFlow::Value::isLifetimeValue))) { + valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings); + values.remove_if(std::mem_fn(&ValueFlow::Value::isLifetimeValue)); + } + if (!var->isPointer() && !var->isSmartPointer()) + values.remove_if(std::mem_fn(&ValueFlow::Value::isTokValue)); + if (tok->astParent()) { + for (std::list::iterator it = values.begin(); it != values.end(); ++it) { + const std::string info = "Assignment '" + tok->astParent()->expressionString() + "', assigned value is " + it->infoString(); + it->errorPath.emplace_back(tok, info); + } + } + + if (tokenlist->isCPP() && Token::Match(var->typeStartToken(), "bool|_Bool")) { + std::list::iterator it; + for (it = values.begin(); it != values.end(); ++it) { + if (it->isIntValue()) + it->intvalue = (it->intvalue != 0); + if (it->isTokValue()) + it ->intvalue = (it->tokvalue != 0); + } + } + + // Static variable initialisation? + if (var->isStatic() && init) + changeKnownToPossible(values); + + // Skip RHS + const Token * nextExpression = tok->astParent() ? nextAfterAstRightmostLeaf(tok->astParent()) : tok->next(); + + if (std::any_of(values.begin(), values.end(), std::mem_fn(&ValueFlow::Value::isTokValue))) { + std::list tokvalues; + std::copy_if(values.begin(), + values.end(), + std::back_inserter(tokvalues), + std::mem_fn(&ValueFlow::Value::isTokValue)); + valueFlowForward(const_cast(nextExpression), + endOfVarScope, + var, + var->declarationId(), + tokvalues, + constValue, + false, + tokenlist, + errorLogger, + settings); + values.remove_if(std::mem_fn(&ValueFlow::Value::isTokValue)); + } + valueFlowForward(const_cast(nextExpression), endOfVarScope, var, var->declarationId(), values, constValue, false, tokenlist, errorLogger, settings); +} + static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings) { for (const Scope * scope : symboldatabase->functionScopes) { @@ -3521,61 +3582,14 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat if (!var || (!var->isLocal() && !var->isGlobal() && !var->isArgument())) continue; - const Token * const endOfVarScope = var->typeStartToken()->scope()->bodyEnd; - // Rhs values.. if (!tok->astOperand2() || tok->astOperand2()->values().empty()) continue; std::list values = tok->astOperand2()->values(); - if (std::any_of(values.begin(), values.end(), std::mem_fn(&ValueFlow::Value::isLifetimeValue))) { - valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings); - values.remove_if(std::mem_fn(&ValueFlow::Value::isLifetimeValue)); - } - if (!var->isPointer()) - values.remove_if(std::mem_fn(&ValueFlow::Value::isTokValue)); - for (std::list::iterator it = values.begin(); it != values.end(); ++it) { - const std::string info = "Assignment '" + tok->expressionString() + "', assigned value is " + it->infoString(); - it->errorPath.emplace_back(tok->astOperand2(), info); - } const bool constValue = tok->astOperand2()->isNumber(); - - if (tokenlist->isCPP() && Token::Match(var->typeStartToken(), "bool|_Bool")) { - std::list::iterator it; - for (it = values.begin(); it != values.end(); ++it) { - if (it->isIntValue()) - it->intvalue = (it->intvalue != 0); - if (it->isTokValue()) - it ->intvalue = (it->tokvalue != 0); - } - } - - // Static variable initialisation? - if (var->isStatic() && var->nameToken() == tok->astOperand1()) - changeKnownToPossible(values); - - // Skip RHS - const Token * nextExpression = nextAfterAstRightmostLeaf(tok); - - if (std::any_of(values.begin(), values.end(), std::mem_fn(&ValueFlow::Value::isTokValue))) { - std::list tokvalues; - std::copy_if(values.begin(), - values.end(), - std::back_inserter(tokvalues), - std::mem_fn(&ValueFlow::Value::isTokValue)); - valueFlowForward(const_cast(nextExpression), - endOfVarScope, - var, - varid, - tokvalues, - constValue, - false, - tokenlist, - errorLogger, - settings); - values.remove_if(std::mem_fn(&ValueFlow::Value::isTokValue)); - } - valueFlowForward(const_cast(nextExpression), endOfVarScope, var, varid, values, constValue, false, tokenlist, errorLogger, settings); + const bool init = var->nameToken() == tok->astOperand1(); + valueFlowForwardAssign(const_cast(tok->astOperand2()), var, values, constValue, init, tokenlist, errorLogger, settings); } } } @@ -4954,6 +4968,55 @@ static bool isContainerSizeChanged(unsigned int varId, const Token *start, const return false; } +static void valueFlowSmartPointer(TokenList *tokenlist, ErrorLogger * errorLogger, const Settings *settings) +{ + for (Token *tok = tokenlist->front(); tok; tok = tok->next()) { + if (!tok->scope()) + continue; + if (!tok->scope()->isExecutable()) + continue; + if (!tok->variable()) + continue; + const Variable * var = tok->variable(); + if (!var->isSmartPointer()) + continue; + if (var->nameToken() == tok) { + if (Token::Match(tok, "%var% (|{") && tok->next()->astOperand2() && tok->next()->astOperand2()->str() != ",") { + const Token * inTok = tok->next()->astOperand2(); + std::list values = inTok->values(); + const bool constValue = inTok->isNumber(); + valueFlowForwardAssign(const_cast(inTok), var, values, constValue, true, tokenlist, errorLogger, settings); + + } else if (Token::Match(tok, "%var% ;")) { + std::list values; + ValueFlow::Value v(0); + v.setKnown(); + values.push_back(v); + valueFlowForwardAssign(tok, var, values, false, true, tokenlist, errorLogger, settings); + } + } else if (Token::Match(tok, "%var% . reset (")) { + if (Token::simpleMatch(tok->tokAt(3), "( )")) { + std::list values; + ValueFlow::Value v(0); + v.setKnown(); + values.push_back(v); + valueFlowForwardAssign(tok->tokAt(4), var, values, false, false, tokenlist, errorLogger, settings); + } else { + const Token * inTok = tok->tokAt(3)->astOperand2(); + std::list values = inTok->values(); + const bool constValue = inTok->isNumber(); + valueFlowForwardAssign(const_cast(inTok), var, values, constValue, false, tokenlist, errorLogger, settings); + } + } else if (Token::Match(tok, "%var% . release ( )")) { + std::list values; + ValueFlow::Value v(0); + v.setKnown(); + values.push_back(v); + valueFlowForwardAssign(tok->tokAt(4), var, values, false, false, tokenlist, errorLogger, settings); + } + } +} + static void valueFlowContainerSize(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger * /*errorLogger*/, const Settings *settings) { // declaration @@ -5302,6 +5365,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase, valueFlowFunctionDefaultParameter(tokenlist, symboldatabase, errorLogger, settings); valueFlowUninit(tokenlist, symboldatabase, errorLogger, settings); if (tokenlist->isCPP()) { + valueFlowSmartPointer(tokenlist, errorLogger, settings); valueFlowContainerSize(tokenlist, symboldatabase, errorLogger, settings); valueFlowContainerAfterCondition(tokenlist, symboldatabase, errorLogger, settings); } diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 4f9a1c36b..002ec12d1 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -90,6 +90,7 @@ private: TEST_CASE(nullpointerExit); TEST_CASE(nullpointerStdString); TEST_CASE(nullpointerStdStream); + TEST_CASE(nullpointerSmartPointer); TEST_CASE(functioncall); TEST_CASE(functioncalllibrary); // use Library to parse function call TEST_CASE(functioncallDefaultArguments); @@ -2259,6 +2260,80 @@ private: } + void nullpointerSmartPointer() { + check("struct Fred { int x; };\n" + "void f(std::shared_ptr p) {\n" + " if (p) {}\n" + " dostuff(p->x);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Either the condition 'p' is redundant or there is possible null pointer dereference: p.\n", errout.str()); + + check("struct Fred { int x; };\n" + "void f(std::shared_ptr p) {\n" + " p = nullptr;\n" + " dostuff(p->x);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Null pointer dereference: p\n", errout.str()); + + check("struct Fred { int x; };\n" + "void f(std::unique_ptr p) {\n" + " if (p) {}\n" + " dostuff(p->x);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Either the condition 'p' is redundant or there is possible null pointer dereference: p.\n", errout.str()); + + check("struct Fred { int x; };\n" + "void f(std::unique_ptr p) {\n" + " p = nullptr;\n" + " dostuff(p->x);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Null pointer dereference: p\n", errout.str()); + + check("struct Fred { int x; };\n" + "void f() {\n" + " std::shared_ptr p;\n" + " dostuff(p->x);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Null pointer dereference: p\n", errout.str()); + + check("struct Fred { int x; };\n" + "void f(std::shared_ptr p) {\n" + " p.reset();\n" + " dostuff(p->x);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Null pointer dereference: p\n", errout.str()); + + check("struct Fred { int x; };\n" + "void f(std::shared_ptr p) {\n" + " Fred * pp = nullptr;\n" + " p.reset(pp);\n" + " dostuff(p->x);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (error) Null pointer dereference: p\n", errout.str()); + + check("struct Fred { int x; };\n" + "void f(Fred& f) {\n" + " std::shared_ptr p;\n" + " p.reset(&f);\n" + " dostuff(p->x);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("struct Fred { int x; };\n" + "void f(std::shared_ptr p) {\n" + " p.release();\n" + " dostuff(p->x);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Null pointer dereference: p\n", errout.str()); + + check("struct Fred { int x; };\n" + "void f() {\n" + " std::shared_ptr p(nullptr);\n" + " dostuff(p->x);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Null pointer dereference: p\n", errout.str()); + } + void functioncall() { // #3443 - function calls // dereference pointer and then check if it's null {