From 60a213e6a56b6bce8c092edf0e6ad71c4a993603 Mon Sep 17 00:00:00 2001
From: Rikard Falkeborn <rikard.falkeborn@gmail.com>
Date: Wed, 3 Jul 2019 08:39:44 +0200
Subject: [PATCH] Fix #9047 (c-style casts before malloc) (#1930)

* Fix #9047 (c-style casts before malloc)

Note that there are still no warnings for c++-style casts

* Fix memleak check with casts of assignments in if-statements

* Fix possible null pointer dereference

As pointed out by cppcheck.

* Add check of astOperand2 when removing casts

This is similar to how it is done in other checks.
---
 cfg/gtk.cfg              |  8 ++------
 lib/checkleakautovar.cpp | 12 +++++++++---
 test/cfg/gnu.c           |  2 +-
 test/cfg/gtk.c           | 13 +++++++++++++
 test/cfg/std.c           |  2 +-
 test/cfg/windows.cpp     |  2 +-
 test/testleakautovar.cpp | 28 ++++++++++++++++++++++++++++
 7 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/cfg/gtk.cfg b/cfg/gtk.cfg
index ddd7ccb9b..53422ee0d 100644
--- a/cfg/gtk.cfg
+++ b/cfg/gtk.cfg
@@ -65,12 +65,8 @@
   <define name="G_TYPE_INSTANCE_GET_PRIVATE(instance, g_type, c_type)" value="((c_type*) g_type_instance_get_private ((GTypeInstance*) (instance), (g_type)))"/>
   <define name="G_OBJECT(obj)" value="(GObject*)(obj)"/>
   <define name="G_OBJECT_CLASS(class)" value="(G_TYPE_CHECK_CLASS_CAST ((class), G_TYPE_OBJECT, GObjectClass))"/>
-  <!-- TODO #9047: use this define instead of the one without the cast as soon as memleaks are detected also with the cast
-  <define name="_G_NEW(struct_type, n_structs, func)" value="((struct_type *) g_##func##_n ((n_structs), sizeof (struct_type)))"/> -->
-  <define name="_G_NEW(struct_type, n_structs, func)" value="(g_##func##_n ((n_structs), sizeof (struct_type)))"/>
-  <!-- TODO #9047: use this define instead of the one without the cast as soon as memleaks are detected also with the cast
-  <define name="_G_RENEW(struct_type, mem, n_structs, func)" value="((struct_type *) g_##func##_n (mem, (n_structs), sizeof (struct_type)))"/> -->
-  <define name="_G_RENEW(struct_type, mem, n_structs, func)" value="(g_##func##_n (mem, (n_structs), sizeof (struct_type)))"/>
+  <define name="_G_NEW(struct_type, n_structs, func)" value="((struct_type *) g_##func##_n ((n_structs), sizeof (struct_type)))"/>
+  <define name="_G_RENEW(struct_type, mem, n_structs, func)" value="((struct_type *) g_##func##_n (mem, (n_structs), sizeof (struct_type)))"/>
   <define name="g_new(struct_type, n_structs)" value="_G_NEW (struct_type, n_structs, malloc)"/>
   <define name="g_new0(struct_type, n_structs)" value="_G_NEW (struct_type, n_structs, malloc0)"/>
   <define name="g_renew(struct_type, mem, n_structs)" value="_G_RENEW (struct_type, mem, n_structs, realloc)"/>
diff --git a/lib/checkleakautovar.cpp b/lib/checkleakautovar.cpp
index 2984d5160..aecd93fe8 100644
--- a/lib/checkleakautovar.cpp
+++ b/lib/checkleakautovar.cpp
@@ -301,7 +301,9 @@ void CheckLeakAutoVar::checkScope(const Token * const startToken,
             }
 
             // right ast part (after `=` operator)
-            const Token* const tokRightAstOperand = tokAssignOp->astOperand2();
+            const Token* tokRightAstOperand = tokAssignOp->astOperand2();
+            while (tokRightAstOperand && tokRightAstOperand->isCast())
+                tokRightAstOperand = tokRightAstOperand->astOperand2() ? tokRightAstOperand->astOperand2() : tokRightAstOperand->astOperand1();
 
             // is variable used in rhs?
             if (isVarUsedInTree(tokRightAstOperand, varTok->varId()))
@@ -368,8 +370,12 @@ void CheckLeakAutoVar::checkScope(const Token * const startToken,
 
                 if (Token::Match(innerTok, "%var% =") && innerTok->astParent() == innerTok->next()) {
                     // allocation?
-                    if (Token::Match(innerTok->tokAt(2), "%type% (")) {
-                        const Library::AllocFunc* f = mSettings->library.alloc(innerTok->tokAt(2));
+                    // right ast part (after `=` operator)
+                    const Token* tokRightAstOperand = innerTok->next()->astOperand2();
+                    while (tokRightAstOperand && tokRightAstOperand->isCast())
+                        tokRightAstOperand = tokRightAstOperand->astOperand2() ? tokRightAstOperand->astOperand2() : tokRightAstOperand->astOperand1();
+                    if (tokRightAstOperand && Token::Match(tokRightAstOperand->previous(), "%type% (")) {
+                        const Library::AllocFunc* f = mSettings->library.alloc(tokRightAstOperand->previous());
                         if (f && f->arg == -1) {
                             VarInfo::AllocInfo& varAlloc = alloctype[innerTok->varId()];
                             varAlloc.type = f->groupId;
diff --git a/test/cfg/gnu.c b/test/cfg/gnu.c
index 677c6a0f2..a043f4144 100644
--- a/test/cfg/gnu.c
+++ b/test/cfg/gnu.c
@@ -79,7 +79,7 @@ void ignoreleak(void)
 {
     char *p = (char *)malloc(10);
     __builtin_memset(&(p[0]), 0, 10);
-    // TODO // cppcheck-suppress memleak
+    // cppcheck-suppress memleak
 }
 
 void memleak_asprintf(char **ptr, const char *fmt, const int arg)
diff --git a/test/cfg/gtk.c b/test/cfg/gtk.c
index 78cfc3716..7abb4991f 100644
--- a/test/cfg/gtk.c
+++ b/test/cfg/gtk.c
@@ -107,6 +107,19 @@ void g_new_test()
     // cppcheck-suppress memleak
 }
 
+void g_new_if_test()
+{
+    struct a {
+        int b;
+    };
+
+    struct a * pNew3;
+    if (pNew3 = g_new(struct a, 6)) {
+        printf("%p", pNew3);
+    }
+    // cppcheck-suppress memleak
+}
+
 void g_try_new0_test()
 {
     struct a {
diff --git a/test/cfg/std.c b/test/cfg/std.c
index 486ce51e4..869da96fd 100644
--- a/test/cfg/std.c
+++ b/test/cfg/std.c
@@ -117,7 +117,7 @@ void ignoreleak(void)
 {
     char *p = (char *)malloc(10);
     memset(&(p[0]), 0, 10);
-    // TODO cppcheck-suppress memleak
+    // cppcheck-suppress memleak
 }
 
 // null pointer
diff --git a/test/cfg/windows.cpp b/test/cfg/windows.cpp
index c7044cd5a..5ae5cc694 100644
--- a/test/cfg/windows.cpp
+++ b/test/cfg/windows.cpp
@@ -379,7 +379,7 @@ void memleak_LocalAlloc()
     (void)LocalFlags(pszBuf);
     LocalLock(pszBuf);
     LocalUnlock(pszBuf);
-    // TODO cppcheck-suppress memleak
+    // cppcheck-suppress memleak
 }
 
 void resourceLeak_CreateSemaphoreA()
diff --git a/test/testleakautovar.cpp b/test/testleakautovar.cpp
index e5efe782d..3eac62d88 100644
--- a/test/testleakautovar.cpp
+++ b/test/testleakautovar.cpp
@@ -60,6 +60,8 @@ private:
         TEST_CASE(assign14);
         TEST_CASE(assign15);
         TEST_CASE(assign16);
+        TEST_CASE(assign17); // #9047
+        TEST_CASE(assign18);
 
         TEST_CASE(deallocuse1);
         TEST_CASE(deallocuse2);
@@ -322,6 +324,32 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void assign17() { // #9047
+        check("void f() {\n"
+              "    char *p = (char*)malloc(10);\n"
+              "}");
+        ASSERT_EQUALS("[test.c:3]: (error) Memory leak: p\n", errout.str());
+
+        check("void f() {\n"
+              "    char *p = (char*)(int*)malloc(10);\n"
+              "}");
+        ASSERT_EQUALS("[test.c:3]: (error) Memory leak: p\n", errout.str());
+    }
+
+    void assign18() {
+        check("void f(int x) {\n"
+              "    char *p;\n"
+              "    if (x && (p = (char*)malloc(10))) { }"
+              "}");
+        ASSERT_EQUALS("[test.c:3]: (error) Memory leak: p\n", errout.str());
+
+        check("void f(int x) {\n"
+              "    char *p;\n"
+              "    if (x && (p = (char*)(int*)malloc(10))) { }"
+              "}");
+        ASSERT_EQUALS("[test.c:3]: (error) Memory leak: p\n", errout.str());
+    }
+
     void deallocuse1() {
         check("void f(char *p) {\n"
               "    free(p);\n"