From 73b41455dd3ff807320dc26ceb43aaea2aefaca3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Mon, 30 Apr 2018 23:13:08 +0200 Subject: [PATCH] CheckClass: If class has memory management it should have copy constructor, operator= and destructor --- lib/checkclass.cpp | 60 +++++++++++++++++++++++++++++++++++++--------- lib/checkclass.h | 8 +++++-- test/testclass.cpp | 53 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 107 insertions(+), 14 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 58b79f385..b8cfa26db 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -305,6 +305,27 @@ void CheckClass::copyconstructors() } } + if (!allocatedVars.empty()) { + bool hasCopyCtor = false; + bool hasOperatorEq = false; + bool hasDestructor = false; + for (const Function &func : scope->functionList) { + if (func.type == Function::eCopyConstructor) + hasCopyCtor = true; + else if (func.type == Function::eOperatorEqual) + hasOperatorEq = true; + else if (func.type == Function::eDestructor) + hasDestructor = true; + } + bool derived = !scope->definedType->derivedFrom.empty(); + if (!hasCopyCtor && !derived) + noCopyConstructorError(scope, derived); + if (!hasOperatorEq) + noOperatorEqError(scope); + if (!hasDestructor) + noDestructorError(scope); + } + std::set copiedVars; const Token* copyCtor = nullptr; for (const Function &func : scope->functionList) { @@ -337,14 +358,9 @@ void CheckClass::copyconstructors() } break; } - if (!copyCtor) { - if (!allocatedVars.empty() && scope->definedType->derivedFrom.empty()) // TODO: Check if base class is non-copyable - noCopyConstructorError(scope->classDef, scope->className, scope->type == Scope::eStruct); - } else { - if (!copiedVars.empty()) { - for (std::set::const_iterator it = copiedVars.begin(); it != copiedVars.end(); ++it) { - copyConstructorShallowCopyError(*it, (*it)->str()); - } + if (copyCtor && !copiedVars.empty()) { + for (std::set::const_iterator it = copiedVars.begin(); it != copiedVars.end(); ++it) { + copyConstructorShallowCopyError(*it, (*it)->str()); } // throw error if count mismatch /* FIXME: This doesn't work. See #4154 @@ -372,12 +388,34 @@ void CheckClass::copyConstructorShallowCopyError(const Token *tok, const std::st "$symbol:" + varname + "\nValue of pointer '$symbol', which points to allocated memory, is copied in copy constructor instead of allocating new memory.", CWE398, false); } -void CheckClass::noCopyConstructorError(const Token *tok, const std::string &classname, bool isStruct) +void CheckClass::noCopyConstructorError(const Scope *scope, bool inconclusive) { - // The constructor might be intentionally missing. Therefore this is not a "warning" + const Token *tok = scope ? scope->classDef : nullptr; + const std::string &classname = scope ? scope->className : "class"; + bool isStruct = scope ? (scope->type == Scope::eStruct) : false; reportError(tok, Severity::style, "noCopyConstructor", "$symbol:" + classname + "\n" + - std::string(isStruct ? "struct" : "class") + " '$symbol' does not have a copy constructor which is recommended since the class contains a pointer to allocated memory.", CWE398, false); + std::string(isStruct ? "struct" : "class") + " '$symbol' does not have a copy constructor which is recommended since the class contains a pointer to allocated memory.", CWE398, inconclusive); +} + +void CheckClass::noOperatorEqError(const Scope *scope) +{ + const Token *tok = scope ? scope->classDef : nullptr; + const std::string &classname = scope ? scope->className : "class"; + bool isStruct = scope ? (scope->type == Scope::eStruct) : false; + reportError(tok, Severity::style, "noOperatorEq", + "$symbol:" + classname + "\n" + + std::string(isStruct ? "struct" : "class") + " '$symbol' does not have a assignment operator which is recommended since the class contains a pointer to allocated memory.", CWE398, false); +} + +void CheckClass::noDestructorError(const Scope *scope) +{ + const Token *tok = scope ? scope->classDef : nullptr; + const std::string &classname = scope ? scope->className : "class"; + bool isStruct = scope ? (scope->type == Scope::eStruct) : false; + reportError(tok, Severity::style, "noDestructor", + "$symbol:" + classname + "\n" + + std::string(isStruct ? "struct" : "class") + " '$symbol' does not have a destructor which is recommended since the class contains a pointer to allocated memory.", CWE398, false); } bool CheckClass::canNotCopy(const Scope *scope) diff --git a/lib/checkclass.h b/lib/checkclass.h index c2b21bd05..a7977f0a9 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -168,7 +168,9 @@ private: void noExplicitConstructorError(const Token *tok, const std::string &classname, bool isStruct); //void copyConstructorMallocError(const Token *cctor, const Token *alloc, const std::string& var_name); void copyConstructorShallowCopyError(const Token *tok, const std::string& varname); - void noCopyConstructorError(const Token *tok, const std::string &classname, bool isStruct); + void noCopyConstructorError(const Scope *scope, bool inconclusive); + void noOperatorEqError(const Scope *scope); + void noDestructorError(const Scope *scope); void uninitVarError(const Token *tok, bool isprivate, const std::string &classname, const std::string &varname, bool inconclusive); void operatorEqVarError(const Token *tok, const std::string &classname, const std::string &varname, bool inconclusive); void unusedPrivateFunctionError(const Token *tok, const std::string &classname, const std::string &funcname); @@ -202,7 +204,9 @@ private: c.noExplicitConstructorError(nullptr, "classname", false); //c.copyConstructorMallocError(nullptr, 0, "var"); c.copyConstructorShallowCopyError(nullptr, "var"); - c.noCopyConstructorError(nullptr, "class", false); + c.noCopyConstructorError(nullptr, false); + c.noOperatorEqError(nullptr); + c.noDestructorError(nullptr); c.uninitVarError(nullptr, false, "classname", "varname", false); c.operatorEqVarError(nullptr, "classname", emptyString, false); c.unusedPrivateFunctionError(nullptr, "classname", "funcname"); diff --git a/test/testclass.cpp b/test/testclass.cpp index 4dee5a10c..db89d8bc1 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -68,6 +68,8 @@ private: TEST_CASE(copyConstructor1); TEST_CASE(copyConstructor2); // ticket #4458 + TEST_CASE(noOperatorEq); // class with memory management should have operator eq + TEST_CASE(noDestructor); // class with memory management should have destructor TEST_CASE(operatorEq1); TEST_CASE(operatorEq2); @@ -564,6 +566,8 @@ private: " p=(char *)malloc(strlen(str)+1);\n" " strcpy(p,str);\n" " }\n" + " F&operator=(const F&);\n" + " ~F();\n" "};"); ASSERT_EQUALS("[test.cpp:5]: (style) Value of pointer 'p', which points to allocated memory, is copied in copy constructor instead of allocating new memory.\n", errout.str()); @@ -575,6 +579,8 @@ private: " F(char *str) {\n" " p = malloc(strlen(str)+1);\n" " }\n" + " ~F();\n" + " F& operator=(const F&f);\n" "};"); TODO_ASSERT_EQUALS("[test.cpp:4]: (style) Value of pointer 'p', which points to allocated memory, is copied in copy constructor instead of allocating new memory.\n" "[test.cpp:3] -> [test.cpp:7]: (warning) Copy constructor does not allocate memory for member 'p' although memory has been allocated in other constructors.\n", @@ -593,6 +599,8 @@ private: " p=(char *)malloc(strlen(str)+1);\n" " strcpy(p,str);\n" " }\n" + " ~F();\n" + " F& operator=(const F&f);\n" "};"); TODO_ASSERT_EQUALS("[test.cpp:5]: (style) Value of pointer 'p', which points to allocated memory, is copied in copy constructor instead of allocating new memory.\n" "[test.cpp:5] -> [test.cpp:10]: (warning) Copy constructor does not allocate memory for member 'p' although memory has been allocated in other constructors.\n", @@ -621,6 +629,8 @@ private: " d=(char *)malloc(strlen(str)+1);\n" " strcpy(p,f.p);\n" " }\n" + " ~kalci();\n" + " kalci& operator=(const kalci&kalci);\n" "};"); ASSERT_EQUALS("", errout.str()); @@ -644,6 +654,8 @@ private: " c=(char *)malloc(strlen(str)+1);\n" " strcpy(p,f.p);\n" " }\n" + " ~F();\n" + " F& operator=(const F&f);\n" "};"); TODO_ASSERT_EQUALS("[test.cpp:14] -> [test.cpp:11]: (warning) Copy constructor does not allocate memory for member 'd' although memory has been allocated in other constructors.\n", "", errout.str()); @@ -656,6 +668,8 @@ private: " : p(malloc(size))\n" " {\n" " }\n" + " ~F();\n" + " F& operator=(const F&f);\n" "};"); ASSERT_EQUALS("", errout.str()); @@ -668,6 +682,8 @@ private: " F(const F &f)\n" " {\n" " }\n" + " ~F();\n" + " F& operator=(const F&f);\n" "};"); TODO_ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:4]: (warning) Copy constructor does not allocate memory for member 'd' although memory has been allocated in other constructors.\n", "", errout.str()); @@ -681,6 +697,8 @@ private: " c=(char *)malloc(100);\n" " d=(char*)malloc(100);\n" " }\n" + " ~F();\n" + " F& operator=(const F&f);\n" "};"); ASSERT_EQUALS("[test.cpp:1]: (style) class 'F' does not have a copy constructor which is recommended since the class contains a pointer to allocated memory.\n", errout.str()); @@ -703,6 +721,8 @@ private: " c=(char *)malloc(strlen(str)+1);\n" " strcpy(d,f.p);\n" " }\n" + " ~F();\n" + " F& operator=(const F&f);\n" "};"); ASSERT_EQUALS("", errout.str()); @@ -712,6 +732,8 @@ private: " F() {\n" " p = malloc(100);\n" " }\n" + " ~F();\n" + " F& operator=(const F&f);\n" "};"); ASSERT_EQUALS("", errout.str()); @@ -722,6 +744,8 @@ private: " F() {\n" " p = malloc(100);\n" " }\n" + " ~F();\n" + " F& operator=(const F&f);\n" "};"); ASSERT_EQUALS("", errout.str()); @@ -731,6 +755,8 @@ private: " F() {\n" " p = malloc(100);\n" " }\n" + " ~F();\n" + " F& operator=(const F&f);\n" "};"); TODO_ASSERT_EQUALS("[test.cpp:2]: (style) 'class F' does not have a copy constructor which is recommended since the class contains a pointer to allocated memory.\n", "", errout.str()); @@ -739,13 +765,17 @@ private: " F() {\n" " p = malloc(100);\n" " }\n" - " F(F& f);\n" // non-copyable + " F(F& f);\n" + " ~F();\n" + " F& operator=(const F&f);\n" "};"); ASSERT_EQUALS("", errout.str()); checkCopyConstructor("class F {\n" " char *p;\n" " F() : p(malloc(100)) {}\n" + " ~F();\n" + " F& operator=(const F&f);\n" "};"); ASSERT_EQUALS("[test.cpp:1]: (style) class 'F' does not have a copy constructor which is recommended since the class contains a pointer to allocated memory.\n", errout.str()); @@ -770,11 +800,32 @@ private: " }\n" " Vector( const Vector<_Tp>& v ) {\n" " }\n" + " ~Vector();\n" + " Vector& operator=(const Vector&v);\n" " _Tp* _M_finish;\n" "};"); ASSERT_EQUALS("", errout.str()); } + void noOperatorEq() { + checkCopyConstructor("struct F {\n" + " char* c;\n" + " F() { c = malloc(100); }\n" + " F(const F &f);\n" + " ~F();\n" + "};"); + ASSERT_EQUALS("[test.cpp:1]: (style) struct 'F' does not have a assignment operator which is recommended since the class contains a pointer to allocated memory.\n", errout.str()); + } + + void noDestructor() { + checkCopyConstructor("struct F {\n" + " char* c;\n" + " F() { c = malloc(100); }\n" + " F(const F &f);\n" + " F&operator=(const F&);" + "};"); + ASSERT_EQUALS("[test.cpp:1]: (style) struct 'F' does not have a destructor which is recommended since the class contains a pointer to allocated memory.\n", errout.str()); + } // Check the operator Equal void checkOpertorEq(const char code[]) {