From 619cfbc56f111639d3f8bc6f6c37f4d55793aa52 Mon Sep 17 00:00:00 2001 From: Zachary Blair Date: Wed, 26 May 2010 01:56:34 -0700 Subject: [PATCH] Fixed #168 (buffer overflow: not enough room for the null terminator) --- lib/checkbufferoverrun.cpp | 80 ++++++++++++++++++++++++++++++++++++++ lib/checkbufferoverrun.h | 3 ++ test/testbufferoverrun.cpp | 78 +++++++++++++++++++++++++++++++++++++ 3 files changed, 161 insertions(+) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 4e343a068..8fcf99c37 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -1228,6 +1228,7 @@ void CheckBufferOverrun::bufferOverrun() { checkGlobalAndLocalVariable(); checkStructVariable(); + checkBufferAllocatedWithStrlen(); } //--------------------------------------------------------------------------- @@ -1403,6 +1404,85 @@ void CheckBufferOverrun::checkSprintfCall(const Token *tok, int size) } + +//--------------------------------------------------------------------------- +// Checking for allocating insufficient memory for copying a string by +// allocating only strlen(src) bytes instead of strlen(src) + 1 bytes (one +// extra for the terminating null character). +// Example: +// char *b = malloc(strlen(a)); // Should be malloc(strlen(a) + 1); +// strcpy(b, a); // <== Buffer overrun +//--------------------------------------------------------------------------- +void CheckBufferOverrun::checkBufferAllocatedWithStrlen() +{ + const char pattern[] = "%var% = new|malloc|g_malloc|g_try_malloc|realloc|g_realloc|g_try_realloc"; + for (const Token *tok = Token::findmatch(_tokenizer->tokens(), pattern); tok; tok = Token::findmatch(tok->next(),pattern)) + { + unsigned int dstVarId; + unsigned int srcVarId; + + // Look for allocation of a buffer based on the size of a string + if (Token::Match(tok, "%var% = malloc|g_malloc|g_try_malloc ( strlen ( %var% ) )")) + { + dstVarId = tok->varId(); + srcVarId = tok->tokAt(6)->varId(); + tok = tok->tokAt(8); + } + else if (Token::Match(tok, "%var% = new char [ strlen ( %var% ) ]")) + { + dstVarId = tok->varId(); + srcVarId = tok->tokAt(7)->varId(); + tok = tok->tokAt(9); + } + else if (Token::Match(tok, "%var% = realloc|g_realloc|g_try_realloc ( %var% , strlen ( %var% ) )")) + { + dstVarId = tok->varId(); + srcVarId = tok->tokAt(8)->varId(); + tok = tok->tokAt(10); + } + else + continue; + + int indentlevel = 0; + for (; tok && tok->next(); tok = tok->next()) + { + // To avoid false positives and added complexity, we will only look for + // improper usage of the buffer within the block that it was allocated + if (tok->str() == "{") + { + ++indentlevel; + } + + else if (tok->str() == "}") + { + --indentlevel; + if (indentlevel < 0) + return; + } + + // If the buffers are modified, we can't be sure of their sizes + if (tok->varId() == srcVarId || tok->varId() == dstVarId) + break; + + if (Token::Match(tok, "strcpy ( %varid% , %var% )", dstVarId) && + tok->tokAt(4)->varId() == srcVarId) + { + bufferOverrun(tok); + } + else if (Token::Match(tok, "sprintf ( %varid% , %str% , %var% )", dstVarId) && + tok->tokAt(6)->varId() == srcVarId && + tok->tokAt(4)->str().find("%s") != std::string::npos) + { + bufferOverrun(tok); + } + + } + + } +} +//--------------------------------------------------------------------------- + + void CheckBufferOverrun::negativeIndexError(const Token *tok, long index) { std::ostringstream ostr; diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index e25a66fd7..e526d66b2 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -95,6 +95,9 @@ public: /** Check for buffer overruns - locate global variables and local function variables and check them with the checkScope function */ void checkGlobalAndLocalVariable(); + /** Check for buffer overruns due to allocating strlen(src) bytes instead of (strlen(src)+1) bytes before copying a string */ + void checkBufferAllocatedWithStrlen(); + /** Check for negative index */ void negativeIndex(); diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index b0facd6f8..083de993f 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -122,6 +122,7 @@ private: TEST_CASE(buffer_overrun_11); TEST_CASE(buffer_overrun_12); TEST_CASE(buffer_overrun_13); + TEST_CASE(buffer_overrun_14); TEST_CASE(sprintf1); TEST_CASE(sprintf2); @@ -1521,6 +1522,83 @@ private: ASSERT_EQUALS("[test.cpp:3]: (error) Buffer access out-of-bounds: a\n", errout.str()); } + void buffer_overrun_14() + { + check("void f(char *a) {\n" + " char *b = new char[strlen(a)];\n" + " strcpy(b, a);\n" + " return b;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (error) Buffer access out-of-bounds\n", errout.str()); + + check("void f(char *a) {\n" + " char *b = new char[strlen(a) + 1];\n" + " strcpy(b, a);\n" + " return b;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void f(char *a) {\n" + " char *b = new char[strlen(a)];\n" + " a[0] = '\\0';\n" + " strcpy(b, a);\n" + " return b;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void f(char *a) {\n" + " char *b = malloc(strlen(a));\n" + " b = realloc(b, 10000);\n" + " strcpy(b, a);\n" + " return b;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void f(char *a) {\n" + " char *b = malloc(strlen(a));\n" + " strcpy(b, a);\n" + " return b;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (error) Buffer access out-of-bounds\n", errout.str()); + + check("void f(char *a) {\n" + " char *b = malloc(strlen(a));\n" + " if (1) {\n" + " strcpy(b, a);\n" + " }\n" + " return b;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Buffer access out-of-bounds\n", errout.str()); + + check("void f(char *a) {\n" + " char *b = malloc(strlen(a) + 1);\n" + " strcpy(b, a);\n" + " return b;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void f(char *a, char *c) {\n" + " char *b = realloc(c, strlen(a));\n" + " strcpy(b, a);\n" + " return b;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (error) Buffer access out-of-bounds\n", errout.str()); + + check("void f(char *a, char *c) {\n" + " char *b = realloc(c, strlen(a) + 1);\n" + " strcpy(b, a);\n" + " return b;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void f(char *a) {\n" + " char *b = malloc(strlen(a));\n" + " sprintf(b, \"%s\", a);\n" + " return b;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (error) Buffer access out-of-bounds\n", errout.str()); + } + void sprintf1() { check("void f()\n"