From ba36c9426bb3e6cf11df7844c0339bc09c137960 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Tue, 21 Jul 2009 17:00:11 +0200 Subject: [PATCH] null pointers: better checking when dereferencing pointer and then checking if it is null (#485) --- src/checkother.cpp | 27 +++++---------------------- src/checkother.h | 7 ------- test/testother.cpp | 44 ++++++++++---------------------------------- 3 files changed, 15 insertions(+), 63 deletions(-) diff --git a/src/checkother.cpp b/src/checkother.cpp index e78069dfb..8c4a9f2ae 100644 --- a/src/checkother.cpp +++ b/src/checkother.cpp @@ -1028,11 +1028,7 @@ void CheckOther::nullPointer() tok2 = tok2->next(); } } -} - -void CheckOther::invalidPointer() -{ // Dereferencing a pointer and then checking if it's NULL.. for (const Token *tok1 = _tokenizer->tokens(); tok1; tok1 = tok1->next()) { @@ -1041,16 +1037,11 @@ void CheckOther::invalidPointer() if (std::string(tok1->strAt(1)) == tok1->strAt(3)) continue; - const unsigned int varid1(tok1->next()->varId()); - tok1 = tok1->tokAt(3); - const unsigned int varid2(tok1->varId()); - if (varid1 == 0 || varid2 == 0) + const unsigned int varid1(tok1->varId()); + if (varid1 == 0) continue; - // token where the variable is dereferenced.. - const Token * dereferenced = 0; - unsigned int indentlevel2 = 0; for (const Token *tok2 = tok1->tokAt(3); tok2; tok2 = tok2->next()) { @@ -1064,9 +1055,6 @@ void CheckOther::invalidPointer() --indentlevel2; } - else if (Token::Match(tok2, "* %varid%", varid1)) - dereferenced = tok2; - // Reassignment of the struct else if (tok2->varId() == varid1) { @@ -1080,10 +1068,10 @@ void CheckOther::invalidPointer() else if (indentlevel2 == 0 && tok2->str() == "return") break; - else if (dereferenced && tok2->str() == "if") + else if (Token::Match(tok2, "if ( !| %varid% )", varid1)) { - if (Token::Match(tok2, "if ( !| %varid% )", varid2)) - invalidPointerError(dereferenced); + nullPointerError(tok1); + break; } } } @@ -1189,11 +1177,6 @@ void CheckOther::nullPointerError(const Token *tok) reportError(tok, Severity::error, "nullPointer", "Possible null pointer dereference"); } -void CheckOther::invalidPointerError(const Token *tok) -{ - reportError(tok, Severity::error, "invalidPointer", "Possible invalid pointer dereference"); -} - void CheckOther::zerodivError(const Token *tok) { reportError(tok, Severity::error, "zerodiv", "Division by zero"); diff --git a/src/checkother.h b/src/checkother.h index 0fce6db0d..a684d3158 100644 --- a/src/checkother.h +++ b/src/checkother.h @@ -72,7 +72,6 @@ public: checkOther.strPlusChar(); checkOther.invalidFunctionUsage(); checkOther.nullPointer(); - checkOther.invalidPointer(); checkOther.checkZeroDivision(); } @@ -112,9 +111,6 @@ public: /** possible null pointer dereference */ void nullPointer(); - /** possible invalid pointer dereference */ - void invalidPointer(); - /** Check zero division*/ void checkZeroDivision(); @@ -145,7 +141,6 @@ private: void conditionAlwaysTrueFalse(const Token *tok, const std::string &truefalse); void strPlusChar(const Token *tok); void nullPointerError(const Token *tok); - void invalidPointerError(const Token *tok); void zerodivError(const Token *tok); void getErrorMessages() @@ -168,7 +163,6 @@ private: conditionAlwaysTrueFalse(0, "true/false"); strPlusChar(0); nullPointerError(0); - invalidPointerError(0); zerodivError(0); } @@ -192,7 +186,6 @@ private: " * condition that is always true/false\n" " * unusal pointer arithmetic. For example: \"abc\" + 'd'\n" " * dereferencing a null pointer\n" - " * dereferencing an invalid pointer\n" " * [[IncompleteStatement|Incomplete statement]]\n"; } }; diff --git a/test/testother.cpp b/test/testother.cpp index bf9adcf97..715a36ac8 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -58,8 +58,7 @@ private: TEST_CASE(nullpointer1); TEST_CASE(nullpointer2); - - TEST_CASE(invalidpointer); + TEST_CASE(nullpointer3); // dereferencing struct and then checking if it's null TEST_CASE(oldStylePointerCast); } @@ -446,40 +445,20 @@ private: ASSERT_EQUALS("", errout.str()); } - - - void checkInvalidPointer(const char code[]) - { - // Tokenize.. - Tokenizer tokenizer; - std::istringstream istr(code); - tokenizer.tokenize(istr, "test.cpp"); - tokenizer.setVarId(); - - // Clear the error buffer.. - errout.str(""); - - // Check for redundant code.. - Settings settings; - settings._checkCodingStyle = true; - CheckOther checkOther(&tokenizer, &settings, this); - checkOther.invalidPointer(); - } - - void invalidpointer() + // Dereferencing a struct and then checking if it is null + void nullpointer3() { // errors.. - checkInvalidPointer("void foo(struct ABC *abc)\n" + checkNullPointer("void foo(struct ABC *abc)\n" "{\n" " int *a = abc->a;\n" - " *a;\n" " if (!abc)\n" " ;\n" "}\n"); - ASSERT_EQUALS("[test.cpp:4]: (error) Possible invalid pointer dereference\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (error) Possible null pointer dereference\n", errout.str()); // ok dereferencing in a condition - checkInvalidPointer("void foo(struct ABC *abc)\n" + checkNullPointer("void foo(struct ABC *abc)\n" "{\n" " if (abc && abc->a);\n" " if (!abc)\n" @@ -488,7 +467,7 @@ private: ASSERT_EQUALS("", errout.str()); // ok to use a linked list.. - checkInvalidPointer("void foo(struct ABC *abc)\n" + checkNullPointer("void foo(struct ABC *abc)\n" "{\n" " abc = abc->next;\n" " if (!abc)\n" @@ -497,20 +476,18 @@ private: ASSERT_EQUALS("", errout.str()); // reassign struct.. - checkInvalidPointer("void foo(struct ABC *abc)\n" + checkNullPointer("void foo(struct ABC *abc)\n" "{\n" " a = abc->a;\n" - " *a;\n" " abc = abc->next;\n" " if (!abc)\n" " ;\n" "}\n"); ASSERT_EQUALS("", errout.str()); - checkInvalidPointer("void foo(struct ABC *abc)\n" + checkNullPointer("void foo(struct ABC *abc)\n" "{\n" " a = abc->a;\n" - " *a;\n" " f(&abc);\n" " if (!abc)\n" " ;\n" @@ -518,12 +495,11 @@ private: ASSERT_EQUALS("", errout.str()); // goto.. - checkInvalidPointer("void foo(struct ABC *abc)\n" + checkNullPointer("void foo(struct ABC *abc)\n" "{\n" " if (!abc)\n" " goto out;" " a = abc->a;\n" - " *a;\n" " return;\n" "out:\n" " if (!abc)\n"