New check: delete 'self pointer' that might be used as 'this' and then use some member/method

lib/checkclass.cpp

@@ -2625,6 +2625,101 @@ void CheckClass::overrideError(const Function *funcInBase, const Function *funcI
                 false);
 }
 
+void CheckClass::checkThisUseAfterFree()
+{
+    if (!mSettings->isEnabled(Settings::WARNING))
+        return;
+
+    for (const Scope * classScope : mSymbolDatabase->classAndStructScopes) {
+
+        for (const Variable &var : classScope->varlist) {
+            // Find possible "self pointer".. pointer/smartpointer member variable of "self" type.
+            if (var.valueType() && var.valueType()->smartPointerType != classScope->definedType && var.valueType()->typeScope != classScope) {
+                const ValueType valueType = ValueType::parseDecl(var.typeStartToken(), mSettings);
+                if (valueType.smartPointerType != classScope->definedType)
+                    continue;
+            }
+
+            // If variable is not static, check that "this" is assigned
+            if (!var.isStatic()) {
+                bool hasAssign = false;
+                for (const Function &func : classScope->functionList) {
+                    if (func.type != Function::Type::eFunction || !func.hasBody())
+                        continue;
+                    for (const Token *tok = func.functionScope->bodyStart; tok != func.functionScope->bodyEnd; tok = tok->next()) {
+                        if (Token::Match(tok, "%varid% = this|shared_from_this", var.declarationId())) {
+                            hasAssign = true;
+                            break;
+                        }
+                    }
+                    if (hasAssign)
+                        break;
+                }
+                if (!hasAssign)
+                    continue;
+            }
+
+            // Check usage of self pointer..
+            for (const Function &func : classScope->functionList) {
+                if (func.type != Function::Type::eFunction || !func.hasBody())
+                    continue;
+
+                const Token * freeToken = nullptr;
+                std::set<const Function *> callstack;
+                checkThisUseAfterFreeRecursive(classScope, &func, &var, callstack, &freeToken);
+            }
+        }
+    }
+}
+
+bool CheckClass::checkThisUseAfterFreeRecursive(const Scope *classScope, const Function *func, const Variable *selfPointer, std::set<const Function *> callstack, const Token **freeToken)
+{
+    if (!func || !func->functionScope)
+        return false;
+
+    // avoid recursion
+    if (callstack.count(func))
+        return false;
+    callstack.insert(func);
+
+    const Token * const bodyStart = func->functionScope->bodyStart;
+    const Token * const bodyEnd = func->functionScope->bodyEnd;
+    for (const Token *tok = bodyStart; tok != bodyEnd; tok = tok->next()) {
+        const bool isDestroyed = *freeToken != nullptr && !func->isStatic();
+        if (Token::Match(tok, "delete %var% ;") && selfPointer == tok->next()->variable()) {
+            *freeToken = tok;
+            tok = tok->tokAt(2);
+        } else if (Token::Match(tok, "%var% . reset ( )") && selfPointer == tok->variable())
+            *freeToken = tok;
+        else if (Token::Match(tok->previous(), "!!. %name% (") && tok->function() && tok->function()->nestedIn == classScope) {
+            if (isDestroyed) {
+                thisUseAfterFree(selfPointer->nameToken(), *freeToken, tok);
+                return true;
+            }
+            if (checkThisUseAfterFreeRecursive(classScope, tok->function(), selfPointer, callstack, freeToken))
+                return true;
+        } else if (isDestroyed && Token::Match(tok->previous(), "!!. %name%") && tok->variable() && tok->variable()->scope() == classScope && !tok->variable()->isStatic() && !tok->variable()->isArgument()) {
+            thisUseAfterFree(selfPointer->nameToken(), *freeToken, tok);
+            return true;
+        } else if (*freeToken && Token::Match(tok, "return|throw"))
+            // TODO
+            return tok->str() == "throw";
+    }
+    return false;
+}
+
+void CheckClass::thisUseAfterFree(const Token *self, const Token *free, const Token *use)
+{
+    std::string selfPointer = self ? self->str() : "ptr";
+    const ErrorPath errorPath = { ErrorPathItem(self, "Assuming '" + selfPointer + "' is used as 'this'"), ErrorPathItem(free, "Delete '" + selfPointer + "', invalidating 'this'"), ErrorPathItem(use, "Call method when 'this' is invalid") };
+    const std::string usestr = use ? use->str() : "x";
+    const std::string usemsg = use && use->function() ? ("Calling method '" + usestr + "()'") : ("Using member '" + usestr + "'");
+    reportError(errorPath, Severity::warning, "thisUseAfterFree",
+                "$symbol:" + selfPointer + "\n" +
+                usemsg + " when 'this' might be invalid",
+                CWE(0), false);
+}
+
 void CheckClass::checkUnsafeClassRefMember()
 {
     if (!mSettings->safeChecks.classes || !mSettings->isEnabled(Settings::WARNING))

lib/checkclass.h

@@ -79,6 +79,7 @@ public:
         checkClass.checkExplicitConstructors();
         checkClass.checkCopyCtorAndEqOperator();
         checkClass.checkOverride();
+        checkClass.checkThisUseAfterFree();
         checkClass.checkUnsafeClassRefMember();
     }
 
@@ -144,6 +145,9 @@ public:
     /** @brief Check that the override keyword is used when overriding virtual functions */
     void checkOverride();
 
+    /** @brief When "self pointer" is destroyed, 'this' might become invalid. */
+    void checkThisUseAfterFree();
+
     /** @brief Unsafe class check - const reference member */
     void checkUnsafeClassRefMember();
 
@@ -183,6 +187,7 @@ private:
     void duplInheritedMembersError(const Token* tok1, const Token* tok2, const std::string &derivedName, const std::string &baseName, const std::string &variableName, bool derivedIsStruct, bool baseIsStruct);
     void copyCtorAndEqOperatorError(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor);
     void overrideError(const Function *funcInBase, const Function *funcInDerived);
+    void thisUseAfterFree(const Token *self, const Token *free, const Token *use);
     void unsafeClassRefMemberError(const Token *tok, const std::string &varname);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const OVERRIDE {
@@ -220,6 +225,7 @@ private:
         c.pureVirtualFunctionCallInConstructorError(nullptr, std::list<const Token *>(), "f");
         c.virtualFunctionCallInConstructorError(nullptr, std::list<const Token *>(), "f");
         c.overrideError(nullptr, nullptr);
+        c.thisUseAfterFree(nullptr, nullptr, nullptr);
         c.unsafeClassRefMemberError(nullptr, "UnsafeClass::var");
     }
 
@@ -249,6 +255,7 @@ private:
                "- Duplicated inherited data members\n"
                // disabled for now "- If 'copy constructor' defined, 'operator=' also should be defined and vice versa\n"
                "- Check that arbitrary usage of public interface does not result in division by zero\n"
+               "- Delete \"self pointer\" and then access 'this'\n"
                "- Check that the 'override' keyword is used when overriding virtual functions\n";
     }
 
@@ -341,6 +348,12 @@ private:
     static bool canNotCopy(const Scope *scope);
 
     static bool canNotMove(const Scope *scope);
+
+    /**
+     * @brief Helper for checkThisUseAfterFree
+     */
+    bool checkThisUseAfterFreeRecursive(const Scope *classScope, const Function *func, const Variable *selfPointer, std::set<const Function *> callstack, const Token **freeToken);
+
 };
 /// @}
 //---------------------------------------------------------------------------

test/testclass.cpp

@@ -220,6 +220,8 @@ private:
         TEST_CASE(override1);
         TEST_CASE(overrideCVRefQualifiers);
 
+        TEST_CASE(checkThisUseAfterFree);
+
         TEST_CASE(unsafeClassRefMember);
     }
 
@@ -7167,6 +7169,144 @@ private:
         checkUnsafeClassRefMember("class C { C(const std::string &s) : s(s) {} const std::string &s; };");
         ASSERT_EQUALS("[test.cpp:1]: (warning) Unsafe class: The const reference member 'C::s' is initialized by a const reference constructor argument. You need to be careful about lifetime issues.\n", errout.str());
     }
+
+    void checkThisUseAfterFree(const char code[]) {
+        // Clear the error log
+        errout.str("");
+
+        // Tokenize..
+        Tokenizer tokenizer(&settings1, this);
+        std::istringstream istr(code);
+        tokenizer.tokenize(istr, "test.cpp");
+
+        // Check..
+        CheckClass checkClass(&tokenizer, &settings1, this);
+        checkClass.checkThisUseAfterFree();
+    }
+
+    void checkThisUseAfterFree() {
+        setMultiline();
+
+        // Calling method..
+        checkThisUseAfterFree("class C {\n"
+                              "public:\n"
+                              "  void dostuff() { delete mInstance; hello(); }\n"
+                              "private:\n"
+                              "  static C *mInstance;\n"
+                              "  void hello() {}\n"
+                              "};");
+        ASSERT_EQUALS("test.cpp:3:warning:Calling method 'hello()' when 'this' might be invalid\n"
+                      "test.cpp:5:note:Assuming 'mInstance' is used as 'this'\n"
+                      "test.cpp:3:note:Delete 'mInstance', invalidating 'this'\n"
+                      "test.cpp:3:note:Call method when 'this' is invalid\n",
+                      errout.str());
+
+        checkThisUseAfterFree("class C {\n"
+                              "public:\n"
+                              "  void dostuff() { mInstance.reset(); hello(); }\n"
+                              "private:\n"
+                              "  static std::shared_ptr<C> mInstance;\n"
+                              "  void hello() {}\n"
+                              "};");
+        ASSERT_EQUALS("test.cpp:3:warning:Calling method 'hello()' when 'this' might be invalid\n"
+                      "test.cpp:5:note:Assuming 'mInstance' is used as 'this'\n"
+                      "test.cpp:3:note:Delete 'mInstance', invalidating 'this'\n"
+                      "test.cpp:3:note:Call method when 'this' is invalid\n",
+                      errout.str());
+
+        checkThisUseAfterFree("class C {\n"
+                              "public:\n"
+                              "  void dostuff() { reset(); hello(); }\n"
+                              "private:\n"
+                              "  static std::shared_ptr<C> mInstance;\n"
+                              "  void hello();\n"
+                              "  void reset() { mInstance.reset(); }\n"
+                              "};");
+        ASSERT_EQUALS("test.cpp:3:warning:Calling method 'hello()' when 'this' might be invalid\n"
+                      "test.cpp:5:note:Assuming 'mInstance' is used as 'this'\n"
+                      "test.cpp:7:note:Delete 'mInstance', invalidating 'this'\n"
+                      "test.cpp:3:note:Call method when 'this' is invalid\n",
+                      errout.str());
+
+        // Use member..
+        checkThisUseAfterFree("class C {\n"
+                              "public:\n"
+                              "  void dostuff() { delete self; x = 123; }\n"
+                              "private:\n"
+                              "  static C *self;\n"
+                              "  int x;\n"
+                              "};");
+        ASSERT_EQUALS("test.cpp:3:warning:Using member 'x' when 'this' might be invalid\n"
+                      "test.cpp:5:note:Assuming 'self' is used as 'this'\n"
+                      "test.cpp:3:note:Delete 'self', invalidating 'this'\n"
+                      "test.cpp:3:note:Call method when 'this' is invalid\n",
+                      errout.str());
+
+        checkThisUseAfterFree("class C {\n"
+                              "public:\n"
+                              "  void dostuff() { delete self; x[1] = 123; }\n"
+                              "private:\n"
+                              "  static C *self;\n"
+                              "  std::map<int,int> x;\n"
+                              "};");
+        ASSERT_EQUALS("test.cpp:3:warning:Using member 'x' when 'this' might be invalid\n"
+                      "test.cpp:5:note:Assuming 'self' is used as 'this'\n"
+                      "test.cpp:3:note:Delete 'self', invalidating 'this'\n"
+                      "test.cpp:3:note:Call method when 'this' is invalid\n",
+                      errout.str());
+
+        // Assign 'shared_from_this()' to non-static smart pointer
+        checkThisUseAfterFree("class C {\n"
+                              "public:\n"
+                              "  void hold() { mInstance = shared_from_this(); }\n"
+                              "  void dostuff() { mInstance.reset(); hello(); }\n"
+                              "private:\n"
+                              "  std::shared_ptr<C> mInstance;\n"
+                              "  void hello() {}\n"
+                              "};");
+        ASSERT_EQUALS("test.cpp:4:warning:Calling method 'hello()' when 'this' might be invalid\n"
+                      "test.cpp:6:note:Assuming 'mInstance' is used as 'this'\n"
+                      "test.cpp:4:note:Delete 'mInstance', invalidating 'this'\n"
+                      "test.cpp:4:note:Call method when 'this' is invalid\n",
+                      errout.str());
+
+        // Avoid FP..
+        checkThisUseAfterFree("class C {\n"
+                              "public:\n"
+                              "  void dostuff() { delete self; x = 123; }\n"
+                              "private:\n"
+                              "  C *self;\n"
+                              "  int x;\n"
+                              "};");
+        ASSERT_EQUALS("", errout.str());
+
+        checkThisUseAfterFree("class C {\n"
+                              "public:\n"
+                              "  void hold() { mInstance = shared_from_this(); }\n"
+                              "  void dostuff() { if (x) { mInstance.reset(); return; } hello(); }\n"
+                              "private:\n"
+                              "  std::shared_ptr<C> mInstance;\n"
+                              "  void hello() {}\n"
+                              "};");
+        ASSERT_EQUALS("", errout.str());
+
+        checkThisUseAfterFree("class C\n"
+                              "{\n"
+                              "public:\n"
+                              "    explicit C(const QString& path) : mPath( path ) {}\n"
+                              "\n"
+                              "    static void initialize(const QString& path) {\n" // <- avoid fp in static method
+                              "        if (instanceSingleton)\n"
+                              "            delete instanceSingleton;\n"
+                              "        instanceSingleton = new C(path);\n"
+                              "    }\n"
+                              "private:\n"
+                              "    static C* instanceSingleton;\n"
+                              "};\n"
+                              "\n"
+                              "C* C::instanceSingleton;");
+        ASSERT_EQUALS("", errout.str());
+    }
 };
 
 REGISTER_TEST(TestClass)