From 50688b28fd87aaeeda27b6505a883ff39fa43cc1 Mon Sep 17 00:00:00 2001 From: Robert Reif Date: Sun, 4 Sep 2011 19:54:57 -0400 Subject: [PATCH] fix #2889 (false negative: buffer access out of bounds on local struct member) --- lib/checkbufferoverrun.cpp | 41 +++++++++++++++++++++--- test/testbufferoverrun.cpp | 65 ++++++++++++++++++++++++++++++++------ 2 files changed, 93 insertions(+), 13 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 5d4102ac8..92ca5c500 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -1141,17 +1141,50 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo if (totalElements == 0) continue; + // get the full variable name + std::string varname; + const Token *tok2 = tok->previous(); + while (tok2 && Token::Match(tok2->tokAt(-1), "%var% .")) + { + varname += (tok2->strAt(-1) + "."); + tok2 = tok2->tokAt(-2); + } + // just taking the address? - const bool addr(Token::Match(tok->previous(), "[.&]") || - Token::simpleMatch(tok->tokAt(-2), "& (")); + const bool addr(Token::Match(tok2, "&") || + Token::simpleMatch(tok2->tokAt(-1), "& (")); + + // is this a member variable? + if (varname.size() && tok2->next()->varId()) + { + // is this variable a pointer? + const Variable *var1 =_tokenizer->getSymbolDatabase()->getVariableFromVarId(tok2->next()->varId()); + if (var1 && var1->typeEndToken()->str() == "*") + { + // is variable public struct member? + if (var->scope()->type == Scope::eStruct && var->isPublic()) + { + // last member of a struct with array size of 0 or 1 could be a variable struct + if (var->dimensions().size() == 1 && var->dimension(0) < 2 && + var->index() == (var->scope()->varlist.size() - 1)) + { + continue; + } + } + } + } + + // taking address of 1 past end? + if (addr && totalIndex == totalElements) + continue; // Is totalIndex in bounds? - if (totalIndex > totalElements || (!addr && totalIndex == totalElements)) + if (totalIndex > totalElements) { arrayIndexOutOfBoundsError(tok, arrayInfo, indexes); } // Is any array index out of bounds? - else if (totalIndex != totalElements) + else { // check each index for overflow for (unsigned int i = 0; i < indexes.size(); ++i) diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 03776f43c..6faa8b5db 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -111,6 +111,7 @@ private: TEST_CASE(array_index_32); 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_multidim); TEST_CASE(array_index_switch_in_for); TEST_CASE(array_index_for_in_for); // FP: #2634 @@ -512,7 +513,9 @@ private: "}\n"); ASSERT_EQUALS("", errout.str()); - // This is not out of bounds because it is a variable length array and the index is within the memory allocated + // This is not out of bounds because it is a variable length array + // and the index is within the memory allocated. + /** @todo this works by accident because we ignore any access to this array */ check("struct ABC\n" "{\n" " char str[1];\n" @@ -523,9 +526,10 @@ private: " struct ABC* x = (struct ABC *)malloc(sizeof(struct ABC) + 10);\n" " x->str[10] = 0;" "}\n"); - TODO_ASSERT_EQUALS("", "[test.cpp:9]: (error) Array 'str[1]' index 10 out of bounds\n", errout.str()); + ASSERT_EQUALS("", errout.str()); - // This is out of bounds because it is outside the memory allocated + // This is out of bounds because it is outside the memory allocated. + /** @todo this doesn't work because we ignore any access to this array */ check("struct ABC\n" "{\n" " char str[1];\n" @@ -536,9 +540,10 @@ private: " struct ABC* x = (struct ABC *)malloc(sizeof(struct ABC) + 10);\n" " x->str[11] = 0;" "}\n"); - ASSERT_EQUALS("[test.cpp:9]: (error) Array 'str[1]' index 11 out of bounds\n", errout.str()); + TODO_ASSERT_EQUALS("[test.cpp:9]: (error) Array 'str[1]' index 11 out of bounds\n", "", errout.str()); // This is out of bounds because it is outside the memory allocated + /** @todo this doesn't work because we ignore any access to this array */ check("struct ABC\n" "{\n" " char str[1];\n" @@ -562,7 +567,7 @@ private: " struct ABC x;\n" " x.str[1] = 0;" "}\n"); - TODO_ASSERT_EQUALS("[test.cpp:9]: (error) Array 'str[1]' index 1 out of bounds\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:9]: (error) Array 'str[1]' index 1 out of bounds\n", errout.str()); check("struct foo\n" "{\n" @@ -1172,12 +1177,54 @@ private: " ptest->b[10][2] = 4;\n" " ptest->b[0][19] = 4;\n" "}\n"); - ASSERT_EQUALS("[test.cpp:10]: (error) Array 'b[10][5]' index b[10][2] out of bounds\n" + ASSERT_EQUALS("[test.cpp:9]: (error) Array 'a[10]' index 10 out of bounds\n" + "[test.cpp:14]: (error) Array 'a[10]' index 10 out of bounds\n" + "[test.cpp:10]: (error) Array 'b[10][5]' index b[10][2] out of bounds\n" "[test.cpp:11]: (error) Array 'b[10][5]' index b[0][19] out of bounds\n" "[test.cpp:15]: (error) Array 'b[10][5]' index b[10][2] out of bounds\n" - "[test.cpp:16]: (error) Array 'b[10][5]' index b[0][19] out of bounds\n" - "[test.cpp:9]: (error) Array 'a[10]' index 10 out of bounds\n" - "[test.cpp:14]: (error) Array 'a[10]' index 10 out of bounds\n", errout.str()); + "[test.cpp:16]: (error) Array 'b[10][5]' index b[0][19] out of bounds\n", errout.str()); + + check("struct TEST\n" + "{\n" + " char a[10][5];\n" + "};\n" + "void foo()\n" + "{\n" + " TEST test;\n" + " test.a[9][5] = 4;\n" + " test.a[0][50] = 4;\n" + " TEST *ptest;\n" + " ptest = &test;\n" + " ptest->a[9][5] = 4;\n" + " ptest->a[0][50] = 4;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:8]: (error) Array 'a[10][5]' index a[9][5] out of bounds\n" + "[test.cpp:9]: (error) Array 'a[10][5]' index a[0][50] out of bounds\n" + "[test.cpp:12]: (error) Array 'a[10][5]' index a[9][5] out of bounds\n" + "[test.cpp:13]: (error) Array 'a[10][5]' index a[0][50] out of bounds\n", errout.str()); + } + + void array_index_35() // ticket #2889 + { + check("void f() {\n" + " struct Struct { unsigned m_Var[1]; } s;\n" + " s.m_Var[1] = 1;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (error) Array 'm_Var[1]' index 1 out of bounds\n", errout.str()); + + check("struct Struct { unsigned m_Var[1]; };\n" + "void f() {\n" + " struct Struct s;\n" + " s.m_Var[1] = 1;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'm_Var[1]' index 1 out of bounds\n", errout.str()); + + check("struct Struct { unsigned m_Var[1]; };\n" + "void f() {\n" + " struct Struct * s = calloc(40);\n" + " s->m_Var[1] = 1;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); } void array_index_multidim()