From 8c093d0f8a129d23d9f79cae660294d2e943387b Mon Sep 17 00:00:00 2001 From: Robert Reif Date: Sat, 27 Aug 2011 21:18:39 -0400 Subject: [PATCH] refactor CheckBufferOverrun::checkScope strncpy check and change experimental to inconclusive --- lib/checkbufferoverrun.cpp | 36 ++++++++++++++++++------------------ lib/checkbufferoverrun.h | 4 ++-- test/testbufferoverrun.cpp | 8 ++++---- 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 3ea22ac99..b2c132b1c 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -155,9 +155,9 @@ void CheckBufferOverrun::sizeArgumentAsCharError(const Token *tok) } -void CheckBufferOverrun::terminateStrncpyError(const Token *tok) +void CheckBufferOverrun::terminateStrncpyError(const Token *tok, const std::string &varname) { - reportError(tok, Severity::warning, "terminateStrncpy", "After a strncpy() the buffer should be zero-terminated"); + reportError(tok, Severity::warning, "terminateStrncpy", "After a strncpy() the buffer '" + varname + "' should be zero-terminated"); } void CheckBufferOverrun::cmdLineArgsError(const Token *tok) @@ -1159,15 +1159,18 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo checkFunctionCall(tok, arrayInfo); } - if (_settings->isEnabled("style")) + if ((Token::Match(tok, "strncpy|strncat ( %varid% , %var% , %num% )", arrayInfo.varid())) || + (Token::Match(tok, "strncpy|strncat ( %varid% , %var% [ %any% ] , %num% )", arrayInfo.varid()))) { + const int offset = tok->strAt(5) == "[" ? 3 : 0; + // check for strncpy which is not terminated - if ((Token::Match(tok, "strncpy ( %varid% , %var% , %num% )", arrayInfo.varid())) || - (Token::Match(tok, "strncpy ( %varid% , %var% [ %any% ] , %num% )", arrayInfo.varid()))) + if (tok->str() == "strncpy") { // strncpy takes entire variable length as input size - const int offset = tok->strAt(5) == "[" ? 9 : 6; - if ((unsigned int)MathLib::toLongNumber(tok->strAt(offset)) >= total_size) + unsigned int num = (unsigned int)MathLib::toLongNumber(tok->strAt(6 + offset)); + + if (num >= total_size) { const Token *tok2 = tok->next()->link()->next(); for (; tok2; tok2 = tok2->next()) @@ -1176,9 +1179,9 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo { if (!Token::Match(tok2, "%varid% [ %any% ] = 0 ;", tok->tokAt(2)->varId())) { - // this is currently 'experimental'. See TestBufferOverrun::terminateStrncpy3 - if (_settings->experimental) - terminateStrncpyError(tok); + // this is currently 'inconclusive'. See TestBufferOverrun::terminateStrncpy3 + if (_settings->isEnabled("style") && _settings->inconclusive) + terminateStrncpyError(tok, tok->strAt(2)); } break; @@ -1186,24 +1189,21 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo } } } - } - // Dangerous usage of strncat.. - if (Token::Match(tok, "strncpy|strncat ( %varid% , %any% , %num% )", arrayInfo.varid())) - { + // Dangerous usage of strncat.. if (tok->str() == "strncat") { - const MathLib::bigint n = MathLib::toLongNumber(tok->strAt(6)); + const MathLib::bigint n = MathLib::toLongNumber(tok->strAt(6 + offset)); if (n >= total_size) strncatUsageError(tok); } // Dangerous usage of strncpy + strncat.. - if (Token::Match(tok->tokAt(8), "; strncat ( %varid% , %any% , %num% )", arrayInfo.varid())) + if (Token::Match(tok->tokAt(8 + offset), "; strncat ( %varid% , %any% , %num% )", arrayInfo.varid())) { - const MathLib::bigint n = MathLib::toLongNumber(tok->strAt(6)) + MathLib::toLongNumber(tok->strAt(15)); + const MathLib::bigint n = MathLib::toLongNumber(tok->strAt(6 + offset)) + MathLib::toLongNumber(tok->strAt(15 + offset)); if (n > total_size) - strncatUsageError(tok->tokAt(9)); + strncatUsageError(tok->tokAt(9 + offset)); } } diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index 82a46226a..6861db55f 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -216,7 +216,7 @@ public: void strncatUsageError(const Token *tok); void outOfBoundsError(const Token *tok, const std::string &what); void sizeArgumentAsCharError(const Token *tok); - void terminateStrncpyError(const Token *tok); + void terminateStrncpyError(const Token *tok, const std::string &varname); void negativeIndexError(const Token *tok, MathLib::bigint index); void cmdLineArgsError(const Token *tok); void pointerOutOfBoundsError(const Token *tok, const std::string &object); // UB when result of calculation is out of bounds @@ -231,7 +231,7 @@ public: c.strncatUsageError(0); c.outOfBoundsError(0, "index"); c.sizeArgumentAsCharError(0); - c.terminateStrncpyError(0); + c.terminateStrncpyError(0, "buffer"); c.negativeIndexError(0, -1); c.cmdLineArgsError(0); c.pointerOutOfBoundsError(0, "array"); diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index acdf90550..b84454f44 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -2794,7 +2794,7 @@ private: " strncpy(baz, bar, sizeof(baz));\n" " bar[99] = 0;\n" "}\n"); - ASSERT_EQUALS("[test.cpp:4]: (warning) After a strncpy() the buffer should be zero-terminated\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (warning) After a strncpy() the buffer 'baz' should be zero-terminated\n", errout.str()); // Test with invalid code that there is no segfault check("char baz[100];\n" @@ -2809,7 +2809,7 @@ private: " foo(baz);\n" " foo(baz);\n" "}\n"); - ASSERT_EQUALS("[test.cpp:4]: (warning) After a strncpy() the buffer should be zero-terminated\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (warning) After a strncpy() the buffer 'baz' should be zero-terminated\n", errout.str()); } void terminateStrncpy2() @@ -2821,7 +2821,7 @@ private: " bar[99] = 0;\n" " return baz;\n" "}\n"); - ASSERT_EQUALS("[test.cpp:4]: (warning) After a strncpy() the buffer should be zero-terminated\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (warning) After a strncpy() the buffer 'baz' should be zero-terminated\n", errout.str()); } void terminateStrncpy3() @@ -2837,7 +2837,7 @@ private: "void bar(char *p) {\n" " strncpy(p, str, 100);\n" "}\n", false); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (warning) After a strncpy() the buffer 'str' should be zero-terminated\n", errout.str()); } void recursive_long_time()