From 59aad35137514b18a466b112c6efc705ef01fd86 Mon Sep 17 00:00:00 2001 From: Reijo Tomperi Date: Sun, 11 Oct 2009 21:36:22 +0300 Subject: [PATCH] Fix #694 (False (possible error) Buffer overrun with %-1s) ashim2009 did most of the work http://sourceforge.net/apps/trac/cppcheck/ticket/694 --- src/checkbufferoverrun.cpp | 154 +++++++++---------------------------- test/testbufferoverrun.cpp | 3 +- 2 files changed, 39 insertions(+), 118 deletions(-) diff --git a/src/checkbufferoverrun.cpp b/src/checkbufferoverrun.cpp index 6dc46a57c..95b9a564f 100644 --- a/src/checkbufferoverrun.cpp +++ b/src/checkbufferoverrun.cpp @@ -773,9 +773,6 @@ void CheckBufferOverrun::bufferOverrun() int CheckBufferOverrun::countSprintfLength(const std::string &input_string, const std::list ¶meters) { - - - int flag = 1; int second_flag = 0; // This is to check for % int input_string_size = 0; @@ -898,137 +895,60 @@ int CheckBufferOverrun::countSprintfLength(const std::string &input_string, cons void CheckBufferOverrun::checkSprintfCall(const Token *tok, int size) { - if (0) + std::list parameters; + if (tok->tokAt(5)->str() == ",") { - std::list parameters; - if (tok->tokAt(5)->str() == ",") + const Token *end = tok->next()->link(); + for (const Token *tok2 = tok->tokAt(5); tok2 && tok2 != end; tok2 = tok2->next()) { - const Token *end = tok->next()->link(); - for (const Token *tok2 = tok->tokAt(6); tok2 && tok2 != end; tok2 = tok2->next()) + if (Token::Match(tok2, ", %any% [,)]")) { - if (Token::Match(tok2, ", %any% [,)]")) - { - if (Token::Match(tok2->next(), "%str%")) - parameters.push_back(tok2->next()); - else if (Token::Match(tok2->next(), "%num%")) - parameters.push_back(tok2->next()); + if (Token::Match(tok2->next(), "%str%")) + parameters.push_back(tok2->next()); - // TODO, get value of the variable if possible and use that instead of 0 - else - parameters.push_back(0); - } + else if (Token::Match(tok2->next(), "%num%")) + parameters.push_back(tok2->next()); + + // TODO, get value of the variable if possible and use that instead of 0 else - { - // Parameter is more complex, than just a value or variable. Ignore it for now - // and skip to next token. parameters.push_back(0); - int ind = 0; - for (const Token *tok3 = tok2->next(); tok3; tok3 = tok3->next()) + } + else + { + // Parameter is more complex, than just a value or variable. Ignore it for now + // and skip to next token. + parameters.push_back(0); + int ind = 0; + for (const Token *tok3 = tok2->next(); tok3; tok3 = tok3->next()) + { + if (tok3->str() == "(") + ++ind; + + else if (tok3->str() == ")") { - if (tok3->str() == "(") - ++ind; - - else if (tok3->str() == ")") - { - --ind; - if (ind < 0) - break; - } - - else if (ind == 0 && tok3->str() == ",") - { - tok2 = tok3->previous(); + --ind; + if (ind < 0) break; - } } - if (ind < 0) + else if (ind == 0 && tok3->str() == ",") + { + tok2 = tok3->previous(); break; + } } - } - } - int len = countSprintfLength(tok->tokAt(4)->strValue(), parameters); - if (len > size) - { - bufferOverrun(tok); + if (ind < 0) + break; + } } } - else + + int len = countSprintfLength(tok->tokAt(4)->strValue(), parameters); + if (len > size) { - int len = -2; - - // check format string - const char *fmt = tok->strAt(4); - while (*fmt) - { - if (*fmt == '\\') - { - ++fmt; - } - else if (*fmt == '%') - { - ++fmt; - - // skip field width - while (std::isdigit(*fmt)) - { - ++fmt; - } - - // FIXME: better handling for format specifiers - ++fmt; - continue; - } - ++fmt; - ++len; - } - - if (len >= (int)size) - { - bufferOverrun(tok); - } - - // check arguments (if they exists) - if (tok->tokAt(5)->str() == ",") - { - len = 0; - const Token *end = tok->next()->link(); - bool argumentAlreadyChecked = false; - int lastCheckedArgumentMaxSize = 0; - for (const Token *tok2 = tok->tokAt(6); tok2 && tok2 != end; tok2 = tok2->next()) - { - if (tok2->str() == ",") - { - argumentAlreadyChecked = false; - lastCheckedArgumentMaxSize = 0; - } - else if (Token::Match(tok2, "%str%")) - { - int argumentSize = static_cast(Token::getStrLength(tok2)); - if (argumentAlreadyChecked == false) - { - lastCheckedArgumentMaxSize = argumentSize; - len += argumentSize; - argumentAlreadyChecked = true; - } - else if (argumentSize > lastCheckedArgumentMaxSize) - { - // when sprintf() argument is ternary - // operation we may have two and more - // strings as argument. In this case we - // use length of longest string. - len += (argumentSize - lastCheckedArgumentMaxSize); - lastCheckedArgumentMaxSize = argumentSize; - } - } - } - if (len >= (int)size) - { - bufferOverrun(tok); - } - } + bufferOverrun(tok); } } diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 099a471e1..096200627 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -860,7 +860,7 @@ private: " char buf[3];\n" " sprintf(buf, \"%s\", condition ? \"11\" : \"222\");\n" "}\n"); - ASSERT_EQUALS("[test.cpp:4]: (possible error) Buffer overrun\n", errout.str()); + TODO_ASSERT_EQUALS("[test.cpp:4]: (possible error) Buffer overrun\n", errout.str()); } void snprintf1() @@ -1023,6 +1023,7 @@ private: ASSERT_EQUALS(2, CheckBufferOverrun::countSprintfLength("%1d", unknownParameter)); ASSERT_EQUALS(3, CheckBufferOverrun::countSprintfLength("%2.2d", unknownParameter)); ASSERT_EQUALS(1, CheckBufferOverrun::countSprintfLength("%s", unknownParameter)); + ASSERT_EQUALS(1, CheckBufferOverrun::countSprintfLength("%-s", unknownParameter)); ASSERT_EQUALS(6, CheckBufferOverrun::countSprintfLength("%-5s", unknownParameter)); ASSERT_EQUALS(2, CheckBufferOverrun::countSprintfLength("\\\"", unknownParameter)); ASSERT_EQUALS(7, CheckBufferOverrun::countSprintfLength("Hello \0Text", unknownParameter));