Track variable lifetime through function calls (#1481)

lib/valueflow.cpp

@@ -2468,61 +2468,19 @@ static bool valueFlowForward(Token * const               startToken,
     return true;
 }
 
-static bool isNotLifetimeValue(const ValueFlow::Value& val)
+static const Variable *getLifetimeVariable(const Token *tok, ErrorPath &errorPath)
 {
-    return !val.isLifetimeValue();
+    const Variable *var = tok->variable();
-}
-
-static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings)
-{
-    const Token * assignTok = tok->astParent();
-    // Assignment
-    if (!assignTok || (assignTok->str() != "=") || assignTok->astParent())
-        return;
-
-    // Lhs should be a variable
-    if (!assignTok->astOperand1() || !assignTok->astOperand1()->varId())
-        return;
-    const Variable *var = assignTok->astOperand1()->variable();
-    if (!var || (!var->isLocal() && !var->isGlobal() && !var->isArgument()))
-        return;
-
-    const Token * const endOfVarScope = var->typeStartToken()->scope()->bodyEnd;
-
-    // Rhs values..
-    if (!assignTok->astOperand2() || assignTok->astOperand2()->values().empty())
-        return;
-
-    if (astIsPointer(assignTok->astOperand2()) && !var->isPointer() && !(var->valueType() && var->valueType()->isIntegral()))
-        return;
-
-    std::list<ValueFlow::Value> values = assignTok->astOperand2()->values();
-
-    // Static variable initialisation?
-    if (var->isStatic() && var->nameToken() == assignTok->astOperand1())
-        changeKnownToPossible(values);
-
-    // Skip RHS
-    const Token * nextExpression = nextAfterAstRightmostLeaf(assignTok);
-
-    // Only forward lifetime values
-    values.remove_if(&isNotLifetimeValue);
-    valueFlowForward(const_cast<Token *>(nextExpression), endOfVarScope, var, var->declarationId(), values, false, false, tokenlist, errorLogger, settings);
-}
-
-static const Variable * getLifetimeVariable(const Token * tok, ErrorPath& errorPath)
-{
-    const Variable * var = tok->variable();
     if (!var)
         return nullptr;
     if (var->isReference() || var->isRValueReference()) {
-        for (const ValueFlow::Value& v:tok->values()) {
+        for (const ValueFlow::Value &v : tok->values()) {
             if (!v.isLifetimeValue())
                 continue;
             if (v.tokvalue == tok)
                 continue;
             errorPath.insert(errorPath.end(), v.errorPath.begin(), v.errorPath.end());
-            const Variable * var2 = getLifetimeVariable(v.tokvalue, errorPath);
+            const Variable *var2 = getLifetimeVariable(v.tokvalue, errorPath);
             if (var2)
                 return var2;
         }
@@ -2531,6 +2489,150 @@ static const Variable * getLifetimeVariable(const Token * tok, ErrorPath& errorP
     return var;
 }
 
+static bool isNotLifetimeValue(const ValueFlow::Value& val)
+{
+    return !val.isLifetimeValue();
+}
+
+static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings);
+
+static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings)
+{
+    const Token *parent = tok->astParent();
+    while (parent && parent->isArithmeticalOp())
+        parent = parent->astParent();
+    if (!parent)
+        return;
+    // Assignment
+    if (parent->str() == "=" && !parent->astParent()) {
+        // Lhs should be a variable
+        if (!parent->astOperand1() || !parent->astOperand1()->varId())
+            return;
+        const Variable *var = parent->astOperand1()->variable();
+        if (!var || (!var->isLocal() && !var->isGlobal() && !var->isArgument()))
+            return;
+
+        const Token *const endOfVarScope = var->typeStartToken()->scope()->bodyEnd;
+
+        // Rhs values..
+        if (!parent->astOperand2() || parent->astOperand2()->values().empty())
+            return;
+
+        if (astIsPointer(parent->astOperand2()) && !var->isPointer() &&
+            !(var->valueType() && var->valueType()->isIntegral()))
+            return;
+
+        std::list<ValueFlow::Value> values = parent->astOperand2()->values();
+
+        // Static variable initialisation?
+        if (var->isStatic() && var->nameToken() == parent->astOperand1())
+            changeKnownToPossible(values);
+
+        // Skip RHS
+        const Token *nextExpression = nextAfterAstRightmostLeaf(parent);
+
+        // Only forward lifetime values
+        values.remove_if(&isNotLifetimeValue);
+        valueFlowForward(const_cast<Token *>(nextExpression),
+                         endOfVarScope,
+                         var,
+                         var->declarationId(),
+                         values,
+                         false,
+                         false,
+                         tokenlist,
+                         errorLogger,
+                         settings);
+        // Function call
+    } else if (Token::Match(parent->previous(), "%name% (")) {
+        valueFlowLifetimeFunction(const_cast<Token *>(parent->previous()), tokenlist, errorLogger, settings);
+    }
+}
+
+struct LifetimeStore {
+    const Token *argtok;
+    std::string message;
+    ValueFlow::Value::LifetimeKind type;
+
+    template <class Predicate>
+    void byRef(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings, Predicate pred) const {
+        ErrorPath errorPath;
+        const Variable *var = getLifetimeVariable(argtok, errorPath);
+        if (!var)
+            return;
+        if (!pred(var))
+            return;
+        errorPath.emplace_back(argtok, message);
+
+        ValueFlow::Value value;
+        value.valueType = ValueFlow::Value::LIFETIME;
+        value.tokvalue = var->nameToken();
+        value.errorPath = errorPath;
+        value.lifetimeKind = type;
+        // Dont add the value a second time
+        if (std::find(tok->values().begin(), tok->values().end(), value) != tok->values().end())
+            return;
+        setTokenValue(tok, value, tokenlist->getSettings());
+        valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings);
+    }
+
+    void byRef(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) const {
+        byRef(tok, tokenlist, errorLogger, settings, [](const Variable *) {
+            return true;
+        });
+    }
+
+    template <class Predicate>
+    void byVal(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings, Predicate pred) const {
+        for (const ValueFlow::Value &v : argtok->values()) {
+            if (!v.isLifetimeValue())
+                continue;
+            const Token *tok3 = v.tokvalue;
+            ErrorPath errorPath = v.errorPath;
+            const Variable *var = getLifetimeVariable(tok3, errorPath);
+            if (!var)
+                continue;
+            if (!pred(var))
+                return;
+            errorPath.emplace_back(argtok, message);
+
+            ValueFlow::Value value;
+            value.valueType = ValueFlow::Value::LIFETIME;
+            value.tokvalue = var->nameToken();
+            value.errorPath = errorPath;
+            value.lifetimeKind = type;
+            // Dont add the value a second time
+            if (std::find(tok->values().begin(), tok->values().end(), value) != tok->values().end())
+                continue;
+            setTokenValue(tok, value, tokenlist->getSettings());
+            valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings);
+        }
+    }
+
+    void byVal(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) const {
+        byVal(tok, tokenlist, errorLogger, settings, [](const Variable *) {
+            return true;
+        });
+    }
+};
+
+static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings)
+{
+    if (!Token::Match(tok, "%name% ("))
+        return;
+    if (Token::Match(tok->tokAt(-2), "std :: ref|cref|tie|front_inserter|back_inserter")) {
+        for (const Token *argtok : getArguments(tok)) {
+            LifetimeStore{argtok, "Passed to '" + tok->str() + "'.", ValueFlow::Value::Object} .byRef(
+                tok->next(), tokenlist, errorLogger, settings);
+        }
+    } else if (Token::Match(tok->tokAt(-2), "std :: make_tuple|tuple_cat|make_pair|make_reverse_iterator|next|prev|move")) {
+        for (const Token *argtok : getArguments(tok)) {
+            LifetimeStore{argtok, "Passed to '" + tok->str() + "'.", ValueFlow::Value::Object} .byVal(
+                tok->next(), tokenlist, errorLogger, settings);
+        }
+    }
+}
+
 struct Lambda {
     explicit Lambda(const Token * tok)
         : capture(nullptr), arguments(nullptr), returnTok(nullptr), bodyTok(nullptr) {
@@ -2570,6 +2672,16 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger
 
             std::set<const Scope *> scopes;
 
+            auto isCapturingVariable = [&](const Variable *var) {
+                const Scope *scope = var->scope();
+                if (scopes.count(scope) > 0)
+                    return false;
+                if (scope->isNestedIn(bodyScope))
+                    return false;
+                scopes.insert(scope);
+                return true;
+            };
+
             // TODO: Handle explicit capture
             bool captureByRef = Token::Match(lam.capture, "[ & ]");
             bool captureByValue = Token::Match(lam.capture, "[ = ]");
@@ -2577,51 +2689,11 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger
             for (const Token * tok2 = lam.bodyTok; tok2 != lam.bodyTok->link(); tok2 = tok2->next()) {
                 ErrorPath errorPath;
                 if (captureByRef) {
-                    const Variable * var = getLifetimeVariable(tok2, errorPath);
+                    LifetimeStore{tok2, "Lambda captures variable by reference here.", ValueFlow::Value::Lambda} .byRef(
-                    if (!var)
+                        tok, tokenlist, errorLogger, settings, isCapturingVariable);
-                        continue;
-                    const Scope * scope = var->scope();
-                    if (scopes.count(scope) > 0)
-                        continue;
-                    if (scope->isNestedIn(bodyScope))
-                        continue;
-                    scopes.insert(scope);
-                    errorPath.emplace_back(tok2, "Lambda captures variable by reference here.");
-
-                    ValueFlow::Value value;
-                    value.valueType = ValueFlow::Value::LIFETIME;
-                    value.tokvalue = var->nameToken();
-                    value.errorPath = errorPath;
-                    value.lifetimeKind = ValueFlow::Value::Lambda;
-                    setTokenValue(tok, value, tokenlist->getSettings());
-
-                    valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings);
                 } else if (captureByValue) {
-                    for (const ValueFlow::Value& v:tok2->values()) {
+                    LifetimeStore{tok2, "Lambda captures variable by value here.", ValueFlow::Value::Lambda} .byVal(
-                        if (!v.isLifetimeValue() && !v.tokvalue)
+                        tok, tokenlist, errorLogger, settings, isCapturingVariable);
-                            continue;
-                        const Token * tok3 = v.tokvalue;
-                        errorPath = v.errorPath;
-                        const Variable * var = getLifetimeVariable(tok3, errorPath);
-                        if (!var)
-                            continue;
-                        const Scope * scope = var->scope();
-                        if (scopes.count(scope) > 0)
-                            continue;
-                        if (scope->isNestedIn(bodyScope))
-                            continue;
-                        scopes.insert(scope);
-                        errorPath.emplace_back(tok2, "Lambda captures variable by value here.");
-
-                        ValueFlow::Value value;
-                        value.valueType = ValueFlow::Value::LIFETIME;
-                        value.tokvalue = var->nameToken();
-                        value.errorPath = errorPath;
-                        value.lifetimeKind = ValueFlow::Value::Lambda;
-                        setTokenValue(tok, value, tokenlist->getSettings());
-
-                        valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings);
-                    }
                 }
             }
         }
@@ -2681,6 +2753,10 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger
             valueFlowForwardLifetime(tok->tokAt(3), tokenlist, errorLogger, settings);
 
         }
+        // Check function calls
+        else if (Token::Match(tok, "%name% (")) {
+            valueFlowLifetimeFunction(tok, tokenlist, errorLogger, settings);
+        }
         // Check variables
         else if (tok->variable()) {
             ErrorPath errorPath;

test/testautovariables.cpp

@@ -123,6 +123,7 @@ private:
         TEST_CASE(danglingLifetimeLambda);
         TEST_CASE(danglingLifetimeContainer);
         TEST_CASE(danglingLifetime);
+        TEST_CASE(danglingLifetimeFunction);
         TEST_CASE(invalidLifetime);
     }
 
@@ -1319,6 +1320,33 @@ private:
               "}\n");
         ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:1] -> [test.cpp:3]: (error) Returning iterator to local container 'x' that will be invalid when returning.\n", errout.str());
 
+        check("auto f() {\n"
+              "    std::vector<int> x;\n"
+              "    auto it = x.begin();\n"
+              "    return std::next(it);\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[test.cpp:3] -> [test.cpp:4] -> [test.cpp:2] -> [test.cpp:4]: (error) Returning object that points to local variable 'x' that will be invalid when returning.\n",
+            errout.str());
+
+        check("auto f() {\n"
+              "    std::vector<int> x;\n"
+              "    auto it = x.begin();\n"
+              "    return it + 1;\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[test.cpp:3] -> [test.cpp:2] -> [test.cpp:4]: (error) Returning iterator to local container 'x' that will be invalid when returning.\n",
+            errout.str());
+
+        check("auto f() {\n"
+              "    std::vector<int> x;\n"
+              "    auto it = x.begin();\n"
+              "    return std::next(it + 1);\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[test.cpp:3] -> [test.cpp:4] -> [test.cpp:2] -> [test.cpp:4]: (error) Returning object that points to local variable 'x' that will be invalid when returning.\n",
+            errout.str());
+
         check("auto f() {\n"
               "  static std::vector<int> x;\n"
               "  return x.begin();\n"
@@ -1383,6 +1411,31 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void danglingLifetimeFunction() {
+        check("auto f() {\n"
+              "    int a;\n"
+              "    return std::ref(a);\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[test.cpp:3] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning object that points to local variable 'a' that will be invalid when returning.\n",
+            errout.str());
+
+        check("auto f() {\n"
+              "    int a;\n"
+              "    return std::make_tuple(std::ref(a));\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[test.cpp:3] -> [test.cpp:3] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning object that points to local variable 'a' that will be invalid when returning.\n",
+            errout.str());
+
+        check("auto f(int x) {\n"
+              "    int a;\n"
+              "    std::tie(a) = x;\n"
+              "    return a;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
+
     void invalidLifetime() {
         check("void foo(int a) {\n"
               "    std::function<void()> f;\n"