From afb5590741996d13ee7e8f91483a87dc3c5b02c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Thu, 13 Feb 2020 10:59:00 +0100 Subject: [PATCH] New check: delete 'self pointer' that might be used as 'this' and then use some member/method --- lib/checkclass.cpp | 95 ++++++++++++++++++++++++++++++ lib/checkclass.h | 13 +++++ test/testclass.cpp | 140 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 248 insertions(+) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 9e647f381..b32eb0d80 100644 --- a/lib/checkclass.cpp +++ b/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 callstack; + checkThisUseAfterFreeRecursive(classScope, &func, &var, callstack, &freeToken); + } + } + } +} + +bool CheckClass::checkThisUseAfterFreeRecursive(const Scope *classScope, const Function *func, const Variable *selfPointer, std::set 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)) diff --git a/lib/checkclass.h b/lib/checkclass.h index b754550ee..305d7cabd 100644 --- a/lib/checkclass.h +++ b/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(), "f"); c.virtualFunctionCallInConstructorError(nullptr, std::list(), "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 callstack, const Token **freeToken); + }; /// @} //--------------------------------------------------------------------------- diff --git a/test/testclass.cpp b/test/testclass.cpp index 684088790..c7c955170 100644 --- a/test/testclass.cpp +++ b/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 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 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 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 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 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)