Skip to content

Commit 98e33a1

Browse files
PKEuSPKEuS
committed
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.
1 parent b69528e commit 98e33a1

File tree

5 files changed

+124
-60
lines changed

5 files changed

+124
-60
lines changed

‎lib/checkbufferoverrun.cpp‎

Lines changed: 63 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -652,71 +652,72 @@ void CheckBufferOverrun::checkScope(const Token *tok, const std::vector<std::str
652652
checkFunctionParameter(*tok, 2, arrayInfo, callstack);
653653
}
654654

655-
// Writing data into array..
656-
if ((declarationId > 0 && Token::Match(tok, "strcpy|strcat ( %varid% , %str% )", declarationId)) ||
657-
(declarationId == 0 && Token::Match(tok, ("strcpy|strcat ( " + varnames + " , %str% )").c_str()))) {
658-
const std::size_t len = Token::getStrLength(tok->tokAt(varcount + 4));
659-
if (total_size > 0 && len >= (unsigned int)total_size) {
660-
bufferOverrunError(tok, declarationId > 0 ? emptyString : varnames);
661-
continue;
662-
}
663-
} else if ((declarationId > 0 && Token::Match(tok, "strcpy|strcat ( %varid% , %var% )", declarationId)) ||
664-
(declarationId == 0 && Token::Match(tok, ("strcpy|strcat ( " + varnames + " , %var% )").c_str()))) {
665-
const Variable *var = tok->tokAt(4)->variable();
666-
if (var && var->isArray() && var->dimensions().size() == 1) {
667-
const std::size_t len = (std::size_t)var->dimension(0);
668-
if (total_size > 0 && len > (unsigned int)total_size) {
669-
if (_settings->inconclusive)
670-
possibleBufferOverrunError(tok, tok->strAt(4), tok->strAt(2), tok->str() == "strcat");
655+
if (total_size > 0) {
656+
// Writing data into array..
657+
if (total_size > 0 && (declarationId > 0 && Token::Match(tok, "strcpy|strcat ( %varid% , %str% )", declarationId)) ||
658+
(declarationId == 0 && Token::Match(tok, ("strcpy|strcat ( " + varnames + " , %str% )").c_str()))) {
659+
const std::size_t len = Token::getStrLength(tok->tokAt(varcount + 4));
660+
if (len >= (unsigned int)total_size) {
661+
bufferOverrunError(tok, declarationId > 0 ? emptyString : varnames);
671662
continue;
672663
}
664+
} else if ((declarationId > 0 && Token::Match(tok, "strcpy|strcat ( %varid% , %var% )", declarationId)) ||
665+
(declarationId == 0 && Token::Match(tok, ("strcpy|strcat ( " + varnames + " , %var% )").c_str()))) {
666+
const Variable *var = tok->tokAt(4)->variable();
667+
if (var && var->isArray() && var->dimensions().size() == 1) {
668+
const std::size_t len = (std::size_t)var->dimension(0);
669+
if (len > (unsigned int)total_size) {
670+
if (_settings->inconclusive)
671+
possibleBufferOverrunError(tok, tok->strAt(4), tok->strAt(2), tok->str() == "strcat");
672+
continue;
673+
}
674+
}
673675
}
674-
}
675676

676-
// Detect few strcat() calls
677-
const std::string strcatPattern = declarationId > 0 ? std::string("strcat ( %varid% , %str% ) ;") : ("strcat ( " + varnames + " , %str% ) ;");
678-
if (Token::Match(tok, strcatPattern.c_str(), declarationId)) {
679-
std::size_t charactersAppend = 0;
680-
const Token *tok2 = tok;
677+
// Detect few strcat() calls
678+
const std::string strcatPattern = declarationId > 0 ? std::string("strcat ( %varid% , %str% ) ;") : ("strcat ( " + varnames + " , %str% ) ;");
679+
if (Token::Match(tok, strcatPattern.c_str(), declarationId)) {
680+
std::size_t charactersAppend = 0;
681+
const Token *tok2 = tok;
681682

682-
while (Token::Match(tok2, strcatPattern.c_str(), declarationId)) {
683-
charactersAppend += Token::getStrLength(tok2->tokAt(4 + varcount));
684-
if (charactersAppend >= static_cast<std::size_t>(total_size)) {
685-
bufferOverrunError(tok2);
686-
break;
683+
while (Token::Match(tok2, strcatPattern.c_str(), declarationId)) {
684+
charactersAppend += Token::getStrLength(tok2->tokAt(4 + varcount));
685+
if (charactersAppend >= static_cast<std::size_t>(total_size)) {
686+
bufferOverrunError(tok2);
687+
break;
688+
}
689+
tok2 = tok2->tokAt(7 + varcount);
687690
}
688-
tok2 = tok2->tokAt(7 + varcount);
689691
}
690-
}
691692

692-
// sprintf..
693-
// TODO: change total_size to an unsigned value and remove the "&& total_size > 0" check.
694-
const std::string sprintfPattern = declarationId > 0 ? std::string("sprintf ( %varid% , %str% [,)]") : ("sprintf ( " + varnames + " , %str% [,)]");
695-
if (Token::Match(tok, sprintfPattern.c_str(), declarationId) && total_size > 0) {
696-
checkSprintfCall(tok, static_cast<unsigned int>(total_size));
697-
}
693+
// sprintf..
694+
const std::string sprintfPattern = declarationId > 0 ? std::string("sprintf ( %varid% , %str% [,)]") : ("sprintf ( " + varnames + " , %str% [,)]");
695+
if (Token::Match(tok, sprintfPattern.c_str(), declarationId)) {
696+
checkSprintfCall(tok, static_cast<unsigned int>(total_size));
697+
}
698698

699-
// snprintf..
700-
const std::string snprintfPattern = declarationId > 0 ? std::string("snprintf ( %varid% , %num% ,") : ("snprintf ( " + varnames + " , %num% ,");
701-
if (Token::Match(tok, snprintfPattern.c_str(), declarationId)) {
702-
const MathLib::bigint n = MathLib::toLongNumber(tok->strAt(4 + varcount));
703-
if ((n > total_size) && total_size > 0)
704-
outOfBoundsError(tok->tokAt(4 + varcount), "snprintf size", true, n, total_size);
705-
}
699+
// snprintf..
700+
const std::string snprintfPattern = declarationId > 0 ? std::string("snprintf ( %varid% , %num% ,") : ("snprintf ( " + varnames + " , %num% ,");
701+
if (Token::Match(tok, snprintfPattern.c_str(), declarationId)) {
702+
const MathLib::bigint n = MathLib::toLongNumber(tok->strAt(4 + varcount));
703+
if (n > total_size)
704+
outOfBoundsError(tok->tokAt(4 + varcount), "snprintf size", true, n, total_size);
705+
}
706706

707-
// Check function call..
708-
if (Token::Match(tok, "%var% (") && total_size > 0) {
709-
// No varid => function calls are not handled
710-
if (declarationId == 0)
711-
continue;
707+
// Check function call..
708+
if (Token::Match(tok, "%var% (")) {
709+
// No varid => function calls are not handled
710+
if (declarationId == 0)
711+
continue;
712712

713-
const ArrayInfo arrayInfo1(declarationId, varnames, total_size / size, size);
714-
const std::list<const Token *> callstack;
715-
checkFunctionCall(tok, arrayInfo1, callstack);
713+
const ArrayInfo arrayInfo1(declarationId, varnames, total_size / size, size);
714+
const std::list<const Token *> callstack;
715+
checkFunctionCall(tok, arrayInfo1, callstack);
716+
}
716717
}
717718

718719
// undefined behaviour: result of pointer arithmetic is out of bounds
719-
else if (declarationId && Token::Match(tok, "= %varid% + %num% ;", declarationId)) {
720+
if (declarationId && Token::Match(tok, "= %varid% + %num% ;", declarationId)) {
720721
const MathLib::bigint index = MathLib::toLongNumber(tok->strAt(3));
721722
if (isPortabilityEnabled && index > size)
722723
pointerOutOfBoundsError(tok->tokAt(2));
@@ -1093,7 +1094,7 @@ void CheckBufferOverrun::checkGlobalAndLocalVariable()
10931094
break;
10941095
if (tok->str() == "{")
10951096
tok = tok->next();
1096-
const ArrayInfo arrayInfo(var, _tokenizer, i);
1097+
const ArrayInfo arrayInfo(var, _tokenizer, &_settings->library, i);
10971098
checkScope(tok, arrayInfo);
10981099
}
10991100
}
@@ -1211,7 +1212,7 @@ void CheckBufferOverrun::checkStructVariable()
12111212
for (var = scope->varlist.begin(); var != scope->varlist.end(); ++var) {
12121213
if (var->isArray()) {
12131214
// create ArrayInfo from the array variable
1214-
ArrayInfo arrayInfo(&*var, _tokenizer);
1215+
ArrayInfo arrayInfo(&*var, _tokenizer, &_settings->library);
12151216

12161217
// find every function
12171218
const std::size_t functions = symbolDatabase->functionScopes.size();
@@ -1416,7 +1417,7 @@ void CheckBufferOverrun::bufferOverrun2()
14161417
if (var->dimension(0) <= 1 && Token::simpleMatch(var->nameToken()->linkAt(1),"] ; }"))
14171418
continue;
14181419

1419-
ArrayInfo arrayInfo(var,_tokenizer);
1420+
ArrayInfo arrayInfo(var, _tokenizer, &_settings->library);
14201421
arrayInfo.varname(varname);
14211422

14221423
valueFlowCheckArrayIndex(tok->next(), arrayInfo);
@@ -1728,7 +1729,7 @@ CheckBufferOverrun::ArrayInfo::ArrayInfo()
17281729
{
17291730
}
17301731

1731-
CheckBufferOverrun::ArrayInfo::ArrayInfo(const Variable *var, const Tokenizer *tokenizer, const unsigned int forcedeclid)
1732+
CheckBufferOverrun::ArrayInfo::ArrayInfo(const Variable *var, const Tokenizer *tokenizer, const Library *library, const unsigned int forcedeclid)
17321733
: _varname(var->name()), _declarationId((forcedeclid == 0U) ? var->declarationId() : forcedeclid)
17331734
{
17341735
for (std::size_t i = 0; i < var->dimensions().size(); i++)
@@ -1737,8 +1738,14 @@ CheckBufferOverrun::ArrayInfo::ArrayInfo(const Variable *var, const Tokenizer *t
17371738
_element_size = tokenizer->sizeOfType(var->typeEndToken());
17381739
else if (var->typeStartToken()->str() == "struct")
17391740
_element_size = 100;
1740-
else
1741+
else {
17411742
_element_size = tokenizer->sizeOfType(var->typeEndToken());
1743+
if (!_element_size) { // try libary
1744+
const Library::PodType* podtype = library->podtype(var->typeEndToken()->str());
1745+
if (podtype)
1746+
_element_size = podtype->size;
1747+
}
1748+
}
17421749
}
17431750

17441751
/**

‎lib/checkbufferoverrun.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ class CPPCHECKLIB CheckBufferOverrun : public Check {
126126

127127
public:
128128
ArrayInfo();
129-
ArrayInfo(const Variable *var, const Tokenizer *tokenizer, const unsigned int forcedeclid = 0);
129+
ArrayInfo(const Variable *var, const Tokenizer *tokenizer, const Library *library, const unsigned int forcedeclid = 0);
130130

131131
/**
132132
* Create array info with specified data

‎lib/library.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -503,7 +503,7 @@ Library::Error Library::load(const tinyxml2::XMLDocument &doc)
503503
if (!name)
504504
return Error(MISSING_ATTRIBUTE, "name");
505505
PodType podType = {0};
506-
const char * const size = node->Attribute("sizeof");
506+
const char * const size = node->Attribute("size");
507507
if (size)
508508
podType.size = atoi(size);
509509
const char * const sign = node->Attribute("sign");

‎test/testbufferoverrun.cpp‎

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ class TestBufferOverrun : public TestFixture {
8282
Tokenizer tokenizer(&settings, this);
8383
std::istringstream istr(code);
8484
tokenizer.tokenize(istr, filename);
85+
tokenizer.simplifyTokenList2();
8586

8687
// Clear the error buffer..
8788
errout.str("");
@@ -223,6 +224,7 @@ class TestBufferOverrun : public TestFixture {
223224
TEST_CASE(buffer_overrun_bailoutIfSwitch); // ticket #2378 : bailoutIfSwitch
224225
TEST_CASE(buffer_overrun_function_array_argument);
225226
TEST_CASE(possible_buffer_overrun_1); // #3035
227+
TEST_CASE(buffer_overrun_readSizeFromCfg);
226228

227229
TEST_CASE(valueflow_string); // using ValueFlow string values in checking
228230

@@ -2733,7 +2735,7 @@ class TestBufferOverrun : public TestFixture {
27332735
" char* const source = (char*) malloc(sizeof(dest));\n"
27342736
" memcpy(&dest, source + sizeof(double), sizeof(dest));\n"
27352737
"}");
2736-
TODO_ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds.\n", "", errout.str());
2738+
ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds.\n", errout.str());
27372739

27382740
checkstd("void foo() {\n"
27392741
" double dest = 23.0;\n"
@@ -2932,6 +2934,61 @@ class TestBufferOverrun : public TestFixture {
29322934
ASSERT_EQUALS("", errout.str());
29332935
}
29342936

2937+
void buffer_overrun_readSizeFromCfg() {
2938+
// Attempt to get size from Cfg files, no false positives if size is not specified
2939+
checkstd("void f() {\n"
2940+
" uint8_t str[256];\n"
2941+
" str[0] = 0;\n"
2942+
" strcat(str, \"toto\");\n"
2943+
"}");
2944+
ASSERT_EQUALS("", errout.str());
2945+
2946+
checkstd("void f() {\n"
2947+
" uint8_t str[2];\n"
2948+
" str[0] = 0;\n"
2949+
" strcat(str, \"toto\");\n"
2950+
"}");
2951+
ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds: str\n", errout.str());
2952+
2953+
checkstd("void f() {\n"
2954+
" int_fast8_t str[256];\n"
2955+
" str[0] = 0;\n"
2956+
" strcat(str, \"toto\");\n"
2957+
"}");
2958+
ASSERT_EQUALS("", errout.str());
2959+
2960+
// The same for structs, where the message comes from a different check
2961+
checkstd("typedef struct mystruct_s {\n"
2962+
" uint8_t str[256];\n"
2963+
"} mystruct_t;\n"
2964+
"void f() {\n"
2965+
" mystruct_t ms;\n"
2966+
" ms.str[0] = 0;\n"
2967+
" strcat((char*)ms.str, \"toto\");\n"
2968+
"}");
2969+
ASSERT_EQUALS("", errout.str());
2970+
2971+
checkstd("typedef struct mystruct_s {\n"
2972+
" uint8_t str[2];\n"
2973+
"} mystruct_t;\n"
2974+
"void f() {\n"
2975+
" mystruct_t ms;\n"
2976+
" ms.str[0] = 0;\n"
2977+
" strcat((char*)ms.str, \"toto\");\n"
2978+
"}");
2979+
ASSERT_EQUALS("[test.cpp:7]: (error) Buffer is accessed out of bounds: ms.str\n", errout.str());
2980+
2981+
checkstd("typedef struct mystruct_s {\n"
2982+
" int_fast8_t str[256];\n"
2983+
"} mystruct_t;\n"
2984+
"void f() {\n"
2985+
" mystruct_t ms;\n"
2986+
" ms.str[0] = 0;\n"
2987+
" strcat((char*)ms.str, \"toto\");\n"
2988+
"}");
2989+
ASSERT_EQUALS("", errout.str());
2990+
}
2991+
29352992
void valueflow_string() { // using ValueFlow string values in checking
29362993
checkstd("void f() {\n"
29372994
" char buf[3];\n"

‎test/testlibrary.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -301,7 +301,7 @@ class TestLibrary : public TestFixture {
301301
void podtype() const {
302302
const char xmldata[] = "<?xml version=\"1.0\"?>\n"
303303
"<def>\n"
304-
" <podtype name=\"s16\" sizeof=\"2\"/>\n"
304+
" <podtype name=\"s16\" size=\"2\"/>\n"
305305
"</def>";
306306
tinyxml2::XMLDocument doc;
307307
doc.Parse(xmldata, sizeof(xmldata));

0 commit comments

Comments
 (0)