From 497c54a1a72d739b2a408fadeaeedb9ab66877aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Sun, 11 Dec 2011 08:16:58 +0100 Subject: [PATCH] Fixed #3168 (false negative: buffer overflow in subfunction) --- lib/checkbufferoverrun.cpp | 77 ++++++++++++++++++++++++++------------ lib/checkbufferoverrun.h | 7 +++- test/testbufferoverrun.cpp | 10 ++++- 3 files changed, 67 insertions(+), 27 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 6b7cb8637..b960a8c45 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -75,17 +75,27 @@ void CheckBufferOverrun::arrayIndexOutOfBoundsError(const std::list &callstack, const std::string &varnames) +{ + reportError(callstack, Severity::error, "bufferAccessOutOfBounds", bufferOverrunMessage(varnames)); } void CheckBufferOverrun::possibleBufferOverrunError(const Token *tok, const std::string &src, const std::string &dst, bool cat) @@ -513,8 +523,7 @@ void CheckBufferOverrun::parse_for_body(const Token *tok2, const ArrayInfo &arra } - -void CheckBufferOverrun::checkFunctionParameter(const Token &tok, unsigned int par, const ArrayInfo &arrayInfo) +void CheckBufferOverrun::checkFunctionParameter(const Token &tok, unsigned int par, const ArrayInfo &arrayInfo, std::list callstack) { // total_size : which parameter in function call takes the total size? std::map total_size; @@ -572,7 +581,7 @@ void CheckBufferOverrun::checkFunctionParameter(const Token &tok, unsigned int p for (unsigned int i = 0; i < arrayInfo.num().size(); ++i) elements *= arrayInfo.num(i); if (sz < 0 || sz > int(elements * arrayInfo.element_size())) { - bufferOverrunError(&tok, arrayInfo.varname()); + bufferOverrunError(callstack, arrayInfo.varname()); } } @@ -666,32 +675,50 @@ void CheckBufferOverrun::checkFunctionParameter(const Token &tok, unsigned int p if (Token::Match(ftok->previous(), "=|;|{|}|%op% %var% [ %num% ]")) { const MathLib::bigint index = MathLib::toLongNumber(ftok->strAt(2)); if (index >= 0 && arrayInfo.num(0) > 0 && index >= arrayInfo.num(0)) { - std::list callstack; - callstack.push_back(&tok); - callstack.push_back(ftok); + std::list callstack2(callstack); + callstack2.push_back(ftok); std::vector indexes; indexes.push_back(index); - arrayIndexOutOfBoundsError(callstack, arrayInfo, indexes); + arrayIndexOutOfBoundsError(callstack2, arrayInfo, indexes); } } } + + // Calling function.. + if (Token::Match(ftok, "%var% (")) { + ArrayInfo ai(arrayInfo); + ai.varid(parameterVarId); + checkFunctionCall(ftok, ai, callstack); + } } } } } -void CheckBufferOverrun::checkFunctionCall(const Token *tok, const ArrayInfo &arrayInfo) +void CheckBufferOverrun::checkFunctionCall(const Token *tok, const ArrayInfo &arrayInfo, std::list callstack) { + // Don't go deeper than 2 levels, the checking can get very slow + // when there is no limit + if (callstack.size() >= 2) + return; + + // Prevent recursion + for (std::list::const_iterator it = callstack.begin(); it != callstack.end(); ++it) { + // Same function name => bail out + if (tok->str() == (*it)->str()) + return; + } + callstack.push_back(tok); // 1st parameter.. if (Token::Match(tok->tokAt(2), "%varid% ,|)", arrayInfo.varid())) - checkFunctionParameter(*tok, 1, arrayInfo); + checkFunctionParameter(*tok, 1, arrayInfo, callstack); else if (Token::Match(tok->tokAt(2), "%varid% + %num% ,|)", arrayInfo.varid())) { const ArrayInfo ai(arrayInfo.limit(MathLib::toLongNumber(tok->strAt(4)))); - checkFunctionParameter(*tok, 1, ai); + checkFunctionParameter(*tok, 1, ai, callstack); } // goto 2nd parameter and check it.. @@ -704,10 +731,10 @@ void CheckBufferOverrun::checkFunctionCall(const Token *tok, const ArrayInfo &ar break; if (tok2->str() == ",") { if (Token::Match(tok2, ", %varid% ,|)", arrayInfo.varid())) - checkFunctionParameter(*tok, 2, arrayInfo); + checkFunctionParameter(*tok, 2, arrayInfo, callstack); else if (Token::Match(tok2, ", %varid% + %num% ,|)", arrayInfo.varid())) { const ArrayInfo ai(arrayInfo.limit(MathLib::toLongNumber(tok2->strAt(3)))); - checkFunctionParameter(*tok, 2, ai); + checkFunctionParameter(*tok, 2, ai, callstack); } break; } @@ -900,10 +927,12 @@ void CheckBufferOverrun::checkScope(const Token *tok, const std::vector 0) { + std::list callstack; + callstack.push_back(tok); if (Token::Match(tok, ("%var% ( " + varnames + " ,").c_str())) - checkFunctionParameter(*tok, 1, arrayInfo); + checkFunctionParameter(*tok, 1, arrayInfo, callstack); if (Token::Match(tok, ("%var% ( %var% , " + varnames + " ,").c_str())) - checkFunctionParameter(*tok, 2, arrayInfo); + checkFunctionParameter(*tok, 2, arrayInfo, callstack); } // Loop.. @@ -975,7 +1004,8 @@ void CheckBufferOverrun::checkScope(const Token *tok, const std::vector callstack; + checkFunctionCall(tok, arrayInfo1, callstack); } // undefined behaviour: result of pointer arithmetic is out of bounds @@ -1099,7 +1129,8 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo // Check function call.. if (Token::Match(tok, "%var% (")) { - checkFunctionCall(tok, arrayInfo); + const std::list callstack; + checkFunctionCall(tok, arrayInfo, callstack); } if (Token::Match(tok, "strncpy|memcpy|memmove ( %varid% , %str% , %num% )", arrayInfo.varid())) { diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index 0b4539669..7ca4d1bb4 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -203,19 +203,22 @@ public: * \param tok token for the function name * \param par on what parameter is the array used * \param arrayInfo the array information + * \param callstack call stack. This is used to prevent recursion and to provide better error messages. Pass a empty list from checkScope etc. */ - void checkFunctionParameter(const Token &tok, const unsigned int par, const ArrayInfo &arrayInfo); + void checkFunctionParameter(const Token &tok, const unsigned int par, const ArrayInfo &arrayInfo, std::list callstack); /** * Helper function that checks if the array is used and if so calls the checkFunctionCall * @param tok token that matches "%var% (" * @param arrayInfo the array information + * \param callstack call stack. This is used to prevent recursion and to provide better error messages. Pass a empty list from checkScope etc. */ - void checkFunctionCall(const Token *tok, const ArrayInfo &arrayInfo); + void checkFunctionCall(const Token *tok, const ArrayInfo &arrayInfo, std::list callstack); void arrayIndexOutOfBoundsError(const Token *tok, const ArrayInfo &arrayInfo, const std::vector &index); void arrayIndexOutOfBoundsError(const std::list &callstack, const ArrayInfo &arrayInfo, const std::vector &index); void bufferOverrunError(const Token *tok, const std::string &varnames = ""); + void bufferOverrunError(const std::list &callstack, const std::string &varnames = ""); void strncatUsageError(const Token *tok); void outOfBoundsError(const Token *tok, const std::string &what, const bool show_size_info, const MathLib::bigint &supplied_size, const MathLib::bigint &actual_size); void sizeArgumentAsCharError(const Token *tok); diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 202ac5147..59d7763d9 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -33,8 +33,6 @@ public: private: - - void check(const char code[], bool experimental = true, const std::string &filename="test.cpp") { // Clear the error buffer.. errout.str(""); @@ -724,6 +722,14 @@ private: " foo(p+1);\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + // #3168 + check("void a(char *p) { memset(p,0,100); }\n" + "void b() {\n" + " char buf[10];\n" + " a(buf);" + "}"); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:1]: (error) Buffer access out-of-bounds: buf\n", errout.str()); }