Merge pull request #2719 from pfultz2/fp-unreachable-alias

Fix issue 9807: False positive: ValueFlow in unreachable code, || lhs is true

lib/astutils.cpp

@@ -1442,9 +1442,16 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings *settings,
     if (!tok)
         return false;
     const Token *tok2 = tok;
-    while (Token::simpleMatch(tok2->astParent(), "*") || (Token::simpleMatch(tok2->astParent(), ".") && !Token::simpleMatch(tok2->astParent()->astParent(), "(")) ||
+    int derefs = 0;
-           (Token::simpleMatch(tok2->astParent(), "[") && tok2 == tok2->astParent()->astOperand1()))
+    while (Token::simpleMatch(tok2->astParent(), "*") || 
+           (Token::simpleMatch(tok2->astParent(), ".") && !Token::simpleMatch(tok2->astParent()->astParent(), "(")) ||
+           (Token::simpleMatch(tok2->astParent(), "[") && tok2 == tok2->astParent()->astOperand1())) {
+        if (Token::simpleMatch(tok2->astParent(), "*") || tok2->astParent()->originalName() == "->")
+            derefs++;
+        if (derefs > indirect)
+            break;
         tok2 = tok2->astParent();
+    }
 
     while (Token::simpleMatch(tok2->astParent(), "?") || (Token::simpleMatch(tok2->astParent(), ":") && Token::simpleMatch(tok2->astParent()->astParent(), "?")))
         tok2 = tok2->astParent();
@@ -1526,6 +1533,11 @@ bool isVariableChanged(const Token *start, const Token *end, const nonneg int va
     return findVariableChanged(start, end, 0, varid, globalvar, settings, cpp, depth) != nullptr;
 }
 
+bool isVariableChanged(const Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth)
+{
+    return findVariableChanged(start, end, indirect, varid, globalvar, settings, cpp, depth) != nullptr;
+}
+
 Token* findVariableChanged(Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth)
 {
     if (!precedes(start, end))

lib/astutils.h

@@ -183,6 +183,7 @@ bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Setti
 
 /** Is variable changed in block of code? */
 bool isVariableChanged(const Token *start, const Token *end, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20);
+bool isVariableChanged(const Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20);
 
 bool isVariableChanged(const Token *tok, int indirect, const Settings *settings, bool cpp, int depth = 20);
 

lib/checkautovariables.cpp

@@ -274,7 +274,9 @@ void CheckAutoVariables::autoVariables()
                     errorInvalidDeallocation(tok, nullptr);
                 else if (tok && tok->variable() && tok->variable()->isPointer()) {
                     for (const ValueFlow::Value &v : tok->values()) {
-                        if (v.isTokValue() && isArrayVar(v.tokvalue)) {
+                        if (!(v.isTokValue()))
+                            continue;
+                        if (isArrayVar(v.tokvalue)) {
                             errorInvalidDeallocation(tok, &v);
                             break;
                         }

lib/checkcondition.cpp

@@ -1119,7 +1119,7 @@ void CheckCondition::checkIncorrectLogicOperator()
 
             // Comparison #1 (LHS)
             const Token *comp1 = tok->astOperand1();
-            if (comp1 && comp1->str() == tok->str())
+            if (comp1->str() == tok->str())
                 comp1 = comp1->astOperand2();
 
             // Comparison #2 (RHS)

lib/ctu.cpp

@@ -424,7 +424,10 @@ static std::list<std::pair<const Token *, MathLib::bigint>> getUnsafeFunction(co
             tok2 = tok2->linkAt(1);
             if (Token::findmatch(tok2->link(), "return|throw", tok2))
                 return ret;
-            if (isVariableChanged(tok2->link(), tok2, argvar->declarationId(), false, settings, tokenizer->isCPP()))
+            int indirect = 0;
+            if(argvar->valueType())
+                indirect = argvar->valueType()->pointer;
+            if (isVariableChanged(tok2->link(), tok2, indirect, argvar->declarationId(), false, settings, tokenizer->isCPP()))
                 return ret;
         }
         if (Token::Match(tok2, "%oror%|&&|?")) {

lib/valueflow.cpp

@@ -1256,7 +1256,9 @@ static void valueFlowPointerAliasDeref(TokenList *tokenlist)
         if (!var->isConst() && isVariableChanged(lifeTok->next(), tok, lifeTok->varId(), !var->isLocal(), tokenlist->getSettings(), tokenlist->isCPP()))
             continue;
         for (const ValueFlow::Value& v:lifeTok->values()) {
-            if (v.isLifetimeValue())
+            // TODO: Move container size values to generic forward
+            // Forward uninit values since not all values can be forwarded directly
+            if (!(v.isContainerSizeValue() || v.isUninitValue()))
                 continue;
             ValueFlow::Value value = v;
             value.errorPath.insert(value.errorPath.begin(), errorPath.begin(), errorPath.end());
@@ -2267,6 +2269,9 @@ struct ValueFlowForwardAnalyzer : ForwardAnalyzer {
 
     virtual ProgramState getProgramState() const = 0;
 
+    virtual const ValueType* getValueType(const Token*) const {
+        return nullptr;
+    }
     virtual int getIndirect(const Token* tok) const {
         const ValueFlow::Value* value = getValue(tok);
         if (value)
@@ -2314,8 +2319,12 @@ struct ValueFlowForwardAnalyzer : ForwardAnalyzer {
 
     virtual Action isAliasModified(const Token* tok) const {
         int indirect = 0;
+        int baseIndirect = 0;
+        const ValueType* vt = getValueType(tok);
+        if (vt)
+            baseIndirect = vt->pointer;
         if (tok->valueType())
-            indirect = tok->valueType()->pointer;
+            indirect = std::max<int>(0, tok->valueType()->pointer - baseIndirect);
         if (isVariableChanged(tok, indirect, getSettings(), isCPP()))
             return Action::Invalid;
         return Action::None;
@@ -2350,6 +2359,25 @@ struct ValueFlowForwardAnalyzer : ForwardAnalyzer {
             }
             // Check for modifications by function calls
             return isModified(tok);
+        } else if (tok->isUnaryOp("*")) {
+            const Token* lifeTok = nullptr;
+            for (const ValueFlow::Value& v:tok->astOperand1()->values()) {
+                if (!v.isLocalLifetimeValue())
+                    continue;
+                if (lifeTok)
+                    return Action::None;
+                lifeTok = v.tokvalue;
+            }
+            if (lifeTok && match(lifeTok)) {
+                Action a = Action::Read;
+                if (isModified(tok).isModified())
+                    a = Action::Invalid;
+                if (Token::Match(tok->astParent(), "%assign%") && astIsLHS(tok))
+                    a |= Action::Read;
+                return a;
+            }
+            return Action::None;
+
         } else if (isAlias(tok)) {
             return isAliasModified(tok);
         } else if (Token::Match(tok, "%name% (") && !Token::simpleMatch(tok->linkAt(1), ") {")) {
@@ -2425,6 +2453,7 @@ struct ValueFlowForwardAnalyzer : ForwardAnalyzer {
 
 struct SingleValueFlowForwardAnalyzer : ValueFlowForwardAnalyzer {
     std::unordered_map<nonneg int, const Variable*> varids;
+    std::unordered_map<nonneg int, const Variable*> aliases;
     ValueFlow::Value value;
 
     SingleValueFlowForwardAnalyzer()
@@ -2439,6 +2468,10 @@ struct SingleValueFlowForwardAnalyzer : ValueFlowForwardAnalyzer {
         return varids;
     }
 
+    const std::unordered_map<nonneg int, const Variable*>& getAliasedVars() const {
+        return aliases;
+    }
+
     virtual const ValueFlow::Value* getValue(const Token*) const OVERRIDE {
         return &value;
     }
@@ -2457,7 +2490,8 @@ struct SingleValueFlowForwardAnalyzer : ValueFlowForwardAnalyzer {
     virtual bool isAlias(const Token* tok) const OVERRIDE {
         if (value.isLifetimeValue())
             return false;
-        for (const auto& p:getVars()) {
+        for(const auto& m:{std::ref(getVars()), std::ref(getAliasedVars())}) {
+            for (const auto& p:m.get()) {
                 nonneg int varid = p.first;
                 const Variable* var = p.second;
                 if (tok->varId() == varid)
@@ -2465,6 +2499,7 @@ struct SingleValueFlowForwardAnalyzer : ValueFlowForwardAnalyzer {
                 if (isAliasOf(var, tok, varid, value))
                     return true;
             }
+        }
         return false;
     }
 
@@ -2529,9 +2564,18 @@ struct VariableForwardAnalyzer : SingleValueFlowForwardAnalyzer {
         : SingleValueFlowForwardAnalyzer(), var(nullptr)
     {}
 
-    VariableForwardAnalyzer(const Variable* v, const ValueFlow::Value& val, const TokenList* t)
+    VariableForwardAnalyzer(const Variable* v, const ValueFlow::Value& val, std::vector<const Variable*> paliases, const TokenList* t)
         : SingleValueFlowForwardAnalyzer(val, t), var(v) {
         varids[var->declarationId()] = var;
+        for(const Variable* av:paliases) {
+            if (!av)
+                continue;
+            aliases[av->declarationId()] = av;
+        }
+    }
+
+    virtual const ValueType* getValueType(const Token*) const OVERRIDE {
+        return var->valueType();
     }
 
     virtual bool match(const Token* tok) const OVERRIDE {
@@ -2545,6 +2589,20 @@ struct VariableForwardAnalyzer : SingleValueFlowForwardAnalyzer {
     }
 };
 
+static void valueFlowForwardVariable(Token* const startToken,
+                                     const Token* const endToken,
+                                     const Variable* const var,
+                                     std::list<ValueFlow::Value> values,
+                                     std::vector<const Variable*> aliases,
+                                     TokenList* const tokenlist,
+                                     const Settings* const settings)
+{
+    for (ValueFlow::Value& v : values) {
+        VariableForwardAnalyzer a(var, v, aliases, tokenlist);
+        valueFlowGenericForward(startToken, endToken, a, settings);
+    }
+}
+
 static bool valueFlowForwardVariable(Token* const startToken,
                                      const Token* const endToken,
                                      const Variable* const var,
@@ -2556,10 +2614,25 @@ static bool valueFlowForwardVariable(Token* const startToken,
                                      ErrorLogger* const,
                                      const Settings* const settings)
 {
-    for (ValueFlow::Value& v : values) {
+    std::vector<const Variable*> aliases;
-        VariableForwardAnalyzer a(var, v, tokenlist);
+    for (const ValueFlow::Value& v : values) {
-        valueFlowGenericForward(startToken, endToken, a, settings);
+        if (!v.tokvalue) 
+            continue;
+        const Token* lifeTok = nullptr;
+        for(const ValueFlow::Value& lv:v.tokvalue->values()) {
+            if (!lv.isLocalLifetimeValue())
+                continue;
+            if (lifeTok) {
+                lifeTok = nullptr;
+                break;
             }
+            lifeTok = lv.tokvalue;
+        }
+        if (lifeTok && lifeTok->variable()) {
+            aliases.push_back(lifeTok->variable());
+        }
+    }
+    valueFlowForwardVariable(startToken, endToken, var, std::move(values), aliases, tokenlist, settings);
     return true;
 }
 
@@ -2578,6 +2651,10 @@ struct ExpressionForwardAnalyzer : SingleValueFlowForwardAnalyzer {
         setupExprVarIds();
     }
 
+    virtual const ValueType* getValueType(const Token*) const OVERRIDE {
+        return expr->valueType();
+    }
+
     static bool nonLocal(const Variable* var, bool deref) {
         return !var || (!var->isLocal() && !var->isArgument()) || (deref && var->isArgument() && var->isPointer()) || var->isStatic() || var->isReference() || var->isExtern();
     }

test/testnullpointer.cpp

@@ -98,6 +98,7 @@ private:
         TEST_CASE(nullpointer55); // #8144
         TEST_CASE(nullpointer56); // #9701
         TEST_CASE(nullpointer57); // #9751
+        TEST_CASE(nullpointer58); // #9807
         TEST_CASE(nullpointer_addressOf); // address of
         TEST_CASE(nullpointerSwitch); // #2626
         TEST_CASE(nullpointer_cast); // #4692
@@ -1840,6 +1841,16 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void nullpointer58() {
+        check("struct myStruct { char entry[0]; };\n"
+              "void f() {\n"
+              "    struct myStruct* sPtr = NULL;\n"
+              "    int sz = (!*(&sPtr) || ((*(&sPtr))->entry[0] > 15)) ?\n"
+              "        sizeof((*(&sPtr))->entry[0]) : 123456789;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
+
     void nullpointer_addressOf() { // address of
         check("void f() {\n"
               "  struct X *x = 0;\n"