diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index c86f08abd..8a2e2293e 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -1566,18 +1566,17 @@ void CheckBufferOverrun::checkStructVariable() else continue; - // Skip array with only 0/1 elements because those are - // often overrun intentionally - // is variable public struct member? + // 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 struct + // 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 array + // 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% ) ;"))) && @@ -1591,15 +1590,29 @@ void CheckBufferOverrun::checkStructVariable() else size = MathLib::toLongNumber(tok3->strAt(8)); - if (size != 100) // magic number for size of class or struct + // 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 { - /** @todo false negatives: only true if dynamically allocated with size larger that struct */ - /** @todo false negatives: calculate real array size based on allocated size */ - continue; + // 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 dynamically sized struct + // size unknown so assume it is a variable sized structure else continue; } diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index 23d5aed95..140954c1e 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -165,6 +165,10 @@ public: { return _num[index]; } + void num(size_t index, MathLib::bigint number) + { + _num[index] = number; + } /** size of each element */ MathLib::bigint element_size() const diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index edec4ecb6..b35b937e2 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -545,7 +545,7 @@ private: ASSERT_EQUALS("", 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 */ + /** @todo this doesn't work because of a bug in sizeof(struct) */ check("struct ABC\n" "{\n" " char str[1];\n" @@ -558,8 +558,21 @@ private: "}\n"); 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. + check("struct ABC\n" + "{\n" + " char str[1];\n" + "};\n" + "\n" + "static void f()\n" + "{\n" + " struct ABC* x = (struct ABC *)malloc(sizeof(ABC) + 10);\n" + " x->str[11] = 0;" + "}\n"); + ASSERT_EQUALS("[test.cpp:9]: (error) Array 'str[11]' 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 */ + /** @todo this doesn't work because of a bug in sizeof(struct) */ check("struct ABC\n" "{\n" " char str[1];\n" @@ -572,6 +585,19 @@ private: "}\n"); TODO_ASSERT_EQUALS("[test.cpp:9]: (error) Array 'str[1]' index 1 out of bounds\n", "", errout.str()); + // This is out of bounds because it is outside the memory allocated + check("struct ABC\n" + "{\n" + " char str[1];\n" + "};\n" + "\n" + "static void f()\n" + "{\n" + " struct ABC* x = (struct ABC *)malloc(sizeof(ABC));\n" + " x->str[1] = 0;" + "}\n"); + ASSERT_EQUALS("[test.cpp:9]: (error) Array 'str[1]' index 1 out of bounds\n", errout.str()); + // This is out of bounds because it is not a variable array check("struct ABC\n" "{\n"