From 98e33a189fcfa5a70169d0bbe556cf6ec3adbe80 Mon Sep 17 00:00:00 2001 From: PKEuS Date: Fri, 30 Jan 2015 20:27:48 +0100 Subject: [PATCH] Enhanced CheckBufferOverrun: - Fixed bug in library: manual and existing libraries use "size", but library.cpp reads "sizeof" as podtype attribute - Fixed a couple of bugs in handling unknown size in checkbufferoverrun.cpp, get size from library if available. --- lib/checkbufferoverrun.cpp | 125 ++++++++++++++++++++----------------- lib/checkbufferoverrun.h | 2 +- lib/library.cpp | 2 +- test/testbufferoverrun.cpp | 59 ++++++++++++++++- test/testlibrary.cpp | 2 +- 5 files changed, 127 insertions(+), 63 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index ff172af19..5a653f142 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -652,71 +652,72 @@ void CheckBufferOverrun::checkScope(const Token *tok, const std::vector 0 && Token::Match(tok, "strcpy|strcat ( %varid% , %str% )", declarationId)) || - (declarationId == 0 && Token::Match(tok, ("strcpy|strcat ( " + varnames + " , %str% )").c_str()))) { - const std::size_t len = Token::getStrLength(tok->tokAt(varcount + 4)); - if (total_size > 0 && len >= (unsigned int)total_size) { - bufferOverrunError(tok, declarationId > 0 ? emptyString : varnames); - continue; - } - } else if ((declarationId > 0 && Token::Match(tok, "strcpy|strcat ( %varid% , %var% )", declarationId)) || - (declarationId == 0 && Token::Match(tok, ("strcpy|strcat ( " + varnames + " , %var% )").c_str()))) { - const Variable *var = tok->tokAt(4)->variable(); - if (var && var->isArray() && var->dimensions().size() == 1) { - const std::size_t len = (std::size_t)var->dimension(0); - if (total_size > 0 && len > (unsigned int)total_size) { - if (_settings->inconclusive) - possibleBufferOverrunError(tok, tok->strAt(4), tok->strAt(2), tok->str() == "strcat"); + if (total_size > 0) { + // Writing data into array.. + if (total_size > 0 && (declarationId > 0 && Token::Match(tok, "strcpy|strcat ( %varid% , %str% )", declarationId)) || + (declarationId == 0 && Token::Match(tok, ("strcpy|strcat ( " + varnames + " , %str% )").c_str()))) { + const std::size_t len = Token::getStrLength(tok->tokAt(varcount + 4)); + if (len >= (unsigned int)total_size) { + bufferOverrunError(tok, declarationId > 0 ? emptyString : varnames); continue; } - } - } - - // Detect few strcat() calls - const std::string strcatPattern = declarationId > 0 ? std::string("strcat ( %varid% , %str% ) ;") : ("strcat ( " + varnames + " , %str% ) ;"); - if (Token::Match(tok, strcatPattern.c_str(), declarationId)) { - std::size_t charactersAppend = 0; - const Token *tok2 = tok; - - while (Token::Match(tok2, strcatPattern.c_str(), declarationId)) { - charactersAppend += Token::getStrLength(tok2->tokAt(4 + varcount)); - if (charactersAppend >= static_cast(total_size)) { - bufferOverrunError(tok2); - break; + } else if ((declarationId > 0 && Token::Match(tok, "strcpy|strcat ( %varid% , %var% )", declarationId)) || + (declarationId == 0 && Token::Match(tok, ("strcpy|strcat ( " + varnames + " , %var% )").c_str()))) { + const Variable *var = tok->tokAt(4)->variable(); + if (var && var->isArray() && var->dimensions().size() == 1) { + const std::size_t len = (std::size_t)var->dimension(0); + if (len > (unsigned int)total_size) { + if (_settings->inconclusive) + possibleBufferOverrunError(tok, tok->strAt(4), tok->strAt(2), tok->str() == "strcat"); + continue; + } } - tok2 = tok2->tokAt(7 + varcount); } - } - // sprintf.. - // TODO: change total_size to an unsigned value and remove the "&& total_size > 0" check. - const std::string sprintfPattern = declarationId > 0 ? std::string("sprintf ( %varid% , %str% [,)]") : ("sprintf ( " + varnames + " , %str% [,)]"); - if (Token::Match(tok, sprintfPattern.c_str(), declarationId) && total_size > 0) { - checkSprintfCall(tok, static_cast(total_size)); - } + // Detect few strcat() calls + const std::string strcatPattern = declarationId > 0 ? std::string("strcat ( %varid% , %str% ) ;") : ("strcat ( " + varnames + " , %str% ) ;"); + if (Token::Match(tok, strcatPattern.c_str(), declarationId)) { + std::size_t charactersAppend = 0; + const Token *tok2 = tok; - // snprintf.. - const std::string snprintfPattern = declarationId > 0 ? std::string("snprintf ( %varid% , %num% ,") : ("snprintf ( " + varnames + " , %num% ,"); - if (Token::Match(tok, snprintfPattern.c_str(), declarationId)) { - const MathLib::bigint n = MathLib::toLongNumber(tok->strAt(4 + varcount)); - if ((n > total_size) && total_size > 0) - outOfBoundsError(tok->tokAt(4 + varcount), "snprintf size", true, n, total_size); - } + while (Token::Match(tok2, strcatPattern.c_str(), declarationId)) { + charactersAppend += Token::getStrLength(tok2->tokAt(4 + varcount)); + if (charactersAppend >= static_cast(total_size)) { + bufferOverrunError(tok2); + break; + } + tok2 = tok2->tokAt(7 + varcount); + } + } - // Check function call.. - if (Token::Match(tok, "%var% (") && total_size > 0) { - // No varid => function calls are not handled - if (declarationId == 0) - continue; + // sprintf.. + const std::string sprintfPattern = declarationId > 0 ? std::string("sprintf ( %varid% , %str% [,)]") : ("sprintf ( " + varnames + " , %str% [,)]"); + if (Token::Match(tok, sprintfPattern.c_str(), declarationId)) { + checkSprintfCall(tok, static_cast(total_size)); + } - const ArrayInfo arrayInfo1(declarationId, varnames, total_size / size, size); - const std::list callstack; - checkFunctionCall(tok, arrayInfo1, callstack); + // snprintf.. + const std::string snprintfPattern = declarationId > 0 ? std::string("snprintf ( %varid% , %num% ,") : ("snprintf ( " + varnames + " , %num% ,"); + if (Token::Match(tok, snprintfPattern.c_str(), declarationId)) { + const MathLib::bigint n = MathLib::toLongNumber(tok->strAt(4 + varcount)); + if (n > total_size) + outOfBoundsError(tok->tokAt(4 + varcount), "snprintf size", true, n, total_size); + } + + // Check function call.. + if (Token::Match(tok, "%var% (")) { + // No varid => function calls are not handled + if (declarationId == 0) + continue; + + const ArrayInfo arrayInfo1(declarationId, varnames, total_size / size, size); + const std::list callstack; + checkFunctionCall(tok, arrayInfo1, callstack); + } } // undefined behaviour: result of pointer arithmetic is out of bounds - else if (declarationId && Token::Match(tok, "= %varid% + %num% ;", declarationId)) { + if (declarationId && Token::Match(tok, "= %varid% + %num% ;", declarationId)) { const MathLib::bigint index = MathLib::toLongNumber(tok->strAt(3)); if (isPortabilityEnabled && index > size) pointerOutOfBoundsError(tok->tokAt(2)); @@ -1093,7 +1094,7 @@ void CheckBufferOverrun::checkGlobalAndLocalVariable() break; if (tok->str() == "{") tok = tok->next(); - const ArrayInfo arrayInfo(var, _tokenizer, i); + const ArrayInfo arrayInfo(var, _tokenizer, &_settings->library, i); checkScope(tok, arrayInfo); } } @@ -1211,7 +1212,7 @@ void CheckBufferOverrun::checkStructVariable() for (var = scope->varlist.begin(); var != scope->varlist.end(); ++var) { if (var->isArray()) { // create ArrayInfo from the array variable - ArrayInfo arrayInfo(&*var, _tokenizer); + ArrayInfo arrayInfo(&*var, _tokenizer, &_settings->library); // find every function const std::size_t functions = symbolDatabase->functionScopes.size(); @@ -1416,7 +1417,7 @@ void CheckBufferOverrun::bufferOverrun2() if (var->dimension(0) <= 1 && Token::simpleMatch(var->nameToken()->linkAt(1),"] ; }")) continue; - ArrayInfo arrayInfo(var,_tokenizer); + ArrayInfo arrayInfo(var, _tokenizer, &_settings->library); arrayInfo.varname(varname); valueFlowCheckArrayIndex(tok->next(), arrayInfo); @@ -1728,7 +1729,7 @@ CheckBufferOverrun::ArrayInfo::ArrayInfo() { } -CheckBufferOverrun::ArrayInfo::ArrayInfo(const Variable *var, const Tokenizer *tokenizer, const unsigned int forcedeclid) +CheckBufferOverrun::ArrayInfo::ArrayInfo(const Variable *var, const Tokenizer *tokenizer, const Library *library, const unsigned int forcedeclid) : _varname(var->name()), _declarationId((forcedeclid == 0U) ? var->declarationId() : forcedeclid) { for (std::size_t i = 0; i < var->dimensions().size(); i++) @@ -1737,8 +1738,14 @@ CheckBufferOverrun::ArrayInfo::ArrayInfo(const Variable *var, const Tokenizer *t _element_size = tokenizer->sizeOfType(var->typeEndToken()); else if (var->typeStartToken()->str() == "struct") _element_size = 100; - else + else { _element_size = tokenizer->sizeOfType(var->typeEndToken()); + if (!_element_size) { // try libary + const Library::PodType* podtype = library->podtype(var->typeEndToken()->str()); + if (podtype) + _element_size = podtype->size; + } + } } /** diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index 19517130d..4038a3dfa 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -126,7 +126,7 @@ public: public: ArrayInfo(); - ArrayInfo(const Variable *var, const Tokenizer *tokenizer, const unsigned int forcedeclid = 0); + ArrayInfo(const Variable *var, const Tokenizer *tokenizer, const Library *library, const unsigned int forcedeclid = 0); /** * Create array info with specified data diff --git a/lib/library.cpp b/lib/library.cpp index 67b5e8311..7816c6331 100644 --- a/lib/library.cpp +++ b/lib/library.cpp @@ -503,7 +503,7 @@ Library::Error Library::load(const tinyxml2::XMLDocument &doc) if (!name) return Error(MISSING_ATTRIBUTE, "name"); PodType podType = {0}; - const char * const size = node->Attribute("sizeof"); + const char * const size = node->Attribute("size"); if (size) podType.size = atoi(size); const char * const sign = node->Attribute("sign"); diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 3693f8ab3..5c16f496d 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -82,6 +82,7 @@ private: Tokenizer tokenizer(&settings, this); std::istringstream istr(code); tokenizer.tokenize(istr, filename); + tokenizer.simplifyTokenList2(); // Clear the error buffer.. errout.str(""); @@ -223,6 +224,7 @@ private: TEST_CASE(buffer_overrun_bailoutIfSwitch); // ticket #2378 : bailoutIfSwitch TEST_CASE(buffer_overrun_function_array_argument); TEST_CASE(possible_buffer_overrun_1); // #3035 + TEST_CASE(buffer_overrun_readSizeFromCfg); TEST_CASE(valueflow_string); // using ValueFlow string values in checking @@ -2733,7 +2735,7 @@ private: " char* const source = (char*) malloc(sizeof(dest));\n" " memcpy(&dest, source + sizeof(double), sizeof(dest));\n" "}"); - TODO_ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds.\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds.\n", errout.str()); checkstd("void foo() {\n" " double dest = 23.0;\n" @@ -2932,6 +2934,61 @@ private: ASSERT_EQUALS("", errout.str()); } + void buffer_overrun_readSizeFromCfg() { + // Attempt to get size from Cfg files, no false positives if size is not specified + checkstd("void f() {\n" + " uint8_t str[256];\n" + " str[0] = 0;\n" + " strcat(str, \"toto\");\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + checkstd("void f() {\n" + " uint8_t str[2];\n" + " str[0] = 0;\n" + " strcat(str, \"toto\");\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds: str\n", errout.str()); + + checkstd("void f() {\n" + " int_fast8_t str[256];\n" + " str[0] = 0;\n" + " strcat(str, \"toto\");\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // The same for structs, where the message comes from a different check + checkstd("typedef struct mystruct_s {\n" + " uint8_t str[256];\n" + "} mystruct_t;\n" + "void f() {\n" + " mystruct_t ms;\n" + " ms.str[0] = 0;\n" + " strcat((char*)ms.str, \"toto\");\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + checkstd("typedef struct mystruct_s {\n" + " uint8_t str[2];\n" + "} mystruct_t;\n" + "void f() {\n" + " mystruct_t ms;\n" + " ms.str[0] = 0;\n" + " strcat((char*)ms.str, \"toto\");\n" + "}"); + ASSERT_EQUALS("[test.cpp:7]: (error) Buffer is accessed out of bounds: ms.str\n", errout.str()); + + checkstd("typedef struct mystruct_s {\n" + " int_fast8_t str[256];\n" + "} mystruct_t;\n" + "void f() {\n" + " mystruct_t ms;\n" + " ms.str[0] = 0;\n" + " strcat((char*)ms.str, \"toto\");\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + void valueflow_string() { // using ValueFlow string values in checking checkstd("void f() {\n" " char buf[3];\n" diff --git a/test/testlibrary.cpp b/test/testlibrary.cpp index 3774c8b48..ec737a512 100644 --- a/test/testlibrary.cpp +++ b/test/testlibrary.cpp @@ -301,7 +301,7 @@ private: void podtype() const { const char xmldata[] = "\n" "\n" - " \n" + " \n" ""; tinyxml2::XMLDocument doc; doc.Parse(xmldata, sizeof(xmldata));