Fix #14954: False negatives: noCopyConstructor, noOperatorEq and unsafeClassCanLeak for file descriptor member (#8769)

Co-authored-by: Aaron Danen <aaron.danen@hpe.com>

Author: ludviggunne

lib/checkclass.cpp

@@ -473,15 +473,15 @@ void CheckClassImpl::copyconstructors()
                     if (Token::Match(tok, "%var% ( new") ||
                         (Token::Match(tok, "%var% ( %name% (") && mSettings.library.getAllocFuncInfo(tok->tokAt(2)))) {
                         const Variable* var = tok->variable();
-                        if (var && var->isPointer() && var->scope() == scope)
+                        if (var && var->scope() == scope && var->valueType() && var->valueType()->type != ValueType::SMART_POINTER)
                             allocatedVars[tok->varId()] = tok;
                     }
                 }
                 for (const Token* const end = func.functionScope->bodyEnd; tok != end; tok = tok->next()) {
                     if (Token::Match(tok, "%var% = new") ||
                         (Token::Match(tok, "%var% = %name% (") && mSettings.library.getAllocFuncInfo(tok->tokAt(2)))) {
                         const Variable* var = tok->variable();
-                        if (var && var->isPointer() && var->scope() == scope && !var->isStatic())
+                        if (var && var->scope() == scope && !var->isStatic() && var->valueType() && var->valueType()->type != ValueType::SMART_POINTER)
                             allocatedVars[tok->varId()] = tok;
                     }
                 }
@@ -493,7 +493,10 @@ void CheckClassImpl::copyconstructors()
                         (Token::Match(tok, "%name% ( %var%") && mSettings.library.getDeallocFuncInfo(tok))) {
                         const Token *vartok = tok->str() == "delete" ? tok->next() : tok->tokAt(2);
                         const Variable* var = vartok->variable();
-                        if (var && var->isPointer() && var->scope() == scope && !var->isStatic())
+                        if (var && var->scope() == scope && !var->isStatic() &&
+                            var->valueType() && ((var->valueType()->type != ValueType::CONTAINER &&
+                                                  var->valueType()->type != ValueType::RECORD &&
+                                                  var->valueType()->type != ValueType::UNKNOWN_TYPE) || var->valueType()->pointer))
                             deallocatedVars[vartok->varId()] = vartok;
                     }
                 }

lib/checkmemoryleak.cpp

@@ -517,7 +517,7 @@ void CheckMemoryLeakInClassImpl::check()
     // only check classes and structures
     for (const Scope * scope : symbolDatabase->classAndStructScopes) {
         for (const Variable &var : scope->varlist) {
-            if (!var.isStatic() && (var.isPointer() || var.isPointerArray())) {
+            if (!var.isStatic()) {
                 // allocation but no deallocation of private variables in public function..
                 const Token *tok = var.typeStartToken();
                 // Either it is of standard type or a non-derived type

test/testclass.cpp

@@ -38,7 +38,7 @@ class TestClass : public TestFixture {
     const Settings settings0_i = settingsBuilder(settings0).certainty(Certainty::inconclusive).build();
     const Settings settings1 = settingsBuilder().severity(Severity::warning).library("std.cfg").build();
     const Settings settings2 = settingsBuilder().severity(Severity::style).library("std.cfg").certainty(Certainty::inconclusive).build();
-    const Settings settings3 = settingsBuilder().severity(Severity::style).library("std.cfg").severity(Severity::warning).build();
+    const Settings settings3 = settingsBuilder().severity(Severity::style).library("std.cfg").severity(Severity::warning).library("posix.cfg").build();
     const Settings settings3_i = settingsBuilder(settings3).certainty(Certainty::inconclusive).build();
     const Settings settings4 = settingsBuilder().severity(Severity::warning).severity(Severity::portability).library("std.cfg").library("posix.cfg").build();
 
@@ -62,6 +62,8 @@ class TestClass : public TestFixture {
         TEST_CASE(copyConstructor4); // base class with private constructor
         TEST_CASE(copyConstructor5); // multiple inheritance
         TEST_CASE(copyConstructor6); // array of pointers
+        TEST_CASE(copyConstructor7); // ticket #14954
+        TEST_CASE(copyConstructor8);
         TEST_CASE(deletedMemberPointer); // deleted member pointer in destructor
         TEST_CASE(noOperatorEq); // class with memory management should have operator eq
         TEST_CASE(noDestructor); // class with memory management should have destructor
@@ -1094,6 +1096,25 @@ class TestClass : public TestFixture {
                            errout_str());
     }
 
+    void copyConstructor7() { // ticket #14954
+        checkCopyConstructor("struct S {\n"
+                             "    explicit S(char *name) { m_fd = mkstemp(name); }\n"
+                             "    ~S() { /* close(m_fd); */ }\n"
+                             "    S &operator =(const S&);\n"
+                             "    int m_fd;\n"
+                             "};\n");
+        ASSERT_EQUALS("[test.cpp:2:30]: (warning) Struct 'S' does not have a copy constructor which is recommended since it has dynamic memory/resource management. [noCopyConstructor]\n", errout_str());
+    }
+
+    void copyConstructor8() {
+        checkCopyConstructor("struct S {\n"
+                             "    S() : m_ptr(new int) {}\n"
+                             "    ~S();\n"
+                             "    std::unique_ptr<int> m_ptr;\n"
+                             "};\n");
+        ASSERT_EQUALS("", errout_str());
+    }
+
     void deletedMemberPointer() {
 
         // delete ...
@@ -1158,6 +1179,15 @@ class TestClass : public TestFixture {
                              "   ~F();\n"
                              "};");
         ASSERT_EQUALS("", errout_str());
+
+        checkCopyConstructor("struct S {\n"
+                             "    explicit S(char *name) { m_fd = mkstemp(name); }\n"
+                             "    S(const S&);\n"
+                             "    ~S() { /* close(m_fd); */ }\n"
+                             "    int m_fd;\n"
+                             "};\n");
+        ASSERT_EQUALS("[test.cpp:2:30]: (warning) Struct 'S' does not have a operator= which is recommended since it has dynamic memory/resource management. [noOperatorEq]\n", errout_str());
+
     }
 
     void noDestructor() {

test/testmemleak.cpp

@@ -488,7 +488,7 @@ class TestMemleakInClass : public TestFixture {
     TestMemleakInClass() : TestFixture("TestMemleakInClass") {}
 
 private:
-    const Settings settings = settingsBuilder().severity(Severity::warning).severity(Severity::style).library("std.cfg").build();
+    const Settings settings = settingsBuilder().severity(Severity::warning).severity(Severity::style).library("std.cfg").library("posix.cfg").build();
 
     /**
      * Tokenize and execute leak check for given code
@@ -533,6 +533,7 @@ class TestMemleakInClass : public TestFixture {
         TEST_CASE(class25); // ticket #4367 - false positive implementation for destructor is not seen
         TEST_CASE(class26); // ticket #10789
         TEST_CASE(class27); // ticket #8126
+        TEST_CASE(class28); // ticket #14954
 
         TEST_CASE(staticvar);
 
@@ -1484,6 +1485,15 @@ class TestMemleakInClass : public TestFixture {
         ASSERT_EQUALS("[test.cpp:6:11]: (style) Class 'S' is unsafe, 'S::a' can leak by wrong usage. [unsafeClassCanLeak]\n", errout_str());
     }
 
+    void class28() { // ticket #14954
+        check("struct S {\n"
+              "    explicit S(char *name) { m_fd = mkstemp(name); }\n"
+              "    ~S() { /* close(m_fd); */ }\n"
+              "    int m_fd;\n"
+              "};\n");
+        ASSERT_EQUALS("[test.cpp:4:9]: (style) Class 'S' is unsafe, 'S::m_fd' can leak by wrong usage. [unsafeClassCanLeak]\n", errout_str());
+    }
+
     void staticvar() {
         check("class A\n"
               "{\n"