From 9aad4fa8ca6e1bb6a6315ea596cc6383b2b1a643 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Thu, 12 Feb 2015 17:29:36 +0100 Subject: [PATCH] CheckBufferOverrun: Remove hardcoding for sprintf and rely on cfg configuration instead --- cfg/std.cfg | 3 + lib/checkbufferoverrun.cpp | 79 +++------- lib/checkbufferoverrun.h | 8 -- test/testbufferoverrun.cpp | 286 +++++++++++++------------------------ 4 files changed, 127 insertions(+), 249 deletions(-) diff --git a/cfg/std.cfg b/cfg/std.cfg index 929e03ee5..747ce8636 100644 --- a/cfg/std.cfg +++ b/cfg/std.cfg @@ -5238,6 +5238,9 @@ false + + + diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 407755766..5998101b2 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -281,7 +281,7 @@ static bool bailoutIfSwitch(const Token *tok, const unsigned int varid) } //--------------------------------------------------------------------------- -static bool checkMinSizes(const std::list &minsizes, const Token * const ftok, const std::size_t arraySize, const Token **charSizeToken) +static bool checkMinSizes(const std::list &minsizes, const Token * const ftok, const std::size_t arraySize, const Token **charSizeToken, const Settings * const settings) { if (charSizeToken) *charSizeToken = nullptr; @@ -318,9 +318,24 @@ static bool checkMinSizes(const std::list &min } break; case Library::ArgumentChecks::MinSize::STRLEN: { - const Token *strtoken = argtok->getValueTokenMaxStrLength(); - if (strtoken && Token::getStrLength(strtoken) >= arraySize) - error = true; + if (settings->library.isargformatstr(ftok,minsize->arg)) { + std::list parameters; + const std::string &formatstr(argtok->str()); + const Token *argtok2 = argtok; + while ((argtok2 = argtok2->nextArgument()) != nullptr) { + if (Token::Match(argtok2, "%num%|%str% [,)]")) + parameters.push_back(argtok2); + else + parameters.push_back(nullptr); + } + const MathLib::bigint len = CheckBufferOverrun::countSprintfLength(formatstr, parameters); + if (len > arraySize + 2U) + error = true; + } else { + const Token *strtoken = argtok->getValueTokenMaxStrLength(); + if (strtoken && Token::getStrLength(strtoken) >= arraySize) + error = true; + } } break; case Library::ArgumentChecks::MinSize::SIZEOF: @@ -347,7 +362,7 @@ void CheckBufferOverrun::checkFunctionParameter(const Token &ftok, unsigned int arraySize *= arrayInfo.num(i); const Token *charSizeToken = nullptr; - if (checkMinSizes(*minsizes, &ftok, (std::size_t)arraySize, &charSizeToken)) + if (checkMinSizes(*minsizes, &ftok, (std::size_t)arraySize, &charSizeToken, _settings)) bufferOverrunError(callstack, arrayInfo.varname()); if (charSizeToken) sizeArgumentAsCharError(charSizeToken); @@ -677,12 +692,6 @@ void CheckBufferOverrun::checkScope(const Token *tok, const std::vector 0 ? std::string("sprintf ( %varid% , %str% [,)]") : ("sprintf ( " + varnames + " , %str% [,)]"); - if (Token::Match(tok, sprintfPattern.c_str(), declarationId)) { - checkSprintfCall(tok, static_cast(total_size)); - } - // snprintf.. const std::string snprintfPattern = declarationId > 0 ? std::string("snprintf ( %varid% , %num% ,") : ("snprintf ( " + varnames + " , %num% ,"); if (Token::Match(tok, snprintfPattern.c_str(), declarationId)) { @@ -965,11 +974,6 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo } } - - if (Token::Match(tok, "sprintf ( %varid% , %str% [,)]", declarationId)) { - checkSprintfCall(tok, total_size); - } - // snprintf.. if (total_size > 0 && Token::Match(tok, "snprintf ( %varid% , %num% ,", declarationId)) { const MathLib::bigint n = MathLib::toLongNumber(tok->strAt(4)); @@ -1471,35 +1475,6 @@ MathLib::bigint CheckBufferOverrun::countSprintfLength(const std::string &input_ return (MathLib::bigint)input_string_size; } -void CheckBufferOverrun::checkSprintfCall(const Token *tok, const MathLib::bigint size) -{ - if (size == 0) - return; - - std::list parameters; - const Token* vaArg = tok->tokAt(2)->nextArgument()->nextArgument(); - while (vaArg) { - if (Token::Match(vaArg->next(), "[,)]")) { - if (vaArg->type() == Token::eString) - parameters.push_back(vaArg); - - else if (vaArg->isNumber()) - parameters.push_back(vaArg); - - else - parameters.push_back(nullptr); - } else // Parameter is more complex than just a value or variable. Ignore it for now and skip to next token. - parameters.push_back(nullptr); - - vaArg = vaArg->nextArgument(); - } - - MathLib::bigint len = countSprintfLength(tok->tokAt(2)->nextArgument()->strValue(), parameters); - if (len > size) { - bufferOverrunError(tok); - } -} - //--------------------------------------------------------------------------- @@ -1547,10 +1522,6 @@ void CheckBufferOverrun::checkBufferAllocatedWithStrlen() if (Token::Match(tok, "strcpy ( %varid% , %var% )", dstVarId) && tok->tokAt(4)->varId() == srcVarId) { bufferOverrunError(tok); - } else if (Token::Match(tok, "sprintf ( %varid% , %str% , %var% )", dstVarId) && - tok->tokAt(6)->varId() == srcVarId && - tok->strAt(4).find("%s") != std::string::npos) { - bufferOverrunError(tok); } } if (!tok) @@ -1582,7 +1553,7 @@ void CheckBufferOverrun::checkStringArgument() const std::list *minsizes = _settings->library.argminsizes(tok, argnr); if (!minsizes) continue; - if (checkMinSizes(*minsizes, tok, Token::getStrSize(strtoken), nullptr)) + if (checkMinSizes(*minsizes, tok, Token::getStrSize(strtoken), nullptr, _settings)) bufferOverrunError(argtok); } } @@ -1635,14 +1606,6 @@ void CheckBufferOverrun::checkInsecureCmdLineArgs() Token::Match(tok, "strcpy|strcat ( %name% , %varid% [", varid)) { cmdLineArgsError(tok); tok = tok->linkAt(1); - } else if (Token::Match(tok, "sprintf ( %name% , %str% , %varid% [", varid) && - tok->strAt(4).find("%s") != std::string::npos) { - cmdLineArgsError(tok); - tok = tok->linkAt(1); - } else if (Token::Match(tok, "sprintf ( %name% , %str% , * %varid%", varid) && - tok->strAt(4).find("%s") != std::string::npos) { - cmdLineArgsError(tok); - tok = tok->linkAt(1); } } } diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index 96656a3b1..14c67795e 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -82,14 +82,6 @@ public: */ static MathLib::bigint countSprintfLength(const std::string &input_string, const std::list ¶meters); - /** - * @brief %Check code that matches: "sprintf ( %varid% , %str% [,)]" when varid is not 0, - * and report found errors. - * @param tok The "sprintf" token. - * @param size The size of the buffer where sprintf is writing. - */ - void checkSprintfCall(const Token *tok, const MathLib::bigint size); - /** Check for buffer overruns - locate struct variables and check them with the .._CheckScope function */ void checkStructVariable(); diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 7ed4cd072..d41c34022 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -183,7 +183,6 @@ private: TEST_CASE(buffer_overrun_14); TEST_CASE(buffer_overrun_15); // ticket #1787 TEST_CASE(buffer_overrun_16); - TEST_CASE(buffer_overrun_17); // ticket #2548 TEST_CASE(buffer_overrun_18); // ticket #2576 - for, calculation with loop variable TEST_CASE(buffer_overrun_19); // #2597 - class member with unknown type TEST_CASE(buffer_overrun_20); // #2986 (segmentation fault) @@ -210,17 +209,6 @@ private: TEST_CASE(pointer_out_of_bounds_2); TEST_CASE(pointer_out_of_bounds_sub); - TEST_CASE(sprintf1); - TEST_CASE(sprintf2); - TEST_CASE(sprintf3); - TEST_CASE(sprintf4); - TEST_CASE(sprintf5); - TEST_CASE(sprintf6); - TEST_CASE(sprintf7); - TEST_CASE(sprintf8); - TEST_CASE(sprintf9); - TEST_CASE(sprintf10); - TEST_CASE(snprintf1); TEST_CASE(snprintf2); TEST_CASE(snprintf4); @@ -258,6 +246,7 @@ private: TEST_CASE(counter_test); TEST_CASE(minsize_argvalue); TEST_CASE(minsize_sizeof); + TEST_CASE(minsize_strlen); TEST_CASE(minsize_mul); TEST_CASE(unknownType); @@ -2343,7 +2332,7 @@ private: // ticket #900 check("void f() {\n" " char *a = new char(30);\n" - " sprintf(a, \"%s\", \"b\");\n" + " strcpy(a, \"b\");\n" " delete a;\n" "}"); ASSERT_EQUALS("[test.cpp:3]: (error) Buffer is accessed out of bounds.\n", errout.str()); @@ -2419,7 +2408,7 @@ private: checkstd("void f(char *a) {\n" " char *b = malloc(strlen(a));\n" - " sprintf(b, \"%s\", a);\n" + " strcpy(b, a);\n" " return b;\n" "}"); ASSERT_EQUALS("[test.cpp:3]: (error) Buffer is accessed out of bounds.\n", errout.str()); @@ -2427,14 +2416,14 @@ private: void buffer_overrun_15() { // ticket #1787 check("class A : public B {\n" - " char val[12];\n" + " char val[2];\n" " void f(int i, int ii);\n" "};\n" "void A::f(int i, int ii)\n" "{\n" - " sprintf(val, \"drive_%d_partition_%d_size\", i, ii) ;\n" + " strcpy(val, \"ab\") ;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (error) Buffer is accessed out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (error) Buffer is accessed out of bounds: val\n", errout.str()); } void buffer_overrun_16() { @@ -2459,14 +2448,6 @@ private: ASSERT_EQUALS("", errout.str()); } - void buffer_overrun_17() { // ticket #2548 - check("void f() {\n" - " char t[8];\n" - " sprintf(t, \"%s\", \"foo bar\");\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) Buffer is accessed out of bounds.\n", errout.str()); - } - void buffer_overrun_18() { // ticket #2576 check("class A {\n" " void foo();\n" @@ -2912,135 +2893,6 @@ private: ASSERT_EQUALS("[test.cpp:4]: (portability) Undefined behaviour, when 'i' is -20 the pointer arithmetic 'x-i' is out of bounds.\n", errout.str()); } - void sprintf1() { - check("void f()\n" - "{\n" - " char str[3];\n" - " sprintf(str, \"%s\", \"abc\");\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds.\n", errout.str()); - - check("void f()\n" - "{\n" - " char * c = new char[10];\n" - " sprintf(c, \"%s\", \"/usr/LongLongLongLongUserName/bin/LongLongApplicationName\");\n" - " delete [] c;\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds.\n", errout.str()); - } - - void sprintf2() { - check("int getnumber();\n" - "void f()\n" - "{\n" - " char str[5];\n" - " sprintf(str, \"%d: %s\", getnumber(), \"abcde\");\n" - "}"); - ASSERT_EQUALS("[test.cpp:5]: (error) Buffer is accessed out of bounds.\n", errout.str()); - } - - void sprintf3() { - check("void f()\n" - "{\n" - " char str[3];\n" - " sprintf(str, \"test\");\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds.\n", errout.str()); - - check("void f()\n" - "{\n" - " char str[5];\n" - " sprintf(str, \"test%s\", \"\");\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void sprintf4() { - // ticket #690 - check("void f()\n" - "{\n" - " char a[3];\n" - " sprintf(a, \"%02ld\", 99);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void sprintf5() { - // ticket #729 - check("void f(int condition)\n" - "{\n" - " char buf[3];\n" - " sprintf(buf, \"%s\", condition ? \"11\" : \"22\");\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void sprintf6() { - check("void f(int condition)\n" - "{\n" - " char buf[3];\n" - " sprintf(buf, \"%s\", condition ? \"11\" : \"222\");\n" - "}"); - TODO_ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds.\n", "", errout.str()); - } - - void sprintf7() { - check("struct Foo { char a[1]; };\n" - "void f()\n" - "{\n" - " struct Foo x;\n" - " sprintf(x.a, \"aa\");\n" - "}"); - ASSERT_EQUALS("[test.cpp:5]: (error) Buffer is accessed out of bounds.\n", errout.str()); - - // This is out of bounds if 'sizeof(ABC)' is 1 (No padding) - check("struct Foo { char a[1]; };\n" - "void f()\n" - "{\n" - " struct Foo *x = malloc(sizeof(Foo));\n" - " sprintf(x.a, \"aa\");\n" - " free(x);\n" - "}"); - TODO_ASSERT_EQUALS("error", "", errout.str()); - - check("struct Foo { char a[1]; };\n" - "void f()\n" - "{\n" - " struct Foo *x = malloc(sizeof(Foo) + 10);\n" - " sprintf(x.a, \"aa\");\n" - " free(x);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void sprintf8() { - check("struct Foo { char a[3]; };\n" - "void f()\n" - "{\n" - " struct Foo x;\n" - " sprintf(x.a, \"aa\");\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void sprintf9() { - check("void f()\n" - "{\n" - " gchar str[3];\n" - " sprintf(str, \"1\");\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void sprintf10() { - check("void f()\n" - "{\n" - " TString str = \"\";\n" - " sprintf(str, \"1\");\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - void snprintf1() { check("void f()\n" "{\n" @@ -3501,9 +3353,11 @@ private: ASSERT_EQUALS(4, CheckBufferOverrun::countSprintfLength("%%%%%d", unknownParameter)); Token strTok(0); - strTok.str("\"12345\""); std::list stringAsParameter; stringAsParameter.push_back(&strTok); + strTok.str("\"\""); + ASSERT_EQUALS(4, CheckBufferOverrun::countSprintfLength("str%s", stringAsParameter)); + strTok.str("\"12345\""); ASSERT_EQUALS(9, CheckBufferOverrun::countSprintfLength("str%s", stringAsParameter)); ASSERT_EQUALS(6, CheckBufferOverrun::countSprintfLength("%-4s", stringAsParameter)); ASSERT_EQUALS(6, CheckBufferOverrun::countSprintfLength("%-5s", stringAsParameter)); @@ -3552,7 +3406,6 @@ private: multipleParams.push_back(&numTok); ASSERT_EQUALS(15, CheckBufferOverrun::countSprintfLength("str%s%d%d", multipleParams)); ASSERT_EQUALS(26, CheckBufferOverrun::countSprintfLength("str%-6s%08ld%08ld", multipleParams)); - } void minsize_argvalue() { @@ -3630,6 +3483,7 @@ private: doc.Parse(xmldata, sizeof(xmldata)); settings.library.load(doc); + // strncpy... check("void f() {\n" " char c[7];\n" " strncpy(c, \"hello\", 7);\n" @@ -3660,14 +3514,99 @@ private: "}", settings); ASSERT_EQUALS("", errout.str()); - // #3168 - check("void a(char *p) { strncpy(p,\"hello world!\",10); }\n" + check("void a(char *p) { strncpy(p,\"hello world!\",10); }\n" // #3168 "void b() {\n" " char buf[5];\n" " a(buf);" "}", settings); ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:1]: (error) Buffer is accessed out of bounds: buf\n", errout.str()); } + void minsize_strlen() { + Settings settings; + const char xmldata[] = "\n" + "\n" + " \n" + " false\n" + " \n" + " \n" + " \n" + " \n" + " \n" + " \n" + " \n" + " \n" + ""; + tinyxml2::XMLDocument doc; + doc.Parse(xmldata, sizeof(xmldata)); + settings.library.load(doc); + + // formatstr.. + check("void f() {\n" + " char str[3];\n" + " mysprintf(str, \"test\");\n" + "}", settings); + ASSERT_EQUALS("[test.cpp:3]: (error) Buffer is accessed out of bounds: str\n", errout.str()); + + check("void f() {\n" + " char str[5];\n" + " mysprintf(str, \"%s\", \"abcde\");\n" + "}", settings); + ASSERT_EQUALS("[test.cpp:3]: (error) Buffer is accessed out of bounds: str\n", errout.str()); + + check("int getnumber();\n" + "void f()\n" + "{\n" + " char str[5];\n" + " mysprintf(str, \"%d: %s\", getnumber(), \"abcde\");\n" + "}", settings); + ASSERT_EQUALS("[test.cpp:5]: (error) Buffer is accessed out of bounds: str\n", errout.str()); + + check("void f() {\n" + " char str[5];\n" + " mysprintf(str, \"test%s\", \"\");\n" + "}", settings); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " char *str = new char[5];\n" + " mysprintf(str, \"abcde\");\n" + "}", settings); + ASSERT_EQUALS("[test.cpp:3]: (error) Buffer is accessed out of bounds.\n", errout.str()); + + check("void f(int condition) {\n" + " char str[5];\n" + " mysprintf(str, \"test%s\", condition ? \"12\" : \"34\");\n" + "}", settings); + ASSERT_EQUALS("", errout.str()); + + check("void f(int condition) {\n" + " char str[5];\n" + " mysprintf(str, \"test%s\", condition ? \"12\" : \"345\");\n" + "}", settings); + TODO_ASSERT_EQUALS("error", "", errout.str()); + + check("struct Foo { char a[1]; };\n" + "void f() {\n" + " struct Foo x;\n" + " mysprintf(x.a, \"aa\");\n" + "}", settings); + ASSERT_EQUALS("[test.cpp:4]: (error) Buffer is accessed out of bounds: x.a\n", errout.str()); + + // This is out of bounds if 'sizeof(ABC)' is 1 (No padding) + check("struct Foo { char a[1]; };\n" + "void f() {\n" + " struct Foo *x = malloc(sizeof(Foo));\n" + " mysprintf(x.a, \"aa\");\n" + "}", settings); + TODO_ASSERT_EQUALS("error", "", errout.str()); + + check("struct Foo { char a[1]; };\n" + "void f() {\n" + " struct Foo *x = malloc(sizeof(Foo) + 10);\n" + " mysprintf(x.a, \"aa\");\n" + "}", settings); + ASSERT_EQUALS("", errout.str()); + } void minsize_mul() { Settings settings; @@ -3907,12 +3846,12 @@ private: } void cmdLineArgs1() { + check("int main(int argc, char* argv[])\n" "{\n" " char prog[10];\n" " strcpy(prog, argv[0]);\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer overrun possible for long command line arguments.\n", errout.str()); check("int main(int argc, char* argv[])\n" @@ -3920,15 +3859,13 @@ private: " char prog[10] = {'\\0'};\n" " strcat(prog, argv[0]);\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer overrun possible for long command line arguments.\n", errout.str()); check("int main(int argc, char* argv[])\n" "{\n" " char prog[10];\n" - " sprintf(prog, \"%s\", argv[0]);\n" + " strcpy(prog, argv[0]);\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer overrun possible for long command line arguments.\n", errout.str()); check("int main(int argc, char **argv, char **envp)\n" @@ -3936,7 +3873,6 @@ private: " char prog[10];\n" " strcpy(prog, argv[0]);\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer overrun possible for long command line arguments.\n", errout.str()); check("int main(int argc, char **argv, char **envp)\n" @@ -3944,15 +3880,13 @@ private: " char prog[10] = {'\\0'};\n" " strcat(prog, argv[0]);\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer overrun possible for long command line arguments.\n", errout.str()); check("int main(int argc, char **argv, char **envp)\n" "{\n" " char prog[10];\n" - " sprintf(prog, \"%s\", argv[0]);\n" + " strcpy(prog, argv[0]);\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer overrun possible for long command line arguments.\n", errout.str()); check("int main(int argc, char **options)\n" @@ -3960,7 +3894,6 @@ private: " char prog[10];\n" " strcpy(prog, options[0]);\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer overrun possible for long command line arguments.\n", errout.str()); check("int main(int argc, char **options)\n" @@ -3968,15 +3901,13 @@ private: " char prog[10] = {'\\0'};\n" " strcat(prog, options[0]);\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer overrun possible for long command line arguments.\n", errout.str()); check("int main(int argc, char **options)\n" "{\n" " char prog[10];\n" - " sprintf(prog, \"%s\", *options);\n" + " strcpy(prog, *options);\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Buffer overrun possible for long command line arguments.\n", errout.str()); check("int main(int argc, char **argv, char **envp)\n" @@ -3985,7 +3916,6 @@ private: " if (strlen(argv[0]) < 10)\n" " strcpy(prog, argv[0]);\n" "}"); - ASSERT_EQUALS("", errout.str()); check("int main(int argc, char **argv, char **envp)\n" @@ -3994,15 +3924,6 @@ private: " if (10 > strlen(argv[0]))\n" " strcat(prog, argv[0]);\n" "}"); - - ASSERT_EQUALS("", errout.str()); - - check("int main(int argc, char **argv, char **envp)\n" - "{\n" - " char prog[10];\n" - " sprintf(prog, \"%p\", argv[0]);\n" - "}"); - ASSERT_EQUALS("", errout.str()); check("int main(int argc, char **argv, char **envp)\n" @@ -4011,14 +3932,13 @@ private: " argv[0][0] = '\\0';\n" " strcpy(prog, argv[0]);\n" "}"); - ASSERT_EQUALS("", errout.str()); // #5835 checkstd("int main(int argc, char* argv[]) {\n" " char prog[10];\n" - " sprintf(prog, \"%s\", argv[0]);\n" - " sprintf(prog, \"%s\", argv[0]);\n" + " strcpy(prog, argv[0]);\n" + " strcpy(prog, argv[0]);\n" "}"); ASSERT_EQUALS("[test.cpp:3]: (error) Buffer overrun possible for long command line arguments.\n" "[test.cpp:4]: (error) Buffer overrun possible for long command line arguments.\n", errout.str());