walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

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.
This commit is contained in:
PKEuS 2015-01-30 20:27:48 +01:00
parent b69528eb80
commit 98e33a189f
5 changed files with 127 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

125
lib/checkbufferoverrun.cpp
View File

@ -652,71 +652,72 @@ void CheckBufferOverrun::checkScope(const Token *tok, const std::vector<std::str
checkFunctionParameter(*tok, 2, arrayInfo, callstack);
}
// Writing data into array..
if ((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 (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<std::size_t>(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<unsigned int>(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<std::size_t>(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<unsigned int>(total_size));
}
const ArrayInfo arrayInfo1(declarationId, varnames, total_size / size, size);
const std::list<const Token *> 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<const Token *> 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;
}
}
}
/**

2
lib/checkbufferoverrun.h
View File

@ -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

2
lib/library.cpp
View File

@ -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");

59
test/testbufferoverrun.cpp
View File

@ -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"

2
test/testlibrary.cpp
View File

@ -301,7 +301,7 @@ private:
void podtype() const {
const char xmldata[] = "<?xml version=\"1.0\"?>\n"
"<def>\n"
" <podtype name=\"s16\" sizeof=\"2\"/>\n"
" <podtype name=\"s16\" size=\"2\"/>\n"
"</def>";
tinyxml2::XMLDocument doc;
doc.Parse(xmldata, sizeof(xmldata));