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

lib/astutils.cpp

@@ -138,6 +138,8 @@ bool astIsPointer(const Token *tok)
     return tok && tok->valueType() && tok->valueType()->pointer;
 }
 
+bool astIsSmartPointer(const Token* tok) { return tok && tok->valueType() && tok->valueType()->smartPointerTypeToken; }
+
 bool astIsIterator(const Token *tok)
 {
     return tok && tok->valueType() && tok->valueType()->type == ValueType::Type::ITERATOR;
@@ -335,7 +337,7 @@ bool isAliased(const Variable *var)
     return isAliased(start, var->scope()->bodyEnd, var->declarationId());
 }
 
-static bool exprDependsOnThis(const Token *expr, nonneg int depth)
+bool exprDependsOnThis(const Token* expr, nonneg int depth)
 {
     if (!expr)
         return false;
@@ -353,7 +355,12 @@ static bool exprDependsOnThis(const Token *expr, nonneg int depth)
             nestedIn = nestedIn->nestedIn;
         }
         return nestedIn == expr->function()->nestedIn;
+    } else if (Token::Match(expr, "%var%") && expr->variable()) {
+        const Variable* var = expr->variable();
+        return (var->isPrivate() || var->isPublic() || var->isProtected());
     }
+    if (Token::simpleMatch(expr, "."))
+        return exprDependsOnThis(expr->astOperand1(), depth);
     return exprDependsOnThis(expr->astOperand1(), depth) || exprDependsOnThis(expr->astOperand2(), depth);
 }
 
@@ -381,7 +388,7 @@ static const Token * followVariableExpression(const Token * tok, bool cpp, const
     if (!varTok)
         return tok;
     // Bailout. If variable value depends on value of "this".
-    if (exprDependsOnThis(varTok, 0))
+    if (exprDependsOnThis(varTok))
         return tok;
     // Skip array access
     if (Token::simpleMatch(varTok, "["))
@@ -1743,10 +1750,10 @@ struct FwdAnalysis::Result FwdAnalysis::checkRecursive(const Token *expr, const
         if (exprVarIds.find(tok->varId()) != exprVarIds.end()) {
             const Token *parent = tok;
             bool other = false;
-            bool same = tok->astParent() && isSameExpression(mCpp, false, expr, tok, mLibrary, false, false, nullptr);
+            bool same = tok->astParent() && isSameExpression(mCpp, false, expr, tok, mLibrary, true, false, nullptr);
-            while (!same && Token::Match(parent->astParent(), "*|.|::|[|%cop%")) {
+            while (!same && Token::Match(parent->astParent(), "*|.|::|[|(|%cop%")) {
                 parent = parent->astParent();
-                if (parent && isSameExpression(mCpp, false, expr, parent, mLibrary, false, false, nullptr)) {
+                if (parent && isSameExpression(mCpp, false, expr, parent, mLibrary, true, false, nullptr)) {
                     same = true;
                     if (mWhat == What::ValueFlow) {
                         KnownAndToken v;

lib/astutils.h

@@ -67,6 +67,8 @@ bool astIsBool(const Token *tok);
 
 bool astIsPointer(const Token *tok);
 
+bool astIsSmartPointer(const Token* tok);
+
 bool astIsIterator(const Token *tok);
 
 bool astIsContainer(const Token *tok);
@@ -92,6 +94,8 @@ const Token* astParentSkipParens(const Token* tok);
 
 bool precedes(const Token * tok1, const Token * tok2);
 
+bool exprDependsOnThis(const Token* expr, nonneg int depth = 0);
+
 bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2, const Library& library, bool pure, bool followVar, ErrorPath* errors=nullptr);
 
 bool isEqualKnownValue(const Token * const tok1, const Token * const tok2);

lib/checkio.cpp

@@ -956,7 +956,7 @@ void CheckIO::checkFormatString(const Token * const tok,
                                 if (argListTok->tokType() != Token::eString &&
                                     argInfo.isKnownType() && !argInfo.isArrayOrPointer()) {
                                     if (!Token::Match(argInfo.typeToken, "char|wchar_t")) {
-                                        if (!(!argInfo.isArrayOrPointer() && argInfo.element))
+                                        if (!argInfo.element)
                                             invalidPrintfArgTypeError_s(tok, numFormat, &argInfo);
                                     }
                                 }

lib/checknullpointer.cpp

@@ -318,6 +318,31 @@ void CheckNullPointer::nullPointerLinkedList()
     }
 }
 
+static bool isNullablePointer(const Token* tok, const Settings* settings)
+{
+    if (!tok)
+        return false;
+    if (astIsPointer(tok))
+        return true;
+    if (astIsSmartPointer(tok))
+        return true;
+    // TODO: Move this logic into ValueType
+    if (Token::simpleMatch(tok, "."))
+        return isNullablePointer(tok->astOperand2(), settings);
+    if (const Variable* var = tok->variable()) {
+        return (var->isPointer() || var->isSmartPointer());
+    }
+    if (Token::Match(tok->previous(), "%name% (")) {
+        if (const Function* f = tok->previous()->function()) {
+            if (f->retDef) {
+                ValueType vt = ValueType::parseDecl(f->retDef, settings);
+                return vt.smartPointerTypeToken || vt.pointer > 0;
+            }
+        }
+    }
+    return false;
+}
+
 void CheckNullPointer::nullPointerByDeRefAndChec()
 {
     const bool printInconclusive = (mSettings->inconclusive);
@@ -328,11 +353,10 @@ void CheckNullPointer::nullPointerByDeRefAndChec()
             continue;
         }
 
-        const Variable *var = tok->variable();
+        if (Token::Match(tok, "%num%|%char%|%str%"))
-        if (!var || tok == var->nameToken())
             continue;
 
-        if (!var->isPointer() && !var->isSmartPointer())
+        if (!isNullablePointer(tok, mSettings))
             continue;
 
         // Can pointer be NULL?
@@ -347,11 +371,11 @@ void CheckNullPointer::nullPointerByDeRefAndChec()
         bool unknown = false;
         if (!isPointerDeRef(tok,unknown)) {
             if (unknown)
-                nullPointerError(tok, tok->str(), value, true);
+                nullPointerError(tok, tok->expressionString(), value, true);
             continue;
         }
 
-        nullPointerError(tok, tok->str(), value, value->isInconclusive());
+        nullPointerError(tok, tok->expressionString(), value, value->isInconclusive());
     }
 }
 

lib/checkuninitvar.cpp

@@ -1101,7 +1101,7 @@ bool CheckUninitVar::isVariableUsage(const Token *vartok, bool pointer, Alloc al
     if (alloc == NO_ALLOC && vartok->next() && vartok->next()->isOp() && !vartok->next()->isAssignmentOp())
         return true;
 
-    if (alloc == NO_ALLOC && vartok->next()->isAssignmentOp() && vartok->next()->str() != "=")
+    if (alloc == NO_ALLOC && vartok->next() && vartok->next()->isAssignmentOp() && vartok->next()->str() != "=")
         return true;
 
     if (vartok->strAt(1) == "]")

lib/symboldatabase.cpp

@@ -5324,6 +5324,12 @@ static const Token * parsedecl(const Token *type, ValueType * const valuetype, V
                 type = type->next();
             }
             continue;
+        } else if (settings->library.isSmartPointer(type)) {
+            const Token* argTok = Token::findsimplematch(type, "<");
+            if (!argTok)
+                continue;
+            valuetype->smartPointerTypeToken = argTok->next();
+            valuetype->smartPointerType = argTok->next()->type();
         } else if (Token::Match(type, "%name% :: %name%")) {
             std::string typestr;
             const Token *end = type;

lib/symboldatabase.h

@@ -1128,15 +1128,73 @@ public:
     nonneg int constness;               ///< bit 0=data, bit 1=*, bit 2=**
     const Scope *typeScope;               ///< if the type definition is seen this point out the type scope
     const ::Type *smartPointerType;       ///< Smart pointer type
+    const Token* smartPointerTypeToken;   ///< Smart pointer type token
     const Library::Container *container;  ///< If the type is a container defined in a cfg file, this is the used container
     const Token *containerTypeToken;      ///< The container type token. the template argument token that defines the container element type.
     std::string originalTypeName;         ///< original type name as written in the source code. eg. this might be "uint8_t" when type is CHAR.
 
-    ValueType() : sign(UNKNOWN_SIGN), type(UNKNOWN_TYPE), bits(0), pointer(0U), constness(0U), typeScope(nullptr), smartPointerType(nullptr), container(nullptr), containerTypeToken(nullptr) {}
+    ValueType()
-    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) {}
+        : sign(UNKNOWN_SIGN),
-    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) {}
+          type(UNKNOWN_TYPE),
-    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) {}
+          bits(0),
-    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) {}
+          pointer(0U),
+          constness(0U),
+          typeScope(nullptr),
+          smartPointerType(nullptr),
+          smartPointerTypeToken(nullptr),
+          container(nullptr),
+          containerTypeToken(nullptr)
+    {}
+    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),
+          smartPointerTypeToken(vt.smartPointerTypeToken),
+          container(vt.container),
+          containerTypeToken(vt.containerTypeToken),
+          originalTypeName(vt.originalTypeName)
+    {}
+    ValueType(enum Sign s, enum Type t, nonneg int p)
+        : sign(s),
+          type(t),
+          bits(0),
+          pointer(p),
+          constness(0U),
+          typeScope(nullptr),
+          smartPointerType(nullptr),
+          smartPointerTypeToken(nullptr),
+          container(nullptr),
+          containerTypeToken(nullptr)
+    {}
+    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),
+          smartPointerTypeToken(nullptr),
+          container(nullptr),
+          containerTypeToken(nullptr)
+    {}
+    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),
+          smartPointerTypeToken(nullptr),
+          container(nullptr),
+          containerTypeToken(nullptr),
+          originalTypeName(otn)
+    {}
     ValueType &operator=(const ValueType &other) = delete;
 
     static ValueType parseDecl(const Token *type, const Settings *settings);

lib/valueflow.cpp

@@ -3552,7 +3552,7 @@ static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLog
         // Variable
     } else if (tok->variable()) {
         const Variable *var = tok->variable();
-        if (!var->typeStartToken() && !var->typeStartToken()->scope())
+        if (!var->typeStartToken() || !var->typeStartToken()->scope())
             return;
         const Token *endOfVarScope = var->typeStartToken()->scope()->bodyEnd;
 
@@ -4388,6 +4388,9 @@ struct ValueFlowConditionHandler {
         for (const Scope *scope : symboldatabase->functionScopes) {
             std::set<unsigned> aliased;
             for (Token *tok = const_cast<Token *>(scope->bodyStart); tok != scope->bodyEnd; tok = tok->next()) {
+                if (Token::Match(tok, "if|while|for ("))
+                    continue;
+
                 if (Token::Match(tok, "= & %var% ;"))
                     aliased.insert(tok->tokAt(2)->varId());
                 const Token* top = tok->astTop();
@@ -4403,21 +4406,22 @@ struct ValueFlowConditionHandler {
                 if (cond.true_values.empty() || cond.false_values.empty())
                     continue;
 
-                std::vector<const Variable*> vars = getExprVariables(cond.vartok, tokenlist, symboldatabase, settings);
+                if (exprDependsOnThis(cond.vartok))
-                if (std::any_of(vars.begin(), vars.end(), [](const Variable* var) {
-                return !var || !(var->isLocal() || var->isGlobal() || var->isArgument());
-                }))
-                continue;
-                if (std::any_of(vars.begin(), vars.end(), [&](const Variable* var) {
-                return aliased.find(var->declarationId()) != aliased.end();
-                })) {
-                    if (settings->debugwarnings)
-                        bailout(tokenlist,
-                                errorLogger,
-                                cond.vartok,
-                                "variable is aliased so we just skip all valueflow after condition");
                     continue;
-                }
+                std::vector<const Variable*> vars = getExprVariables(cond.vartok, tokenlist, symboldatabase, settings);
+                if (std::any_of(vars.begin(), vars.end(), [](const Variable* var) { return !var; }))
+                    continue;
+                if (!vars.empty() && (vars.front()))
+                    if (std::any_of(vars.begin(), vars.end(), [&](const Variable* var) {
+                            return var && aliased.find(var->declarationId()) != aliased.end();
+                        })) {
+                        if (settings->debugwarnings)
+                            bailout(tokenlist,
+                                    errorLogger,
+                                    cond.vartok,
+                                    "variable is aliased so we just skip all valueflow after condition");
+                        continue;
+                    }
 
                 if (Token::Match(tok->astParent(), "%oror%|&&")) {
                     Token *parent = tok->astParent();

test/testnullpointer.cpp

@@ -78,6 +78,10 @@ private:
         TEST_CASE(nullpointer36); // #9264
         TEST_CASE(nullpointer37); // #9315
         TEST_CASE(nullpointer38);
+        TEST_CASE(nullpointer39); // #2153
+        TEST_CASE(nullpointer40);
+        TEST_CASE(nullpointer41);
+        TEST_CASE(nullpointer42);
         TEST_CASE(nullpointer_addressOf); // address of
         TEST_CASE(nullpointerSwitch); // #2626
         TEST_CASE(nullpointer_cast); // #4692
@@ -1474,6 +1478,61 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void nullpointer39()
+    {
+        check("struct A { int * x; };\n"
+              "void f(struct A *a) {\n"
+              "    if (a->x == NULL) {}\n"
+              "    *(a->x);\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[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",
+            errout.str());
+    }
+
+    void nullpointer40()
+    {
+        check("struct A { std::unique_ptr<int> x; };\n"
+              "void f(struct A *a) {\n"
+              "    if (a->x == nullptr) {}\n"
+              "    *(a->x);\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[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",
+            errout.str());
+    }
+
+    void nullpointer41()
+    {
+        check("struct A { int * g() const; };\n"
+              "void f(struct A *a) {\n"
+              "    if (a->g() == nullptr) {}\n"
+              "    *(a->g());\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[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",
+            errout.str());
+
+        check("struct A { int * g(); };\n"
+              "void f(struct A *a) {\n"
+              "    if (a->g() == nullptr) {}\n"
+              "    *(a->g());\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
+
+    void nullpointer42()
+    {
+        check("struct A { std::unique_ptr<int> g() const; };\n"
+              "void f(struct A *a) {\n"
+              "    if (a->g() == nullptr) {}\n"
+              "    *(a->g());\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[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",
+            errout.str());
+    }
+
     void nullpointer_addressOf() { // address of
         check("void f() {\n"
               "  struct X *x = 0;\n"