From 7451c5cece86256a6f83bbd0541f2a9bcd1f1d53 Mon Sep 17 00:00:00 2001 From: Robert Reif Date: Mon, 5 Sep 2011 15:59:41 -0400 Subject: [PATCH] warn when buffer is not zero terminated after memmove --- lib/checkbufferoverrun.cpp | 40 +++++++++++++------------------------- lib/checkbufferoverrun.h | 8 ++++---- test/testbufferoverrun.cpp | 33 ++++++++++++++++++++++++------- 3 files changed, 44 insertions(+), 37 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 99d43d929..91c1f6562 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -155,22 +155,15 @@ void CheckBufferOverrun::sizeArgumentAsCharError(const Token *tok) } -void CheckBufferOverrun::terminateStrncpyError(const Token *tok, const std::string &varname, bool conclusive) +void CheckBufferOverrun::terminateStrncpyError(const Token *tok, const std::string &varname) { - if (conclusive) - reportError(tok, Severity::warning, "terminateStrncpy", - "The buffer '" + varname + "' is not zero-terminated after the call to strncpy().\n" - "The use of strncpy() usually indicates that the programmer wants to ensure " - "the buffer is zero-terminated after the call. This will cause bugs later in the code if " - "the code assumes buffer is zero-terminated."); - else - reportError(tok, Severity::warning, "terminateStrncpy", - "The buffer '" + varname + "' may not be zero-terminated after the call to strncpy().\n" - "The use of strncpy() usually indicates that the programmer wants to ensure " - "the buffer is zero-terminated after the call. However if the (buffer) size given for " - "the strncpy() call matches the actual buffer size strncpy() does not add the " - "zero at the end of the buffer. This may cause bugs later in the code if " - "the code assumes buffer is zero-terminated."); + reportError(tok, Severity::warning, "terminateStrncpy", + "The buffer '" + varname + "' may not be zero-terminated after the call to strncpy().\n" + "The use of strncpy() usually indicates that the programmer wants to ensure " + "the buffer is zero-terminated after the call. However if the (buffer) size given for " + "the strncpy() call matches the actual buffer size strncpy() does not add the " + "zero at the end of the buffer. This may cause bugs later in the code if " + "the code assumes buffer is zero-terminated."); } void CheckBufferOverrun::cmdLineArgsError(const Token *tok) @@ -178,10 +171,10 @@ void CheckBufferOverrun::cmdLineArgsError(const Token *tok) reportError(tok, Severity::error, "insecureCmdLineArgs", "Buffer overrun possible for long cmd-line args"); } -void CheckBufferOverrun::terminateMemcpyError(const Token *tok, const std::string &varname) +void CheckBufferOverrun::bufferNotZeroTerminatedError(const Token *tok, const std::string &varname, const std::string &function) { - reportError(tok, Severity::warning, "terminateMemcpy", - "The buffer '" + varname + "' is not zero-terminated after the call to memcpy().\n" + reportError(tok, Severity::warning, "bufferNotZeroTerminated", + "The buffer '" + varname + "' is not zero-terminated after the call to " + function + "().\n" "This will cause bugs later in the code if the code assumes buffer is zero-terminated."); } @@ -1234,18 +1227,13 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo checkFunctionCall(tok, arrayInfo); } - if (Token::Match(tok, "strncpy|memcpy ( %varid% , %str% , %num% )", arrayInfo.varid())) + if (Token::Match(tok, "strncpy|memcpy|memmove ( %varid% , %str% , %num% )", arrayInfo.varid())) { unsigned int num = (unsigned int)MathLib::toLongNumber(tok->strAt(6)); if (Token::getStrLength(tok->tokAt(4)) >= total_size && total_size == num) { if (_settings->inconclusive) - { - if (tok->str() == "strncpy") - terminateStrncpyError(tok, tok->strAt(2), true); - else - terminateMemcpyError(tok, tok->strAt(2)); - } + bufferNotZeroTerminatedError(tok, tok->strAt(2), tok->str()); } } @@ -1271,7 +1259,7 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo { // this is currently 'inconclusive'. See TestBufferOverrun::terminateStrncpy3 if (_settings->isEnabled("style") && _settings->inconclusive) - terminateStrncpyError(tok, tok->strAt(2), false); + terminateStrncpyError(tok, tok->strAt(2)); } break; diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index 27ee3fb89..23d5aed95 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -216,8 +216,8 @@ 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, const std::string &varname, bool conclusive); - void terminateMemcpyError(const Token *tok, const std::string &varname); + void terminateStrncpyError(const Token *tok, const std::string &varname); + void bufferNotZeroTerminatedError(const Token *tok, const std::string &varname, const std::string &function); 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 @@ -232,8 +232,8 @@ public: c.strncatUsageError(0); c.outOfBoundsError(0, "index"); c.sizeArgumentAsCharError(0); - c.terminateStrncpyError(0, "buffer", false); - c.terminateMemcpyError(0, "buffer"); + c.terminateStrncpyError(0, "buffer"); + c.bufferNotZeroTerminatedError(0, "buffer", "strncpy"); c.negativeIndexError(0, -1); c.cmdLineArgsError(0); c.pointerOutOfBoundsError(0, "array"); diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index d700fd68d..96e8ead5a 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -231,6 +231,8 @@ private: // Access array and then check if the used index is within bounds TEST_CASE(arrayIndexThenCheck); + + TEST_CASE(bufferNotZeroTerminated); } @@ -2583,13 +2585,6 @@ private: " memchr(a, b, 10);\n" "}\n"); TODO_ASSERT_EQUALS("[test.cpp:4]: (error) Buffer access out-of-bounds\n", "", errout.str()); - - check("void f()\n" - "{\n" - " char c[6];\n" - " memcpy(c,\"hello!\",sizeof(c));\n" - "}\n"); - ASSERT_EQUALS("[test.cpp:4]: (warning) The buffer 'c' is not zero-terminated after the call to memcpy().\n", errout.str()); } // ticket #2121 - buffer access out of bounds when using uint32_t @@ -3315,6 +3310,30 @@ private: "}"); ASSERT_EQUALS("[test.cpp:2]: (style) Array index i is used before limits check\n", errout.str()); } + + void bufferNotZeroTerminated() + { + check("void f()\n" + "{\n" + " char c[6];\n" + " strncpy(c,\"hello!\",sizeof(c));\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (warning) The buffer 'c' is not zero-terminated after the call to strncpy().\n", errout.str()); + + check("void f()\n" + "{\n" + " char c[6];\n" + " memcpy(c,\"hello!\",sizeof(c));\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (warning) The buffer 'c' is not zero-terminated after the call to memcpy().\n", errout.str()); + + check("void f()\n" + "{\n" + " char c[6];\n" + " memmove(c,\"hello!\",sizeof(c));\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (warning) The buffer 'c' is not zero-terminated after the call to memmove().\n", errout.str()); + } }; REGISTER_TEST(TestBufferOverrun)