From abe696877ba789a05e2ebe64c88a6d0d2e0f7a87 Mon Sep 17 00:00:00 2001 From: chrchr Date: Mon, 12 Sep 2022 15:57:37 +0200 Subject: [PATCH 1/6] Fix #1655 wrong usage of std::string in memcpy --- lib/checkclass.cpp | 21 +++++++++++++-------- lib/checkclass.h | 2 +- test/testclass.cpp | 10 ++++++++++ 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index d081afb12c0..cce0e0fc25e 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -1353,6 +1353,10 @@ void CheckClass::checkMemset() if (numIndirToVariableType == 1) type = var->typeScope(); + + if (!type && mSettings->library.detectContainerOrIterator(var->typeStartToken())) { + memsetError(tok, tok->str(), var->getTypeName(), {}, /*isContainer*/ true); + } } } @@ -1468,15 +1472,16 @@ void CheckClass::mallocOnClassError(const Token* tok, const std::string &memfunc "since no constructor is called and class members remain uninitialized. Consider using 'new' instead.", CWE665, Certainty::normal); } -void CheckClass::memsetError(const Token *tok, const std::string &memfunc, const std::string &classname, const std::string &type) +void CheckClass::memsetError(const Token *tok, const std::string &memfunc, const std::string &classname, const std::string &type, bool isContainer) { - reportError(tok, Severity::error, "memsetClass", - "$symbol:" + memfunc +"\n" - "$symbol:" + classname +"\n" - "Using '" + memfunc + "' on " + type + " that contains a " + classname + ".\n" - "Using '" + memfunc + "' on " + type + " that contains a " + classname + " is unsafe, because constructor, destructor " - "and copy operator calls are omitted. These are necessary for this non-POD type to ensure that a valid object " - "is created.", CWE762, Certainty::normal); + const std::string typeStr = isContainer ? std::string() : (type + " that contains a "); + const std::string msg = "$symbol:" + memfunc + "\n" + "$symbol:" + classname + "\n" + "Using '" + memfunc + "' on " + typeStr + classname + ".\n" + "Using '" + memfunc + "' on " + typeStr + classname + " is unsafe, because constructor, destructor " + "and copy operator calls are omitted. These are necessary for this non-POD type to ensure that a valid object " + "is created."; + reportError(tok, Severity::error, "memsetClass", msg, CWE762, Certainty::normal); } void CheckClass::memsetErrorReference(const Token *tok, const std::string &memfunc, const std::string &type) diff --git a/lib/checkclass.h b/lib/checkclass.h index cc7bcf9cd7a..19e2ca07709 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -206,7 +206,7 @@ class CPPCHECKLIB CheckClass : public Check { void missingMemberCopyError(const Token *tok, Function::Type functionType, const std::string& classname, const std::string& varname); 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); - void memsetError(const Token *tok, const std::string &memfunc, const std::string &classname, const std::string &type); + void memsetError(const Token *tok, const std::string &memfunc, const std::string &classname, const std::string &type, bool isContainer = false); void memsetErrorReference(const Token *tok, const std::string &memfunc, const std::string &type); void memsetErrorFloat(const Token *tok, const std::string &type); void mallocOnClassError(const Token* tok, const std::string &memfunc, const Token* classTok, const std::string &classname); diff --git a/test/testclass.cpp b/test/testclass.cpp index 2aa74cedd11..683739cc131 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -3109,6 +3109,16 @@ class TestClass : public TestFixture { " memset(b, 0, sizeof(b));\n" "}"); ASSERT_EQUALS("", errout.str()); + + // #1655 + Settings s; + LOAD_LIB_2(s.library, "std.cfg"); + checkNoMemset("void f() {\n" + " char c[] = \"abc\";\n" + " std::string s;\n" + " memcpy(&s, c, strlen(c) + 1);\n" + "}\n", s); + ASSERT_EQUALS("[test.cpp:4]: (error) Using 'memcpy' on std::string.\n", errout.str()); } void memsetOnInvalid() { // Ticket #5425 From 12fe3de5a50cb7b89f726539fd434e6cf5e7e1bb Mon Sep 17 00:00:00 2001 From: chrchr Date: Thu, 15 Sep 2022 15:08:40 +0200 Subject: [PATCH 2/6] Fix memsetClass FP --- lib/checkclass.cpp | 2 +- test/testclass.cpp | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index cce0e0fc25e..a1a1330085e 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -1354,7 +1354,7 @@ void CheckClass::checkMemset() if (numIndirToVariableType == 1) type = var->typeScope(); - if (!type && mSettings->library.detectContainerOrIterator(var->typeStartToken())) { + if (!type && !var->isPointer() && mSettings->library.detectContainerOrIterator(var->typeStartToken())) { memsetError(tok, tok->str(), var->getTypeName(), {}, /*isContainer*/ true); } } diff --git a/test/testclass.cpp b/test/testclass.cpp index 683739cc131..ca950e27b03 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -3119,6 +3119,18 @@ class TestClass : public TestFixture { " memcpy(&s, c, strlen(c) + 1);\n" "}\n", s); ASSERT_EQUALS("[test.cpp:4]: (error) Using 'memcpy' on std::string.\n", errout.str()); + + checkNoMemset("template \n" + " void f(T* dst, const T* src, int N) {\n" + " std::memcpy(dst, src, N * sizeof(T));\n" + "}\n" + "void g() {\n" + " typedef std::vector* P;\n" + " P Src[2]{};\n" + " P Dst[2];\n" + " f

(Dst, Src, 2);\n" + "}\n", s); + ASSERT_EQUALS("", errout.str()); } void memsetOnInvalid() { // Ticket #5425 From 83f7c7344410c91542132da7dde31f34ac74a287 Mon Sep 17 00:00:00 2001 From: chrchr Date: Wed, 21 Sep 2022 13:47:22 +0200 Subject: [PATCH 3/6] Fix #8619 FN memset on container containing structs with containers --- lib/checkclass.cpp | 8 ++++---- test/testclass.cpp | 12 ++++++++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index a1a1330085e..e62dc52b265 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -1319,13 +1319,13 @@ void CheckClass::checkMemset() const Token *typeTok = nullptr; const Scope *type = nullptr; - if (Token::Match(arg3, "sizeof ( %type% ) )")) + if (Token::Match(arg3, "sizeof ( %type% ) )|*")) typeTok = arg3->tokAt(2); - else if (Token::Match(arg3, "sizeof ( %type% :: %type% ) )")) + else if (Token::Match(arg3, "sizeof ( %type% :: %type% ) )|*")) typeTok = arg3->tokAt(4); - else if (Token::Match(arg3, "sizeof ( struct %type% ) )")) + else if (Token::Match(arg3, "sizeof ( struct %type% ) )|*")) typeTok = arg3->tokAt(3); - else if (Token::simpleMatch(arg3, "sizeof ( * this ) )") || Token::simpleMatch(arg1, "this ,")) { + else if (Token::simpleMatch(arg3, "sizeof ( * this ) )|*") || Token::simpleMatch(arg1, "this ,")) { type = findFunctionOf(arg3->scope()); } else if (Token::Match(arg1, "&|*|%var%")) { int numIndirToVariableType = 0; // Offset to the actual type in terms of dereference/addressof diff --git a/test/testclass.cpp b/test/testclass.cpp index ca950e27b03..a2f6f1f5f67 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -3110,6 +3110,18 @@ class TestClass : public TestFixture { "}"); ASSERT_EQUALS("", errout.str()); + // #8619 + checkNoMemset("struct S { std::vector m; };\n" + "void f() {\n" + " std::vector v(5);\n" + " memset(&v[0], 0, sizeof(S) * v.size());\n" + " memset(&v[0], 0, v.size() * sizeof(S));\n" + "}\n"); + TODO_ASSERT_EQUALS("[test.cpp:4]: (error) Using 'memset' on struct that contains a 'std::vector'.\n" + "[test.cpp:5]: (error) Using 'memset' on struct that contains a 'std::vector'.\n", + "[test.cpp:4]: (error) Using 'memset' on struct that contains a 'std::vector'.\n", + errout.str()); + // #1655 Settings s; LOAD_LIB_2(s.library, "std.cfg"); From 5566aa34dce4572d8a519ac1cbf8d98fe96c900b Mon Sep 17 00:00:00 2001 From: chrchr Date: Wed, 21 Sep 2022 14:24:05 +0200 Subject: [PATCH 4/6] Token::Match --- lib/checkclass.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index e62dc52b265..21e312bf141 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -1325,7 +1325,7 @@ void CheckClass::checkMemset() typeTok = arg3->tokAt(4); else if (Token::Match(arg3, "sizeof ( struct %type% ) )|*")) typeTok = arg3->tokAt(3); - else if (Token::simpleMatch(arg3, "sizeof ( * this ) )|*") || Token::simpleMatch(arg1, "this ,")) { + else if (Token::Match(arg3, "sizeof ( * this ) )|*") || Token::simpleMatch(arg1, "this ,")) { type = findFunctionOf(arg3->scope()); } else if (Token::Match(arg1, "&|*|%var%")) { int numIndirToVariableType = 0; // Offset to the actual type in terms of dereference/addressof From 1cecc0cdf539debc648ba48e3852eb136f31e25c Mon Sep 17 00:00:00 2001 From: chrchr Date: Wed, 21 Sep 2022 15:02:50 +0200 Subject: [PATCH 5/6] Use AST --- lib/checkclass.cpp | 23 +++++++++++++++-------- test/testclass.cpp | 11 +++++++---- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 21e312bf141..40c7b112bb1 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -1319,14 +1319,21 @@ void CheckClass::checkMemset() const Token *typeTok = nullptr; const Scope *type = nullptr; - if (Token::Match(arg3, "sizeof ( %type% ) )|*")) - typeTok = arg3->tokAt(2); - else if (Token::Match(arg3, "sizeof ( %type% :: %type% ) )|*")) - typeTok = arg3->tokAt(4); - else if (Token::Match(arg3, "sizeof ( struct %type% ) )|*")) - typeTok = arg3->tokAt(3); - else if (Token::Match(arg3, "sizeof ( * this ) )|*") || Token::simpleMatch(arg1, "this ,")) { - type = findFunctionOf(arg3->scope()); + const Token* sizeofTok = arg3->previous()->astOperand2(); // try to find sizeof() in argument expression + if (sizeofTok && sizeofTok->astOperand1() && Token::simpleMatch(sizeofTok->astOperand1()->previous(), "sizeof (")) + sizeofTok = sizeofTok->astOperand1(); + else if (sizeofTok && sizeofTok->astOperand2() && Token::simpleMatch(sizeofTok->astOperand2()->previous(), "sizeof (")) + sizeofTok = sizeofTok->astOperand2(); + if (Token::simpleMatch(sizeofTok, "(")) + sizeofTok = sizeofTok->previous(); + if (Token::Match(sizeofTok, "sizeof ( %type% )")) + typeTok = sizeofTok->tokAt(2); + else if (Token::Match(sizeofTok, "sizeof ( %type% :: %type% )")) + typeTok = sizeofTok->tokAt(4); + else if (Token::Match(sizeofTok, "sizeof ( struct %type% )")) + typeTok = sizeofTok->tokAt(3); + else if (Token::Match(sizeofTok, "sizeof ( * this )") || Token::simpleMatch(arg1, "this ,")) { + type = findFunctionOf(sizeofTok->scope()); } else if (Token::Match(arg1, "&|*|%var%")) { int numIndirToVariableType = 0; // Offset to the actual type in terms of dereference/addressof for (;; arg1 = arg1->next()) { diff --git a/test/testclass.cpp b/test/testclass.cpp index 0ad4bdce2c6..0f857e2ac64 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -3115,11 +3115,14 @@ class TestClass : public TestFixture { " std::vector v(5);\n" " memset(&v[0], 0, sizeof(S) * v.size());\n" " memset(&v[0], 0, v.size() * sizeof(S));\n" + " memset(&v[0], 0, 5 * sizeof(S));\n" + " memset(&v[0], 0, sizeof(S) * 5);\n" "}\n"); - TODO_ASSERT_EQUALS("[test.cpp:4]: (error) Using 'memset' on struct that contains a 'std::vector'.\n" - "[test.cpp:5]: (error) Using 'memset' on struct that contains a 'std::vector'.\n", - "[test.cpp:4]: (error) Using 'memset' on struct that contains a 'std::vector'.\n", - errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Using 'memset' on struct that contains a 'std::vector'.\n" + "[test.cpp:5]: (error) Using 'memset' on struct that contains a 'std::vector'.\n" + "[test.cpp:6]: (error) Using 'memset' on struct that contains a 'std::vector'.\n" + "[test.cpp:7]: (error) Using 'memset' on struct that contains a 'std::vector'.\n", + errout.str()); // #1655 Settings s; From 657810d8a47faeb3b9d2e1ebabac08b71b462bea Mon Sep 17 00:00:00 2001 From: chrchr Date: Wed, 21 Sep 2022 15:41:49 +0200 Subject: [PATCH 6/6] simpleMatch --- lib/checkclass.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 40c7b112bb1..f96bf2a63c8 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -1332,7 +1332,7 @@ void CheckClass::checkMemset() typeTok = sizeofTok->tokAt(4); else if (Token::Match(sizeofTok, "sizeof ( struct %type% )")) typeTok = sizeofTok->tokAt(3); - else if (Token::Match(sizeofTok, "sizeof ( * this )") || Token::simpleMatch(arg1, "this ,")) { + else if (Token::simpleMatch(sizeofTok, "sizeof ( * this )") || Token::simpleMatch(arg1, "this ,")) { type = findFunctionOf(sizeofTok->scope()); } else if (Token::Match(arg1, "&|*|%var%")) { int numIndirToVariableType = 0; // Offset to the actual type in terms of dereference/addressof