Fix 7812: False negative: return pointer of local variable (#3583)

* Fix 7812: False negative: return pointer of local variable

* Format

* Add test case for 3029

* Format

lib/checkautovariables.cpp

@@ -653,7 +653,7 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token
                             continue;
                         if ((tokvalue->variable() && !isEscapedReference(tokvalue->variable()) &&
                              isInScope(tokvalue->variable()->nameToken(), scope)) ||
-                            isDeadTemporary(mTokenizer->isCPP(), tokvalue, tok, &mSettings->library)) {
+                            isDeadTemporary(mTokenizer->isCPP(), tokvalue, nullptr, &mSettings->library)) {
                             errorReturnDanglingLifetime(tok, &val);
                             break;
                         }

lib/symboldatabase.cpp

@@ -68,9 +68,10 @@ SymbolDatabase::SymbolDatabase(const Tokenizer *tokenizer, const Settings *setti
     createSymbolDatabaseSetScopePointers();
     createSymbolDatabaseSetVariablePointers();
     setValueTypeInTokenList(false);
-    createSymbolDatabaseSetFunctionPointers(true);
     createSymbolDatabaseSetTypePointers();
     createSymbolDatabaseSetSmartPointerType();
+    createSymbolDatabaseSetFunctionPointers(true);
+    setValueTypeInTokenList(false);
     createSymbolDatabaseEnums();
     createSymbolDatabaseEscapeFunctions();
     createSymbolDatabaseIncompleteVars();
@@ -5273,8 +5274,10 @@ const Function* SymbolDatabase::findFunction(const Token *tok) const
 
     // check for member function
     else if (Token::Match(tok->tokAt(-2), "!!this .")) {
-        const Token *tok1 = tok->tokAt(-2);
+        const Token* tok1 = tok->previous()->astOperand1();
-        if (Token::Match(tok1, "%var% .")) {
+        if (tok1 && tok1->valueType() && tok1->valueType()->typeScope) {
+            return tok1->valueType()->typeScope->findFunction(tok, tok1->valueType()->constness == 1);
+        } else if (Token::Match(tok1, "%var% .")) {
             const Variable *var = getVariableFromVarId(tok1->varId());
             if (var && var->typeScope())
                 return var->typeScope()->findFunction(tok, var->valueType()->constness == 1);
@@ -5299,6 +5302,12 @@ const Function* SymbolDatabase::findFunction(const Token *tok) const
             currScope = currScope->nestedIn;
         }
     }
+    // Check for contructor
+    if (Token::Match(tok, "%name% (|{")) {
+        ValueType vt = ValueType::parseDecl(tok, mSettings);
+        if (vt.typeScope)
+            return vt.typeScope->findFunction(tok, false);
+    }
     return nullptr;
 }
 

lib/valueflow.cpp

@@ -3706,6 +3706,9 @@ static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLog
 {
     if (!Token::Match(tok, "%name% ("))
         return;
+    Token* memtok = nullptr;
+    if (Token::Match(tok->astParent(), ". %name% (") && astIsRHS(tok))
+        memtok = tok->astParent()->astOperand1();
     int returnContainer = settings->library.returnValueContainer(tok);
     if (returnContainer >= 0) {
         std::vector<const Token *> args = getArguments(tok);
@@ -3740,19 +3743,20 @@ static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLog
             LifetimeStore{argtok, "Passed to '" + tok->str() + "'.", ValueFlow::Value::LifetimeKind::Object}.byVal(
                 tok->next(), tokenlist, errorLogger, settings);
         }
-    } else if (Token::Match(tok->tokAt(-2), "%var% . push_back|push_front|insert|push|assign") &&
+    } else if (memtok && Token::Match(tok->astParent(), ". push_back|push_front|insert|push|assign") &&
-               astIsContainer(tok->tokAt(-2))) {
+               astIsContainer(memtok)) {
-        Token *vartok = tok->tokAt(-2);
         std::vector<const Token *> args = getArguments(tok);
         std::size_t n = args.size();
         if (n > 1 && Token::typeStr(args[n - 2]) == Token::typeStr(args[n - 1]) &&
             (((astIsIterator(args[n - 2]) && astIsIterator(args[n - 1])) ||
               (astIsPointer(args[n - 2]) && astIsPointer(args[n - 1]))))) {
-            LifetimeStore{args.back(), "Added to container '" + vartok->str() + "'.", ValueFlow::Value::LifetimeKind::Object}.byDerefCopy(
+            LifetimeStore{
-                vartok, tokenlist, errorLogger, settings);
+                args.back(), "Added to container '" + memtok->str() + "'.", ValueFlow::Value::LifetimeKind::Object}
+            .byDerefCopy(memtok, tokenlist, errorLogger, settings);
         } else if (!args.empty() && isLifetimeBorrowed(args.back(), settings)) {
-            LifetimeStore{args.back(), "Added to container '" + vartok->str() + "'.", ValueFlow::Value::LifetimeKind::Object}.byVal(
+            LifetimeStore{
-                vartok, tokenlist, errorLogger, settings);
+                args.back(), "Added to container '" + memtok->str() + "'.", ValueFlow::Value::LifetimeKind::Object}
+            .byVal(memtok, tokenlist, errorLogger, settings);
         }
     } else if (tok->function()) {
         const Function *f = tok->function();
@@ -3776,6 +3780,17 @@ static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLog
                     continue;
                 if (!v.tokvalue)
                     continue;
+                if (exprDependsOnThis(v.tokvalue) && memtok) {
+                    LifetimeStore ls = LifetimeStore{memtok,
+                                                     "Passed to member function '" + tok->expressionString() + "'.",
+                                                     ValueFlow::Value::LifetimeKind::Object};
+                    ls.inconclusive = inconclusive;
+                    ls.forward = false;
+                    ls.errorPath = v.errorPath;
+                    ls.errorPath.emplace_front(returnTok, "Return " + lifetimeType(returnTok, &v) + ".");
+                    update |= ls.byRef(tok->next(), tokenlist, errorLogger, settings);
+                    continue;
+                }
                 const Variable *var = v.tokvalue->variable();
                 LifetimeStore ls = LifetimeStore::fromFunctionArg(f, tok, var, tokenlist, errorLogger);
                 if (!ls.argtok)

test/testautovariables.cpp

@@ -151,6 +151,7 @@ private:
         TEST_CASE(danglingLifetimeImplicitConversion);
         TEST_CASE(danglingTemporaryLifetime);
         TEST_CASE(danglingLifetimeBorrowedMembers);
+        TEST_CASE(danglingLifetimeClassMemberFunctions);
         TEST_CASE(invalidLifetime);
         TEST_CASE(deadPointer);
         TEST_CASE(splitNamespaceAuto); // crash #10473
@@ -2246,10 +2247,8 @@ private:
               "auto f() {\n"
               "    return g().begin();\n"
               "}");
-        TODO_ASSERT_EQUALS(
+        ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:3]: (error) Returning iterator that will be invalid when returning.\n",
-            "[test.cpp:3] -> [test.cpp:3]: (error) Returning iterator that will be invalid when returning.\n",
+                      errout.str());
-            "",
-            errout.str());
 
         check("std::vector<int> f();\n"
               "auto f() {\n"
@@ -3291,6 +3290,41 @@ private:
         ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:5] -> [test.cpp:6]: (error) Using pointer that is a temporary.\n",
                       errout.str());
     }
+
+    void danglingLifetimeClassMemberFunctions()
+    {
+        check("struct S {\n"
+              "    S(int i) : i(i) {}\n"
+              "    int i;\n"
+              "    int* ptr() { return &i; }\n"
+              "};\n"
+              "int* fun(int i) { \n"
+              "    return S(i).ptr();\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[test.cpp:4] -> [test.cpp:4] -> [test.cpp:7] -> [test.cpp:7]: (error) Returning pointer that will be invalid when returning.\n",
+            errout.str());
+
+        check("struct Fred\n"
+              "{\n"
+              "    int x[2];\n"
+              "    Fred() {\n"
+              "        x[0] = 0x41;\n"
+              "        x[1] = 0x42;\n"
+              "    }\n"
+              "    const int *get_x() {\n"
+              "        return x;\n"
+              "    }\n"
+              "};\n"
+              "static const int *foo() {\n"
+              "    Fred fred;\n"
+              "    return fred.get_x();\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[test.cpp:9] -> [test.cpp:9] -> [test.cpp:14] -> [test.cpp:13] -> [test.cpp:14]: (error) Returning pointer to local variable 'fred' that will be invalid when returning.\n",
+            errout.str());
+    }
+
     void invalidLifetime() {
         check("void foo(int a) {\n"
               "    std::function<void()> f;\n"

test/testnullpointer.cpp

@@ -2088,7 +2088,9 @@ private:
               "    if (!y) {}\n"
               "  }\n"
               "}\n");
-        ASSERT_EQUALS("", errout.str());
+        ASSERT_EQUALS(
+            "[test.cpp:13] -> [test.cpp:9]: (warning) Either the condition '!y' is redundant or there is possible null pointer dereference: x->g().\n",
+            errout.str());
     }
 
     void nullpointer65() {