Propagate partially uninit variables in ValueFlow (#3533)

lib/analyzer.h

@@ -50,6 +50,7 @@ struct Analyzer {
             Idempotent = (1 << 5),
             Incremental = (1 << 6),
             SymbolicMatch = (1 << 7),
+            Internal = (1 << 8),
         };
 
         void set(unsigned int f, bool state = true) {
@@ -96,6 +97,10 @@ struct Analyzer {
             return get(SymbolicMatch);
         }
 
+        bool isInternal() const {
+            return get(Internal);
+        }
+
         bool matches() const {
             return get(Match);
         }

lib/checkuninitvar.cpp

@@ -1484,6 +1484,37 @@ void CheckUninitVar::uninitvarError(const Token *tok, const std::string &varname
     reportError(errorPath, Severity::error, "uninitvar", "$symbol:" + varname + "\nUninitialized variable: $symbol", CWE_USE_OF_UNINITIALIZED_VARIABLE, Certainty::normal);
 }
 
+void CheckUninitVar::uninitvarError(const Token* tok, const ValueFlow::Value& v)
+{
+    const Token* ltok = tok;
+    if (tok && Token::simpleMatch(tok->astParent(), ".") && astIsRHS(tok))
+        ltok = tok->astParent();
+    const std::string& varname = ltok ? ltok->expressionString() : "x";
+    ErrorPath errorPath = v.errorPath;
+    errorPath.emplace_back(tok, "");
+    if (v.subexpressions.empty()) {
+        reportError(errorPath,
+                    Severity::error,
+                    "uninitvar",
+                    "$symbol:" + varname + "\nUninitialized variable: $symbol",
+                    CWE_USE_OF_UNINITIALIZED_VARIABLE,
+                    Certainty::normal);
+        return;
+    }
+    std::string vars = v.subexpressions.size() == 1 ? "variable: " : "variables: ";
+    std::string prefix;
+    for (const std::string& var : v.subexpressions) {
+        vars += prefix + varname + "." + var;
+        prefix = ", ";
+    }
+    reportError(errorPath,
+                Severity::error,
+                "uninitvar",
+                "$symbol:" + varname + "\nUninitialized " + vars,
+                CWE_USE_OF_UNINITIALIZED_VARIABLE,
+                Certainty::normal);
+}
+
 void CheckUninitVar::uninitStructMemberError(const Token *tok, const std::string &membername)
 {
     reportError(tok,
@@ -1492,20 +1523,34 @@ void CheckUninitVar::uninitStructMemberError(const Token *tok, const std::string
                 "$symbol:" + membername + "\nUninitialized struct member: $symbol", CWE_USE_OF_UNINITIALIZED_VARIABLE, Certainty::normal);
 }
 
-static bool isUsedByFunction(const Token* tok, int indirect, const Settings* settings)
+enum class FunctionUsage { None, PassedByReference, Used };
+
+static FunctionUsage getFunctionUsage(const Token* tok, int indirect, const Settings* settings)
 {
     const bool addressOf = tok->astParent() && tok->astParent()->isUnaryOp("&");
 
     int argnr;
     const Token* ftok = getTokenArgumentFunction(tok, argnr);
     if (!ftok)
-        return false;
+        return FunctionUsage::None;
-    const bool isnullbad = settings->library.isnullargbad(ftok, argnr + 1);
+    if (ftok->function()) {
-    if (indirect == 0 && astIsPointer(tok) && !addressOf && isnullbad)
+        std::vector<const Variable*> args = getArgumentVars(ftok, argnr);
-        return true;
+        for (const Variable* arg : args) {
-    bool hasIndirect = false;
+            if (!arg)
-    const bool isuninitbad = settings->library.isuninitargbad(ftok, argnr + 1, indirect, &hasIndirect);
+                continue;
-    return isuninitbad && (!addressOf || isnullbad);
+            if (arg->isReference())
+                return FunctionUsage::PassedByReference;
+        }
+    } else {
+        const bool isnullbad = settings->library.isnullargbad(ftok, argnr + 1);
+        if (indirect == 0 && astIsPointer(tok) && !addressOf && isnullbad)
+            return FunctionUsage::Used;
+        bool hasIndirect = false;
+        const bool isuninitbad = settings->library.isuninitargbad(ftok, argnr + 1, indirect, &hasIndirect);
+        if (isuninitbad && (!addressOf || isnullbad))
+            return FunctionUsage::Used;
+    }
+    return FunctionUsage::None;
 }
 
 static bool isLeafDot(const Token* tok)
@@ -1573,7 +1618,10 @@ void CheckUninitVar::valueFlowUninit()
                     if (Token::Match(tok->astParent(), ". %var%") && !isleaf)
                         continue;
                 }
-                if (!isUsedByFunction(tok, v->indirect, mSettings)) {
+                FunctionUsage fusage = getFunctionUsage(tok, v->indirect, mSettings);
+                if (!v->subexpressions.empty() && fusage == FunctionUsage::PassedByReference)
+                    continue;
+                if (fusage != FunctionUsage::Used) {
                     if (!(Token::Match(tok->astParent(), ". %name% (") && uninitderef) &&
                         isVariableChanged(tok, v->indirect, mSettings, mTokenizer->isCPP()))
                         continue;
@@ -1581,10 +1629,7 @@ void CheckUninitVar::valueFlowUninit()
                     if (isVariableChangedByFunctionCall(tok, v->indirect, mSettings, &inconclusive) || inconclusive)
                         continue;
                 }
-                const Token* ltok = tok;
+                uninitvarError(tok, *v);
-                if (Token::simpleMatch(tok->astParent(), ".") && astIsRHS(tok))
-                    ltok = tok->astParent();
-                uninitvarError(ltok, ltok->expressionString(), v->errorPath);
                 ids.insert(tok->exprId());
                 if (v->tokvalue)
                     ids.insert(v->tokvalue->exprId());

lib/checkuninitvar.h

@@ -110,6 +110,7 @@ public:
     /** @brief Analyse all file infos for all TU */
     bool analyseWholeProgram(const CTU::FileInfo *ctu, const std::list<Check::FileInfo*> &fileInfo, const Settings& settings, ErrorLogger &errorLogger) OVERRIDE;
 
+    void uninitvarError(const Token* tok, const ValueFlow::Value& v);
     void uninitstringError(const Token *tok, const std::string &varname, bool strncpy_);
     void uninitdataError(const Token *tok, const std::string &varname);
     void uninitvarError(const Token *tok, const std::string &varname, ErrorPath errorPath);
@@ -132,10 +133,12 @@ private:
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const OVERRIDE {
         CheckUninitVar c(nullptr, settings, errorLogger);
 
+        ValueFlow::Value v{};
+
         // error
+        c.uninitvarError(nullptr, v);
         c.uninitstringError(nullptr, "varname", true);
         c.uninitdataError(nullptr, "varname");
-        c.uninitvarError(nullptr, "varname");
         c.uninitStructMemberError(nullptr, "a.b");
     }
 

lib/valueflow.cpp

@@ -564,6 +564,12 @@ static void setTokenValue(Token* tok, ValueFlow::Value value, const Settings* se
         if (Token::Match(tok, ". %var%"))
             setTokenValue(tok->next(), value, settings);
         ValueFlow::Value pvalue = value;
+        if (!value.subexpressions.empty()) {
+            if (Token::Match(parent, ". %var%") && contains(value.subexpressions, parent->next()->str()))
+                pvalue.subexpressions.clear();
+        }
+        if (!pvalue.subexpressions.empty())
+            return;
         if (parent->isUnaryOp("&")) {
             pvalue.indirect++;
             setTokenValue(parent, pvalue, settings);
@@ -1957,6 +1963,10 @@ struct ValueFlowAnalyzer : Analyzer {
 
     virtual bool match(const Token* tok) const = 0;
 
+    virtual bool internalMatch(const Token*) const {
+        return false;
+    }
+
     virtual bool isAlias(const Token* tok, bool& inconclusive) const = 0;
 
     using ProgramState = std::unordered_map<nonneg int, ValueFlow::Value>;
@@ -2332,6 +2342,8 @@ struct ValueFlowAnalyzer : Analyzer {
         const bool inconclusiveRefs = refs.size() != 1;
         for (const ReferenceToken& ref:refs) {
             Action a = analyzeToken(ref.token, tok, d, inconclusiveRefs);
+            if (internalMatch(ref.token))
+                a |= Action::Internal;
             if (a != Action::None)
                 return a;
         }
@@ -2428,6 +2440,11 @@ struct ValueFlowAnalyzer : Analyzer {
             makeConditional();
     }
 
+    virtual void internalUpdate(Token*, const ValueFlow::Value&, Direction)
+    {
+        assert(false && "Internal update unimplemented.");
+    }
+
     virtual void update(Token* tok, Action a, Direction d) OVERRIDE {
         ValueFlow::Value* value = getValue(tok);
         if (!value)
@@ -2439,6 +2456,8 @@ struct ValueFlowAnalyzer : Analyzer {
             value = &localValue;
             isSameSymbolicValue(tok, &localValue);
         }
+        if (a.isInternal())
+            internalUpdate(tok, *value, d);
         // Read first when moving forward
         if (d == Direction::Forward && a.isRead())
             setTokenValue(tok, *value, getSettings());
@@ -2699,10 +2718,13 @@ struct OppositeExpressionAnalyzer : ExpressionAnalyzer {
 };
 
 struct SubExpressionAnalyzer : ExpressionAnalyzer {
-    SubExpressionAnalyzer() : ExpressionAnalyzer() {}
+    using PartialReadContainer = std::vector<std::pair<Token *, ValueFlow::Value>>;
+    // A shared_ptr is used so partial reads can be captured even after forking
+    std::shared_ptr<PartialReadContainer> partialReads;
+    SubExpressionAnalyzer() : ExpressionAnalyzer(), partialReads(nullptr) {}
 
     SubExpressionAnalyzer(const Token* e, const ValueFlow::Value& val, const TokenList* t)
-        : ExpressionAnalyzer(e, val, t)
+        : ExpressionAnalyzer(e, val, t), partialReads(std::make_shared<PartialReadContainer>())
     {}
 
     virtual bool submatch(const Token* tok, bool exact = true) const = 0;
@@ -2718,6 +2740,14 @@ struct SubExpressionAnalyzer : ExpressionAnalyzer {
     {
         return tok->astOperand1() && tok->astOperand1()->exprId() == expr->exprId() && submatch(tok);
     }
+    virtual bool internalMatch(const Token* tok) const OVERRIDE
+    {
+        return tok->exprId() == expr->exprId() && !(astIsLHS(tok) && submatch(tok->astParent(), false));
+    }
+    virtual void internalUpdate(Token* tok, const ValueFlow::Value& v, Direction) OVERRIDE
+    {
+        partialReads->push_back(std::make_pair(tok, v));
+    }
 
     // No reanalysis for subexression
     virtual ValuePtr<Analyzer> reanalyze(Token*, const std::string&) const OVERRIDE {
@@ -6387,6 +6417,19 @@ static bool needsInitialization(const Variable* var, bool cpp)
     return false;
 }
 
+static void addToErrorPath(ValueFlow::Value& value, const ValueFlow::Value& from)
+{
+    std::unordered_set<const Token*> locations;
+    if (from.condition && !value.condition)
+        value.condition = from.condition;
+    std::copy_if(from.errorPath.begin(),
+                 from.errorPath.end(),
+                 std::back_inserter(value.errorPath),
+                 [&](const ErrorPathItem& e) {
+        return locations.insert(e.first).second;
+    });
+}
+
 static void valueFlowUninit(TokenList* tokenlist, SymbolDatabase* /*symbolDatabase*/, const Settings* settings)
 {
     for (Token *tok = tokenlist->front(); tok; tok = tok->next()) {
@@ -6423,6 +6466,7 @@ static void valueFlowUninit(TokenList* tokenlist, SymbolDatabase* /*symbolDataba
 
         bool partial = false;
 
+        std::map<Token*, ValueFlow::Value> partialReads;
         if (const Scope* scope = var->typeScope()) {
             if (Token::findsimplematch(scope->bodyStart, "union", scope->bodyEnd))
                 continue;
@@ -6435,9 +6479,32 @@ static void valueFlowUninit(TokenList* tokenlist, SymbolDatabase* /*symbolDataba
                 }
                 MemberExpressionAnalyzer analyzer(memVar.nameToken()->str(), vardecl, uninitValue, tokenlist);
                 valueFlowGenericForward(vardecl->next(), vardecl->scope()->bodyEnd, analyzer, settings);
+
+                for (auto&& p : *analyzer.partialReads) {
+                    Token* tok2 = p.first;
+                    const ValueFlow::Value& v = p.second;
+                    // Try to insert into map
+                    auto pp = partialReads.insert(std::make_pair(tok2, v));
+                    ValueFlow::Value& v2 = pp.first->second;
+                    bool inserted = pp.second;
+                    // Merge the two values if it is already in map
+                    if (!inserted) {
+                        if (v.valueType != v2.valueType)
+                            continue;
+                        addToErrorPath(v2, v);
+                    }
+                    v2.subexpressions.push_back(memVar.nameToken()->str());
+                }
             }
         }
 
+        for (auto&& p : partialReads) {
+            Token* tok2 = p.first;
+            const ValueFlow::Value& v = p.second;
+
+            setTokenValue(tok2, v, settings);
+        }
+
         if (partial)
             continue;
 
@@ -7370,6 +7437,7 @@ ValueFlow::Value::Value(const Token* c, long long val, Bound b)
     indirect(0),
     path(0),
     wideintvalue(0),
+    subexpressions(),
     lifetimeKind(LifetimeKind::Object),
     lifetimeScope(LifetimeScope::Local),
     valueKind(ValueKind::Possible)

lib/valueflow.h

@@ -98,6 +98,7 @@ namespace ValueFlow {
             indirect(0),
             path(0),
             wideintvalue(val),
+            subexpressions(),
             lifetimeKind(LifetimeKind::Object),
             lifetimeScope(LifetimeScope::Local),
             valueKind(ValueKind::Possible)
@@ -352,6 +353,8 @@ namespace ValueFlow {
         /** int value before implicit truncation */
         long long wideintvalue;
 
+        std::vector<std::string> subexpressions;
+
         enum class LifetimeKind {
             // Pointer points to a member of lifetime
             Object,

test/testsuppressions.cpp

@@ -81,6 +81,9 @@ private:
     Suppressions::ErrorMessage errorMessage(const std::string &errorId) const {
         Suppressions::ErrorMessage ret;
         ret.errorId = errorId;
+        ret.hash = 0;
+        ret.lineNumber = 0;
+        ret.certainty = Certainty::CertaintyLevel::normal;
         return ret;
     }
 

test/testuninitvar.cpp

@@ -4543,7 +4543,7 @@ private:
                         "        p = new S(io);\n"
                         "    p->Write();\n"
                         "}");
-        ASSERT_EQUALS("[test.cpp:8] -> [test.cpp:10]: (error) Uninitialized variable: p\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:8] -> [test.cpp:10]: (error) Uninitialized variable: p.rIo\n", errout.str());
 
         // Unknown types
         {
@@ -5229,6 +5229,15 @@ private:
                         "}");
         ASSERT_EQUALS("", errout.str());
 
+        valueFlowUninit("struct AB { int a; int b; };\n"
+                        "void do_something(const struct AB &ab) { a = ab.b; }\n"
+                        "void f(void) {\n"
+                        "    struct AB ab;\n"
+                        "    ab.a = 0;\n"
+                        "    do_something(ab);\n"
+                        "}");
+        ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:2]: (error) Uninitialized variable: ab.b\n", errout.str());
+
         valueFlowUninit("struct AB { int a; int b; };\n"
                         "void f(void) {\n"
                         "    struct AB ab;\n"
@@ -5252,6 +5261,97 @@ private:
                         "test.c");
         ASSERT_EQUALS("[test.c:4]: (error) Uninitialized variable: ab.a\n", errout.str());
 
+        valueFlowUninit("struct AB { int a; int b; };\n"
+                        "void f(void) {\n"
+                        "    struct AB ab;\n"
+                        "    ab.a = 1;\n"
+                        "    x = ab;\n"
+                        "}\n",
+                        "test.c");
+        ASSERT_EQUALS("[test.c:5]: (error) Uninitialized variable: ab.b\n", errout.str());
+
+        valueFlowUninit("struct AB { int a; int b; };\n"
+                        "void f(void) {\n"
+                        "    struct AB ab;\n"
+                        "    ab.a = 1;\n"
+                        "    x = *(&ab);\n"
+                        "}\n",
+                        "test.c");
+        ASSERT_EQUALS("[test.c:5] -> [test.c:5]: (error) Uninitialized variable: *(&ab).b\n", errout.str());
+
+        valueFlowUninit("void f(void) {\n"
+                        "    struct AB ab;\n"
+                        "    int x;\n"
+                        "    ab.a = (void*)&x;\n"
+                        "    dostuff(&ab,0);\n"
+                        "}\n",
+                        "test.c");
+        ASSERT_EQUALS("", errout.str());
+
+        valueFlowUninit("struct Element {\n"
+                        "    static void f() { }\n"
+                        "};\n"
+                        "void test() {\n"
+                        "    Element *element; element->f();\n"
+                        "}");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: element\n", errout.str());
+
+        valueFlowUninit("struct Element {\n"
+                        "    static void f() { }\n"
+                        "};\n"
+                        "void test() {\n"
+                        "    Element *element; (*element).f();\n"
+                        "}");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: element\n", errout.str());
+
+        valueFlowUninit("struct Element {\n"
+                        "    static int v;\n"
+                        "};\n"
+                        "void test() {\n"
+                        "    Element *element; element->v;\n"
+                        "}");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: element\n", errout.str());
+
+        valueFlowUninit("struct Element {\n"
+                        "    static int v;\n"
+                        "};\n"
+                        "void test() {\n"
+                        "    Element *element; (*element).v;\n"
+                        "}");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: element\n", errout.str());
+
+        valueFlowUninit("struct Element {\n"
+                        "    void f() { }\n"
+                        "};\n"
+                        "void test() {\n"
+                        "    Element *element; element->f();\n"
+                        "}");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: element\n", errout.str());
+
+        valueFlowUninit("struct Element {\n"
+                        "    void f() { }\n"
+                        "};\n"
+                        "void test() {\n"
+                        "    Element *element; (*element).f();\n"
+                        "}");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: element\n", errout.str());
+
+        valueFlowUninit("struct Element {\n"
+                        "    int v;\n"
+                        "};\n"
+                        "void test() {\n"
+                        "    Element *element; element->v;\n"
+                        "}");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: element\n", errout.str());
+
+        valueFlowUninit("struct Element {\n"
+                        "    int v;\n"
+                        "};\n"
+                        "void test() {\n"
+                        "    Element *element; (*element).v;\n"
+                        "}");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: element\n", errout.str());
+
         valueFlowUninit("struct AB { int a; int b; };\n" // pass struct member by address
                         "void f(void) {\n"
                         "    struct AB ab;\n"
@@ -5472,7 +5572,7 @@ private:
                         "    s.a = 0;\n"
                         "    return s;\n"
                         "}\n");
-        TODO_ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: s.b\n", "", errout.str());
+        ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: s.b\n", errout.str());
 
         valueFlowUninit("struct S { int a; int b; };\n" // #9810
                         "void f(void) {\n"