Skip to content

Commit d6be455

Browse files
lucasmroddanmar
lucasmrod
authored and
danmar
committed
Fixed cppcheck-opensource#4840 (false negative: buffer access out of bounds)
1 parent 5a9975b commit d6be455

4 files changed

Lines changed: 61 additions & 8 deletions

File tree

‎lib/checkbufferoverrun.cpp‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -319,6 +319,11 @@ static bool for_condition(const Token *tok2, unsigned int varid, std::string &mi
319319
maxMinFlipped = true;
320320
max_value = min_value;
321321
min_value = tok2->str();
322+
} else if (Token::Match(tok2, "%varid% -- ; )", varid) ||
323+
Token::Match(tok2, "-- %varid% ; )", varid)) {
324+
maxMinFlipped = true;
325+
max_value = min_value;
326+
min_value = (tok2->str() == "--") ? "1" : "0";
322327
} else {
323328
// parse condition
324329
while (tok2 && tok2->str() != ";") {
@@ -819,7 +824,8 @@ void CheckBufferOverrun::checkScopeForBody(const Token *tok, const ArrayInfo &ar
819824
}
820825
if (!tok2 || tok2->str() != ";")
821826
return;
822-
if (!for3(tok2->next(), counter_varid, min_counter_value, max_counter_value, maxMinFlipped))
827+
const bool hasFor3 = tok2->next()->str() != ")";
828+
if (hasFor3 && !for3(tok2->next(), counter_varid, min_counter_value, max_counter_value, maxMinFlipped))
823829
return;
824830

825831
if (Token::Match(tok2->next(), "%var% =") && MathLib::toLongNumber(max_counter_value) < size)

‎lib/tokenize.cpp‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3382,15 +3382,22 @@ bool Tokenizer::simplifyTokenList()
33823382

33833383
simplifyUndefinedSizeArray();
33843384

3385+
simplifyCasts();
3386+
3387+
// Simplify simple calculations before replace constants, this allows the replacement of constants that are calculated
3388+
// e.g. const static int value = sizeof(X)/sizeof(Y);
3389+
simplifyCalculations();
3390+
33853391
// Replace constants..
33863392
for (Token *tok = list.front(); tok; tok = tok->next()) {
3387-
if (Token::Match(tok, "const static| %type% %var% = %num% ;")) {
3393+
if (Token::Match(tok, "const static| %type% %var% = %num% ;") ||
3394+
Token::Match(tok, "const static| %type% %var% ( %num% ) ;")) {
33883395
unsigned int offset = 0;
33893396
if (tok->strAt(1) == "static")
33903397
offset = 1;
33913398
const unsigned int varId(tok->tokAt(2 + offset)->varId());
33923399
if (varId == 0) {
3393-
tok = tok->tokAt(5);
3400+
tok = tok->tokAt(5 + offset);
33943401
continue;
33953402
}
33963403

@@ -3413,8 +3420,6 @@ bool Tokenizer::simplifyTokenList()
34133420
}
34143421
}
34153422

3416-
simplifyCasts();
3417-
34183423
// Simplify simple calculations..
34193424
simplifyCalculations();
34203425

@@ -5937,9 +5942,10 @@ bool Tokenizer::simplifyKnownVariables()
59375942
tok = tok->previous();
59385943
goback = false;
59395944
}
5940-
if (tok->isName() && Token::Match(tok, "static| const| static| %type% const| %var% = %any% ;")) {
5945+
if (tok->isName() && (Token::Match(tok, "static| const| static| %type% const| %var% = %any% ;") ||
5946+
Token::Match(tok, "static| const| static| %type% const| %var% ( %any% ) ;"))) {
59415947
bool isconst = false;
5942-
for (const Token *tok2 = tok; tok2->str() != "="; tok2 = tok2->next()) {
5948+
for (const Token *tok2 = tok; (tok2->str() != "=") && (tok2->str() != "("); tok2 = tok2->next()) {
59435949
if (tok2->str() == "const") {
59445950
isconst = true;
59455951
break;
@@ -5962,7 +5968,7 @@ bool Tokenizer::simplifyKnownVariables()
59625968

59635969
const Token * const vartok = (tok->next() && tok->next()->str() == "const") ? tok->tokAt(2) : tok->next();
59645970
const Token * const valuetok = vartok->tokAt(2);
5965-
if (Token::Match(valuetok, "%bool%|%char%|%num%|%str% ;")) {
5971+
if (Token::Match(valuetok, "%bool%|%char%|%num%|%str% )| ;")) {
59665972
//check if there's not a reference usage inside the code
59675973
bool withreference = false;
59685974
for (const Token *tok2 = valuetok->tokAt(2); tok2; tok2 = tok2->next()) {

‎test/testbufferoverrun.cpp‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,7 @@ class TestBufferOverrun : public TestFixture {
121121
TEST_CASE(array_index_43); // struct with array
122122
TEST_CASE(array_index_44); // #3979
123123
TEST_CASE(array_index_45); // #4207 - calling function with variable number of parameters (...)
124+
TEST_CASE(array_index_46); // #4840 - two-statement for loop
124125
TEST_CASE(array_index_multidim);
125126
TEST_CASE(array_index_switch_in_for);
126127
TEST_CASE(array_index_for_in_for); // FP: #2634
@@ -1506,6 +1507,36 @@ class TestBufferOverrun : public TestFixture {
15061507
ASSERT_EQUALS("", errout.str());
15071508
}
15081509

1510+
// Two statement for-loop
1511+
void array_index_46() {
1512+
// #4840
1513+
check("void bufferAccessOutOfBounds2() {\n"
1514+
" char *buffer[]={\"a\",\"b\",\"c\"};\n"
1515+
" for(int i=3; i--;) {\n"
1516+
" printf(\"files(%i): %s\n\", 3-i, buffer[3-i]);\n"
1517+
" }\n"
1518+
"}");
1519+
ASSERT_EQUALS("[test.cpp:4]: (error) Array 'buffer[3]' accessed at index 3, which is out of bounds.\n", errout.str());
1520+
1521+
check("void f() {\n"
1522+
" int buffer[9];\n"
1523+
" long int i;\n"
1524+
" for(i=10; i--;) {\n"
1525+
" buffer[i] = i;\n"
1526+
" }\n"
1527+
"}");
1528+
ASSERT_EQUALS("[test.cpp:5]: (error) Buffer is accessed out of bounds: buffer\n", errout.str());
1529+
1530+
// Correct access limits -> i from 9 to 0
1531+
check("void f() {\n"
1532+
" int buffer[10];\n"
1533+
" for(unsigned long int i=10; i--;) {\n"
1534+
" buffer[i] = i;\n"
1535+
" }\n"
1536+
"}");
1537+
ASSERT_EQUALS("", errout.str());
1538+
}
1539+
15091540
void array_index_multidim() {
15101541
check("void f()\n"
15111542
"{\n"

‎test/testtokenize.cpp‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,7 @@ class TestTokenizer : public TestFixture {
333333
TEST_CASE(simplify_constants2);
334334
TEST_CASE(simplify_constants3);
335335
TEST_CASE(simplify_constants4);
336+
TEST_CASE(simplify_constants5);
336337
TEST_CASE(simplify_null);
337338
TEST_CASE(simplifyMulAndParens); // Ticket #2784 + #3184
338339

@@ -5295,6 +5296,15 @@ class TestTokenizer : public TestFixture {
52955296
ASSERT_EQUALS("x = 4 ;\ny = 50 ;", tokenizeAndStringify(code,true));
52965297
}
52975298

5299+
void simplify_constants5() {
5300+
const char code[] = "int buffer[10];\n"
5301+
"static const int NELEMS = sizeof(buffer)/sizeof(int);\n"
5302+
"static const int NELEMS2(sizeof(buffer)/sizeof(int));\n"
5303+
"x = NELEMS;\n"
5304+
"y = NELEMS2;\n";
5305+
ASSERT_EQUALS("int buffer [ 10 ] ;\n\n\nx = 10 ;\ny = 10 ;", tokenizeAndStringify(code,true));
5306+
}
5307+
52985308
void simplify_null() {
52995309
{
53005310
const char code[] =

0 commit comments

Comments
 (0)