From 3f517b5f231a0354e01a4b4907566913e0588ce9 Mon Sep 17 00:00:00 2001 From: Robert Reif Date: Sun, 11 Sep 2011 21:51:05 -0400 Subject: [PATCH] partial fix for #2960 (false negative: buffer access out of bounds) --- lib/checkbufferoverrun.cpp | 202 ++++++++++++++++++------------------- test/testbufferoverrun.cpp | 15 +++ 2 files changed, 114 insertions(+), 103 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index c734790b1..8cc1c334d 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -1511,7 +1511,7 @@ void CheckBufferOverrun::checkStructVariable() if (func_scope->type != Scope::eFunction) continue; - // is this a member function of this class/struct? + // check for member variables if (func_scope->functionOf == &*scope) { // only check non-empty function @@ -1523,129 +1523,125 @@ void CheckBufferOverrun::checkStructVariable() } } - // not a member function of this class/struct - else + // skip inner scopes.. + /** @todo false negatives: handle inner scopes someday */ + if (scope->nestedIn->isClassOrStruct()) + continue; + + std::vector varname; + varname.push_back(""); + varname.push_back(arrayInfo.varname()); + + // search the function and it's parameters + for (const Token *tok3 = func_scope->classDef; tok3 && tok3 != func_scope->classEnd; tok3 = tok3->next()) { - // skip inner scopes.. - /** @todo false negatives: handle inner scopes someday */ - if (scope->nestedIn->isClassOrStruct()) + // search for the class/struct name + if (tok3->str() != scope->className) continue; - std::vector varname; - varname.push_back(""); - varname.push_back(arrayInfo.varname()); + // Declare variable: Fred fred1; + if (Token::Match(tok3->next(), "%var% ;")) + varname[0] = tok3->strAt(1); - // search the function and it's parameters - for (const Token *tok3 = func_scope->classDef; tok3 && tok3 != func_scope->classEnd; tok3 = tok3->next()) + // Declare pointer or reference: Fred *fred1 + else if (Token::Match(tok3->next(), "*|& %var% [,);=]")) + varname[0] = tok3->strAt(2); + + else + continue; + + // check for variable sized structure + if (scope->type == Scope::eStruct && var->isPublic()) { - // search for the class/struct name - if (tok3->str() != scope->className) - continue; - - // Declare variable: Fred fred1; - if (Token::Match(tok3->next(), "%var% ;")) - varname[0] = tok3->strAt(1); - - // Declare pointer or reference: Fred *fred1 - else if (Token::Match(tok3->next(), "*|& %var% [,);=]")) - varname[0] = tok3->strAt(2); - - else - continue; - - // check for variable sized structure - if (scope->type == Scope::eStruct && var->isPublic()) + // last member of a struct with array size of 0 or 1 could be a variable sized structure + if (var->dimensions().size() == 1 && var->dimension(0) < 2 && + var->index() == (scope->varlist.size() - 1)) { - // last member of a struct with array size of 0 or 1 could be a variable sized structure - if (var->dimensions().size() == 1 && var->dimension(0) < 2 && - var->index() == (scope->varlist.size() - 1)) + // dynamically allocated so could be variable sized structure + if (tok3->next()->str() == "*") { - // dynamically allocated so could be variable sized structure - if (tok3->next()->str() == "*") + // check for allocation + if ((Token::Match(tok3->tokAt(3), "; %var% = malloc ( %num% ) ;") || + (Token::Match(tok3->tokAt(3), "; %var% = (") && + Token::Match(tok3->tokAt(6)->link(), ") malloc ( %num% ) ;"))) && + (tok3->strAt(4) == tok3->strAt(2))) { - // check for allocation - if ((Token::Match(tok3->tokAt(3), "; %var% = malloc ( %num% ) ;") || - (Token::Match(tok3->tokAt(3), "; %var% = (") && - Token::Match(tok3->tokAt(6)->link(), ") malloc ( %num% ) ;"))) && - (tok3->strAt(4) == tok3->strAt(2))) - { - MathLib::bigint size; + MathLib::bigint size; - // find size of allocation - if (tok3->strAt(3) == "(") // has cast - size = MathLib::toLongNumber(tok3->tokAt(6)->link()->strAt(3)); - else - size = MathLib::toLongNumber(tok3->strAt(8)); - - // We don't calculate the size of a structure even when we know - // the size of the members. We just assign a length of 100 for - // any struct. If the size is less than 100, we assume the - // programmer knew the size and specified it rather than using - // sizeof(struct). If the size is greater than 100, we assume - // the programmer specified the size as sizeof(struct) + number. - // Either way, this is just a guess and could be wrong. The - // information to make the right decision has been simplified - // away by the time we get here. - if (size != 100) // magic number for size of struct - { - // check if a real size was specified and give up - // malloc(10) rather than malloc(sizeof(struct)) - if (size < 100) - continue; - - // calculate real array size based on allocated size - MathLib::bigint elements = (size - 100) / arrayInfo.element_size(); - arrayInfo.num(0, arrayInfo.num(0) + elements); - } - } - - // size unknown so assume it is a variable sized structure + // find size of allocation + if (tok3->strAt(3) == "(") // has cast + size = MathLib::toLongNumber(tok3->tokAt(6)->link()->strAt(3)); else - continue; + size = MathLib::toLongNumber(tok3->strAt(8)); + + // We don't calculate the size of a structure even when we know + // the size of the members. We just assign a length of 100 for + // any struct. If the size is less than 100, we assume the + // programmer knew the size and specified it rather than using + // sizeof(struct). If the size is greater than 100, we assume + // the programmer specified the size as sizeof(struct) + number. + // Either way, this is just a guess and could be wrong. The + // information to make the right decision has been simplified + // away by the time we get here. + if (size != 100) // magic number for size of struct + { + // check if a real size was specified and give up + // malloc(10) rather than malloc(sizeof(struct)) + if (size < 100) + continue; + + // calculate real array size based on allocated size + MathLib::bigint elements = (size - 100) / arrayInfo.element_size(); + arrayInfo.num(0, arrayInfo.num(0) + elements); + } } + + // size unknown so assume it is a variable sized structure + else + continue; } } + } - // Goto end of statement. - const Token *CheckTok = NULL; - while (tok3 && tok3 != func_scope->classEnd) + // Goto end of statement. + const Token *CheckTok = NULL; + while (tok3 && tok3 != func_scope->classEnd) + { + // End of statement. + if (tok3->str() == ";") { - // End of statement. - if (tok3->str() == ";") - { - CheckTok = tok3; - break; - } - - // End of function declaration.. - if (Token::simpleMatch(tok3, ") ;")) - break; - - // Function implementation.. - if (Token::simpleMatch(tok3, ") {")) - { - CheckTok = tok3->tokAt(2); - break; - } - - tok3 = tok3->next(); + CheckTok = tok3; + break; } - if (!tok3) + // End of function declaration.. + if (Token::simpleMatch(tok3, ") ;")) break; - if (!CheckTok) - continue; + // Function implementation.. + if (Token::simpleMatch(tok3, ") {")) + { + CheckTok = tok3->tokAt(2); + break; + } - // Check variable usage.. - ArrayInfo temp = arrayInfo; - temp.varid(0); // do variable lookup by variable and member names rather than varid - std::string varnames; // use class and member name for messages - for (unsigned int i = 0; i < varname.size(); ++i) - varnames += (i == 0 ? "" : ".") + varname[i]; - temp.varname(varnames); - checkScope(CheckTok, varname, temp); + tok3 = tok3->next(); } + + if (!tok3) + break; + + if (!CheckTok) + continue; + + // Check variable usage.. + ArrayInfo temp = arrayInfo; + temp.varid(0); // do variable lookup by variable and member names rather than varid + std::string varnames; // use class and member name for messages + for (unsigned int i = 0; i < varname.size(); ++i) + varnames += (i == 0 ? "" : ".") + varname[i]; + temp.varname(varnames); + checkScope(CheckTok, varname, temp); } } } diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index c7f619f8b..b692e9b78 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -112,6 +112,7 @@ private: TEST_CASE(array_index_33); // ticket #3044 TEST_CASE(array_index_34); // ticket #3063 TEST_CASE(array_index_35); // ticket #2889 + TEST_CASE(array_index_36); // ticket #2960 TEST_CASE(array_index_multidim); TEST_CASE(array_index_switch_in_for); TEST_CASE(array_index_for_in_for); // FP: #2634 @@ -1281,6 +1282,20 @@ private: ASSERT_EQUALS("", errout.str()); } + void array_index_36() // ticket #2960 + { + check("class Fred {\n" + " Fred(const Fred &);\n" + "private:\n" + " bool m_b[2];\n" + "};\n" + "Fred::Fred(const Fred & rhs) {\n" + " m_b[2] = rhs.m_b[2];\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:7]: (error) Array 'm_b[2]' index 2 out of bounds\n" + "[test.cpp:7]: (error) Array 'rhs.m_b[2]' index 2 out of bounds\n", errout.str()); + } + void array_index_multidim() { check("void f()\n"