Fix 10418: false negative: knownConditionTrueFalse (#3981)

* Improve handling inverted condition

* Fix tests

* Rename variables for clarity

* Add initial test

* Add another test

* Format

* Fix FP

lib/valueflow.cpp

@@ -5437,6 +5437,10 @@ struct ConditionHandler {
         // Whether to insert impossible values for the condition or only use possible values
         bool impossible = true;
 
+        bool isBool() const {
+            return astIsBool(vartok);
+        }
+
         Condition() : vartok(nullptr), true_values(), false_values(), inverted(false), impossible(true) {}
     };
 
@@ -5604,39 +5608,65 @@ struct ConditionHandler {
         });
     }
 
+    static Token* skipNotAndCasts(Token* tok, bool* inverted = nullptr)
+    {
+        for (; tok->astParent(); tok = tok->astParent()) {
+            if (Token::simpleMatch(tok->astParent(), "!")) {
+                if (inverted)
+                    *inverted ^= true;
+                continue;
+            }
+            if (Token::Match(tok->astParent(), "==|!=")) {
+                Token* sibling = tok->astSibling();
+                if (sibling->hasKnownIntValue() && (astIsBool(tok) || astIsBool(sibling))) {
+                    bool value = sibling->values().front().intvalue;
+                    if (inverted)
+                        *inverted ^= value == Token::simpleMatch(tok->astParent(), "!=");
+                    continue;
+                }
+            }
+            if (tok->astParent()->isCast() && astIsBool(tok->astParent()))
+                continue;
+            return tok;
+        }
+        return tok;
+    }
+
     void afterCondition(TokenList* tokenlist,
                         SymbolDatabase* symboldatabase,
                         ErrorLogger* errorLogger,
                         const Settings* settings) const {
-        traverseCondition(tokenlist, symboldatabase, [&](const Condition& cond, Token* tok, const Scope* scope) {
+        traverseCondition(tokenlist, symboldatabase, [&](const Condition& cond, Token* condTok, const Scope* scope) {
-            const Token* top = tok->astTop();
+            const Token* top = condTok->astTop();
 
             std::list<ValueFlow::Value> thenValues;
             std::list<ValueFlow::Value> elseValues;
 
-            if (!Token::Match(tok, "!=|=|(|.") && tok != cond.vartok) {
+            if (!Token::Match(condTok, "!=|=|(|.") && condTok != cond.vartok) {
                 thenValues.insert(thenValues.end(), cond.true_values.begin(), cond.true_values.end());
-                if (cond.impossible && isConditionKnown(tok, false))
+                if (cond.impossible && isConditionKnown(condTok, false))
                     insertImpossible(elseValues, cond.false_values);
             }
-            if (!Token::Match(tok, "==|!")) {
+            if (!Token::Match(condTok, "==|!")) {
                 elseValues.insert(elseValues.end(), cond.false_values.begin(), cond.false_values.end());
-                if (cond.impossible && isConditionKnown(tok, true)) {
+                if (cond.impossible && isConditionKnown(condTok, true)) {
                     insertImpossible(thenValues, cond.true_values);
-                    if (tok == cond.vartok && astIsBool(tok))
+                    if (cond.isBool())
                         insertNegateKnown(thenValues, cond.true_values);
                 }
             }
 
-            if (cond.inverted)
+            bool inverted = cond.inverted;
+            Token* ctx = skipNotAndCasts(condTok, &inverted);
+            if (inverted)
                 std::swap(thenValues, elseValues);
 
-            if (Token::Match(tok->astParent(), "%oror%|&&")) {
+            if (Token::Match(ctx->astParent(), "%oror%|&&")) {
-                Token* parent = tok->astParent();
+                Token* parent = ctx->astParent();
-                if (astIsRHS(tok) && astIsLHS(parent) && parent->astParent() &&
+                if (astIsRHS(ctx) && astIsLHS(parent) && parent->astParent() &&
                     parent->str() == parent->astParent()->str())
                     parent = parent->astParent();
-                else if (!astIsLHS(tok)) {
+                else if (!astIsLHS(ctx)) {
                     parent = nullptr;
                 }
                 if (parent) {
@@ -5650,7 +5680,7 @@ struct ConditionHandler {
                         values = thenValues;
                     else if (op == "||")
                         values = elseValues;
-                    if (Token::Match(tok, "==|!=") || (tok == cond.vartok && astIsBool(tok)))
+                    if (Token::Match(condTok, "==|!=") || cond.isBool())
                         changePossibleToKnown(values);
                     if (astIsFloat(cond.vartok, false) ||
                         (!cond.vartok->valueType() &&
@@ -5669,7 +5699,7 @@ struct ConditionHandler {
             }
 
             {
-                const Token* tok2 = tok;
+                const Token* tok2 = condTok;
                 std::string op;
                 bool mixedOperators = false;
                 while (tok2->astParent()) {
@@ -5703,38 +5733,37 @@ struct ConditionHandler {
                 }
             }
 
-            // if astParent is "!" we need to invert codeblock
+            Token* condTop = ctx->astParent();
             {
-                const Token* tok2 = tok;
+                bool inverted2 = false;
-                while (tok2 && tok2->astParent() && tok2->astParent()->str() != "?") {
+                while (Token::Match(condTop, "%oror%|&&")) {
-                    const Token* parent = tok2->astParent();
+                    Token* parent = skipNotAndCasts(condTop, &inverted2)->astParent();
-                    while (parent && parent->str() == "&&")
+                    if (!parent)
-                        parent = parent->astParent();
+                        break;
-                    if (parent && (parent->str() == "!" || Token::simpleMatch(parent, "== false")))
+                    condTop = parent;
-                        std::swap(thenValues, elseValues);
-                    tok2 = parent;
                 }
+                if (inverted2)
+                    std::swap(thenValues, elseValues);
             }
 
-            Token* condTop = tok;
+            if (Token::simpleMatch(condTop, "?")) {
-            while (Token::Match(condTop->astParent(), "%oror%|&&|!"))
+                Token* colon = condTop->astOperand2();
-                condTop = condTop->astParent();
-
-            if (Token::simpleMatch(condTop->astParent(), "?")) {
-                Token* colon = condTop->astParent()->astOperand2();
                 forward(colon->astOperand1(), cond.vartok, thenValues, tokenlist, settings);
                 forward(colon->astOperand2(), cond.vartok, elseValues, tokenlist, settings);
                 // TODO: Handle after condition
                 return;
             }
 
+            if (condTop != top && condTop->str() != ";")
+                return;
+
             if (!Token::Match(top->previous(), "if|while|for ("))
                 return;
 
             if (top->previous()->str() == "for") {
-                if (!Token::Match(tok, "%comp%"))
+                if (!Token::Match(condTok, "%comp%"))
                     return;
-                if (!Token::simpleMatch(tok->astParent(), ";"))
+                if (!Token::simpleMatch(condTok->astParent(), ";"))
                     return;
                 const Token* stepTok = getStepTok(top);
                 if (cond.vartok->varId() == 0)
@@ -5753,9 +5782,9 @@ struct ConditionHandler {
                     return;
                 if (Token::simpleMatch(stepTok, "--") && bounds.count(ValueFlow::Value::Bound::Upper) > 0)
                     return;
-                const Token* childTok = tok->astOperand1();
+                const Token* childTok = condTok->astOperand1();
                 if (!childTok)
-                    childTok = tok->astOperand2();
+                    childTok = condTok->astOperand2();
                 if (!childTok)
                     return;
                 if (childTok->varId() != cond.vartok->varId())
@@ -5773,7 +5802,7 @@ struct ConditionHandler {
                 ProgramMemory pm;
                 execute(initTok, &pm, nullptr, nullptr);
                 MathLib::bigint result = 1;
-                execute(tok, &pm, &result, nullptr);
+                execute(condTok, &pm, &result, nullptr);
                 if (result == 0)
                     return;
                 // Remove condition since for condition is not redundant
@@ -5802,7 +5831,7 @@ struct ConditionHandler {
                 if (!startToken)
                     continue;
                 std::list<ValueFlow::Value>& values = (i == 0 ? thenValues : elseValues);
-                valueFlowSetConditionToKnown(tok, values, i == 0);
+                valueFlowSetConditionToKnown(condTok, values, i == 0);
 
                 Analyzer::Result r =
                     forward(startTokens[i], startTokens[i]->link(), cond.vartok, values, tokenlist, settings);
@@ -5837,7 +5866,7 @@ struct ConditionHandler {
                 bool dead_if = deadBranch[0];
                 bool dead_else = deadBranch[1];
                 const Token* unknownFunction = nullptr;
-                if (tok->astParent() && Token::Match(top->previous(), "while|for ("))
+                if (condTok->astParent() && Token::Match(top->previous(), "while|for ("))
                     dead_if = !isBreakScope(after);
                 else if (!dead_if)
                     dead_if = isReturnScope(after, &settings->library, &unknownFunction);
@@ -5883,7 +5912,7 @@ struct ConditionHandler {
                     return;
 
                 if (dead_if || dead_else) {
-                    const Token* parent = tok->astParent();
+                    const Token* parent = condTok->astParent();
                     // Skip the not operator
                     while (Token::simpleMatch(parent, "!"))
                         parent = parent->astParent();
@@ -5901,8 +5930,8 @@ struct ConditionHandler {
                         values.remove_if(std::mem_fn(&ValueFlow::Value::isImpossible));
                         changeKnownToPossible(values);
                     } else {
-                        valueFlowSetConditionToKnown(tok, values, true);
+                        valueFlowSetConditionToKnown(condTok, values, true);
-                        valueFlowSetConditionToKnown(tok, values, false);
+                        valueFlowSetConditionToKnown(condTok, values, false);
                     }
                 }
                 if (values.empty())
@@ -6096,6 +6125,21 @@ static void valueFlowInferCondition(TokenList* tokenlist,
 }
 
 struct SymbolicConditionHandler : SimpleConditionHandler {
+
+    static bool isNegatedBool(const Token* tok)
+    {
+        if (!Token::simpleMatch(tok, "!"))
+            return false;
+        return (astIsBool(tok->astOperand1()));
+    }
+
+    static const Token* skipNot(const Token* tok)
+    {
+        if (!Token::simpleMatch(tok, "!"))
+            return tok;
+        return tok->astOperand1();
+    }
+
     virtual std::vector<Condition> parse(const Token* tok, const Settings*) const override
     {
         if (!Token::Match(tok, "%comp%"))
@@ -6108,27 +6152,36 @@ struct SymbolicConditionHandler : SimpleConditionHandler {
             return {};
 
         std::vector<Condition> result;
-        for (int i = 0; i < 2; i++) {
+        auto addCond = [&](const Token* lhsTok, const Token* rhsTok, bool inverted) {
-            const bool lhs = i == 0;
+            for (int i = 0; i < 2; i++) {
-            const Token* vartok = lhs ? tok->astOperand1() : tok->astOperand2();
+                const bool lhs = i == 0;
-            const Token* valuetok = lhs ? tok->astOperand2() : tok->astOperand1();
+                const Token* vartok = lhs ? lhsTok : rhsTok;
-            if (valuetok->exprId() == 0)
+                const Token* valuetok = lhs ? rhsTok : lhsTok;
-                continue;
+                if (valuetok->exprId() == 0)
-            if (valuetok->hasKnownSymbolicValue(vartok))
+                    continue;
-                continue;
+                if (valuetok->hasKnownSymbolicValue(vartok))
-            if (vartok->hasKnownSymbolicValue(valuetok))
+                    continue;
-                continue;
+                if (vartok->hasKnownSymbolicValue(valuetok))
-            ValueFlow::Value true_value;
+                    continue;
-            ValueFlow::Value false_value;
+                ValueFlow::Value true_value;
-            setConditionalValues(tok, !lhs, 0, true_value, false_value);
+                ValueFlow::Value false_value;
-            setSymbolic(true_value, valuetok);
+                setConditionalValues(tok, !lhs, 0, true_value, false_value);
-            setSymbolic(false_value, valuetok);
+                setSymbolic(true_value, valuetok);
+                setSymbolic(false_value, valuetok);
 
-            Condition cond;
+                Condition cond;
-            cond.true_values = {true_value};
+                cond.true_values = {true_value};
-            cond.false_values = {false_value};
+                cond.false_values = {false_value};
-            cond.vartok = vartok;
+                cond.vartok = vartok;
-            result.push_back(cond);
+                cond.inverted = inverted;
+                result.push_back(cond);
+            }
+        };
+        addCond(tok->astOperand1(), tok->astOperand2(), false);
+        if (Token::Match(tok, "==|!=") && (isNegatedBool(tok->astOperand1()) || isNegatedBool(tok->astOperand2()))) {
+            const Token* lhsTok = skipNot(tok->astOperand1());
+            const Token* rhsTok = skipNot(tok->astOperand2());
+            addCond(lhsTok, rhsTok, !(isNegatedBool(tok->astOperand1()) && isNegatedBool(tok->astOperand2())));
         }
         return result;
     }

test/testcondition.cpp

@@ -4029,6 +4029,13 @@ private:
                       "[test.cpp:4]: (style) Condition 'wcslen(L\"abc\")==3' is always true\n"
                       "[test.cpp:5]: (style) Condition 'wcslen(L\"abc\")==1' is always false\n",
                       errout.str());
+
+        check("int foo(bool a, bool b) {\n"
+              "  if(!a && b && (!a == !b))\n"
+              "   return 1;\n"
+              "  return 0;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Condition '!a==!b' is always false\n", errout.str());
     }
 
     void alwaysTrueSymbolic()
@@ -4167,6 +4174,13 @@ private:
               "        return CMD_OK;\n"
               "}\n");
         ASSERT_EQUALS("", errout.str());
+
+        check("int foo(bool a, bool b) {\n"
+              "  if((!a == !b) && !a && b)\n"
+              "   return 1;\n"
+              "  return 0;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Condition 'b' is always false\n", errout.str());
     }
 
     void alwaysTrueInfer() {

test/testnullpointer.cpp

@@ -135,6 +135,7 @@ private:
         TEST_CASE(nullpointer89); // #10640
         TEST_CASE(nullpointer90); // #6098
         TEST_CASE(nullpointer91); // #10678
+        TEST_CASE(nullpointer92);
         TEST_CASE(nullpointer_addressOf); // address of
         TEST_CASE(nullpointerSwitch); // #2626
         TEST_CASE(nullpointer_cast); // #4692
@@ -2682,6 +2683,23 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void nullpointer92()
+    {
+        check("bool g(bool);\n"
+              "int f(int* i) {\n"
+              "    if (!g(!!i)) return 0;\n"
+              "    return *i;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool g(bool);\n"
+              "int f(int* i) {\n"
+              "    if (!g(!i)) return 0;\n"
+              "    return *i;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
+
     void nullpointer_addressOf() { // address of
         check("void f() {\n"
               "  struct X *x = 0;\n"

test/testvalueflow.cpp

@@ -5495,7 +5495,7 @@ private:
                "  if (v.empty()) {}\n"
                "  if (!v.empty() && v[10]==0) {}\n" // <- no container size for 'v[10]'
                "}";
-        ASSERT(tokenValues(code, "v [").empty());
+        ASSERT(removeImpossible(tokenValues(code, "v [")).empty());
 
         code = "void f() {\n"
                "  std::list<int> ints;\n"  // No value => ints is empty
@@ -6015,7 +6015,9 @@ private:
                "    if (!v.empty() && v[0] != 0) {}\n"
                "    return v;\n"
                "}\n";
-        ASSERT_EQUALS(true, tokenValues(code, "v [ 0 ] != 0 ) { }", ValueFlow::Value::ValueType::CONTAINER_SIZE).empty());
+        ASSERT_EQUALS(
+            true,
+            removeImpossible(tokenValues(code, "v [ 0 ] != 0 ) { }", ValueFlow::Value::ValueType::CONTAINER_SIZE)).empty());
 
         code = "std::vector<int> f() {\n"
                "    std::vector<int> v;\n"