From 0d18050b55b451dd9c77efb69717f1d59bf26732 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Fri, 5 Feb 2010 17:35:18 +0100 Subject: [PATCH] Fixed #984 (improve output if strncpy is followed by strncat) --- lib/checkother.cpp | 39 +++++++++++++++++++++++++++++++++++---- lib/checkother.h | 2 ++ test/testother.cpp | 21 +++++++++++++-------- 3 files changed, 50 insertions(+), 12 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 0ff5f72f2..12d99258f 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1352,7 +1352,7 @@ class CheckUninitVar : public ExecutionPath public: // Startup constructor CheckUninitVar(Check *c) - : ExecutionPath(c, 0), pointer(false), array(false), alloc(false) + : ExecutionPath(c, 0), pointer(false), array(false), alloc(false), strncpy_(false) { } @@ -1367,7 +1367,7 @@ private: // internal constructor for creating extra checks CheckUninitVar(Check *c, unsigned int v, const std::string &name, bool p, bool a) - : ExecutionPath(c, v), varname(name), pointer(p), array(a), alloc(false) + : ExecutionPath(c, v), varname(name), pointer(p), array(a), alloc(false), strncpy_(false) { } @@ -1375,6 +1375,7 @@ private: const bool pointer; const bool array; bool alloc; + bool strncpy_; // p = malloc .. static void alloc_pointer(std::list &checks, unsigned int varid) @@ -1488,6 +1489,26 @@ private: } + /** Initialize an array with strncpy.. */ + static void init_strncpy(std::list &checks, const Token *tok) + { + const unsigned int varid(tok->varId()); + if (!varid) + return; + + std::list::const_iterator it; + for (it = checks.begin(); it != checks.end(); ++it) + { + CheckUninitVar *c = dynamic_cast(*it); + if (c && c->varId == varid) + { + c->strncpy_ = true; + } + } + } + + + /** * use - called from the use* functions below. * @param foundError this is set to true if an error is found @@ -1526,7 +1547,9 @@ private: CheckOther *checkOther = dynamic_cast(c->owner); if (checkOther) { - if (c->pointer && c->alloc) + if (c->strncpy_) + checkOther->uninitstringError(tok, c->varname); + else if (c->pointer && c->alloc) checkOther->uninitdataError(tok, c->varname); else checkOther->uninitvarError(tok, c->varname); @@ -1794,8 +1817,11 @@ private: } // strncpy doesn't 0-terminate first parameter - if (Token::Match(&tok, "strncpy (")) + if (Token::Match(&tok, "strncpy ( %var% ,")) + { + init_strncpy(checks, tok.tokAt(2)); return tok.next()->link(); + } if (Token::Match(&tok, "asm ( )")) { @@ -2178,6 +2204,11 @@ void CheckOther::nullPointerError(const Token *tok, const std::string &varname, reportError(tok, Severity::error, "nullPointer", "Possible null pointer dereference: " + varname + " - otherwise it is redundant to check if " + varname + " is null at line " + MathLib::toString(line)); } +void CheckOther::uninitstringError(const Token *tok, const std::string &varname) +{ + reportError(tok, Severity::error, "uninitstring", "Dangerous usage of '" + varname + "' (strncpy doesn't always 0-terminate it)"); +} + void CheckOther::uninitdataError(const Token *tok, const std::string &varname) { reportError(tok, Severity::error, "uninitdata", "Data is allocated but not initialized: " + varname); diff --git a/lib/checkother.h b/lib/checkother.h index 5ef652539..930d998eb 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -155,6 +155,7 @@ public: void nullPointerError(const Token *tok); // variable name unknown / doesn't exist void nullPointerError(const Token *tok, const std::string &varname); void nullPointerError(const Token *tok, const std::string &varname, const int line); + void uninitstringError(const Token *tok, const std::string &varname); void uninitdataError(const Token *tok, const std::string &varname); void uninitvarError(const Token *tok, const std::string &varname); void zerodivError(const Token *tok); @@ -166,6 +167,7 @@ public: sprintfOverlappingDataError(0, "varname"); udivError(0); nullPointerError(0, "pointer"); + uninitstringError(0, "varname"); uninitdataError(0, "varname"); uninitvarError(0, "varname"); zerodivError(0); diff --git a/test/testother.cpp b/test/testother.cpp index 918ae1d81..7a203900a 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -77,6 +77,7 @@ private: TEST_CASE(uninitvar_enum); // enum variables TEST_CASE(uninitvar_if); // handling if/while/switch TEST_CASE(uninitvar_references); // references + TEST_CASE(uninitvar_strncpy); // strncpy doesn't always 0-terminate TEST_CASE(uninitvar_func); // analyse functions TEST_CASE(oldStylePointerCast); @@ -1540,14 +1541,6 @@ private: " strchr(s, ' ');\n" "};\n"); ASSERT_EQUALS("[test.cpp:4]: (error) Uninitialized variable: s\n", errout.str()); - - checkUninitVar("void f()\n" - "{\n" - " char s[20];\n" - " strncpy(s, \"abcde\", 2);\n" - " strcat(s, \"abc\");\n" - "};\n"); - ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: s\n", errout.str()); } // alloc.. @@ -1698,6 +1691,18 @@ private: ASSERT_EQUALS("", errout.str()); } + // strncpy doesn't always 0-terminate.. + void uninitvar_strncpy() + { + checkUninitVar("void f()\n" + "{\n" + " char a[100];\n" + " strncpy(a, s, 20);\n" + " strncat(a, s, 20);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (error) Dangerous usage of 'a' (strncpy doesn't always 0-terminate it)\n", errout.str()); + } + std::string analyseFunctions(const char code[]) {