CheckClass: If class has memory management it should have copy constructor, operator= and destructor

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<const Token*> copiedVars;
         const Token* copyCtor = nullptr;
         for (const Function &func : scope->functionList) {
@@ -337,15 +358,10 @@ void CheckClass::copyconstructors()
             }
             break;
         }
-        if (!copyCtor) {
+        if (copyCtor && !copiedVars.empty()) {
-            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 Token*>::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
             for (std::map<unsigned int, const Token*>::const_iterator i = allocatedVars.begin(); i != allocatedVars.end(); ++i) {
@@ -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)

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");

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[]) {