From 59086fa59920b65e1e185f6f0bcf51b3a035dae0 Mon Sep 17 00:00:00 2001 From: Zachary Blair Date: Fri, 28 May 2010 22:51:28 -0700 Subject: [PATCH] Fixed #818 (Detect sprintf buffer overrun with struct members) --- lib/checkbufferoverrun.cpp | 55 ++++++++++++++---------- test/testbufferoverrun.cpp | 88 +++++++++++++++++++++++++++++++++++++- 2 files changed, 118 insertions(+), 25 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index b15487b2e..87690e7e6 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -671,38 +671,38 @@ void CheckBufferOverrun::checkScope(const Token *tok, const std::vector 0 && Token::Match(tok, "strcat ( %varid% , %str% ) ;", varid)) + const std::string strcatPattern = varid > 0 ? "strcat ( %varid% , %str% ) ;" : ("strcat ( " + varnames + " , %str% ) ;"); + if (Token::Match(tok, strcatPattern.c_str(), varid)) { size_t charactersAppend = 0; const Token *tok2 = tok; - while (tok2 && Token::Match(tok2, "strcat ( %varid% , %str% ) ;", varid)) + while (tok2 && Token::Match(tok2, strcatPattern.c_str(), varid)) { - charactersAppend += Token::getStrLength(tok2->tokAt(4)); + charactersAppend += Token::getStrLength(tok2->tokAt(4 + varc)); if (charactersAppend >= static_cast(total_size)) { bufferOverrun(tok2); break; } - tok2 = tok2->tokAt(7); + tok2 = tok2->tokAt(7 + varc); } } - // sprintf / snprintf.. - if (varid > 0) + // sprintf.. + const std::string sprintfPattern = varid > 0 ? "sprintf ( %varid% , %str% [,)]" : ("sprintf ( " + varnames + " , %str% [,)]"); + if (Token::Match(tok, sprintfPattern.c_str(), varid)) { - if (Token::Match(tok, "sprintf ( %varid% , %str% [,)]", varid)) - { - checkSprintfCall(tok, total_size); - } + checkSprintfCall(tok, total_size); + } - // snprintf.. - if (Token::Match(tok, "snprintf ( %varid% , %num% ,", varid)) - { - int n = MathLib::toLongNumber(tok->strAt(4)); - if (n > total_size) - outOfBounds(tok->tokAt(4), "snprintf size"); - } + // snprintf.. + const std::string snprintfPattern = varid > 0 ? "snprintf ( %varid% , %num% ," : ("snprintf ( " + varnames + " , %num% ,"); + if (Token::Match(tok, snprintfPattern.c_str(), varid)) + { + int n = MathLib::toLongNumber(tok->strAt(4 + varc)); + if (n > total_size) + outOfBounds(tok->tokAt(4 + varc), "snprintf size"); } // Function calls not handled @@ -1344,14 +1344,23 @@ int CheckBufferOverrun::countSprintfLength(const std::string &input_string, cons return input_string_size; } - void CheckBufferOverrun::checkSprintfCall(const Token *tok, int size) { - std::list parameters; - if (tok->tokAt(5)->str() == ",") + const Token *end = tok->next()->link(); + + // Count the number of tokens in the buffer variable's name + int varc = 0; + for (const Token *tok1 = tok->tokAt(3); tok1 != end; tok1 = tok1->next()) { - const Token *end = tok->next()->link(); - for (const Token *tok2 = tok->tokAt(5); tok2 && tok2 != end; tok2 = tok2->next()) + if (tok1->str() == ",") + break; + ++ varc; + } + + std::list parameters; + if (tok->tokAt(5 + varc)->str() == ",") + { + for (const Token *tok2 = tok->tokAt(5 + varc); tok2 && tok2 != end; tok2 = tok2->next()) { if (Token::Match(tok2, ", %any% [,)]")) { @@ -1396,7 +1405,7 @@ void CheckBufferOverrun::checkSprintfCall(const Token *tok, int size) } } - int len = countSprintfLength(tok->tokAt(4)->strValue(), parameters); + int len = countSprintfLength(tok->tokAt(4 + varc)->strValue(), parameters); if (len > size) { bufferOverrun(tok); diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index bee9b7602..12e210431 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -130,14 +130,22 @@ private: TEST_CASE(sprintf4); TEST_CASE(sprintf5); TEST_CASE(sprintf6); + TEST_CASE(sprintf7); + TEST_CASE(sprintf8); TEST_CASE(snprintf1); TEST_CASE(snprintf2); TEST_CASE(snprintf3); TEST_CASE(snprintf4); + TEST_CASE(snprintf5); + TEST_CASE(snprintf6); TEST_CASE(strncat1); TEST_CASE(strncat2); + TEST_CASE(strncat3); + + TEST_CASE(strcat1); + TEST_CASE(strcat2); TEST_CASE(memfunc); // memchr/memset/memcpy @@ -1677,6 +1685,28 @@ private: ASSERT_EQUALS("", errout.str()); // catch changes TODO_ASSERT_EQUALS("[test.cpp:4]: (error) Buffer access 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" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (error) Buffer access out-of-bounds\n", errout.str()); + } + + void sprintf8() + { + check("struct Foo { char a[3]; };\n" + "void f()\n" + "{\n" + " struct Foo x;\n" + " sprintf(x.a, \"aa\");\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } void snprintf1() { @@ -1718,8 +1748,27 @@ private: ASSERT_EQUALS("", errout.str()); } - - + void snprintf5() + { + check("struct Foo { char a[1]; };\n" + "void f()\n" + "{\n" + " struct Foo x;\n" + " snprintf(x.a, 2, \"aa\");\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (error) snprintf size is out of bounds\n", errout.str()); + } + + void snprintf6() + { + check("struct Foo { char a[3]; };\n" + "void f()\n" + "{\n" + " struct Foo x;\n" + " snprintf(x.a, 2, \"aa\");\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } void strncat1() { @@ -1742,6 +1791,41 @@ private: ASSERT_EQUALS("[test.cpp:4]: (style) Dangerous usage of strncat. Tip: the 3rd parameter means maximum number of characters to append\n", errout.str()); } + void strncat3() + { + check("struct Foo { char a[4]; };\n" + "void f(char *a)\n" + "{\n" + " struct Foo x;\n" + " strncat(x.a, a, 5);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (error) Buffer access out-of-bounds: x.a\n", errout.str()); + } + + + void strcat1() + { + check("struct Foo { char a[4]; };\n" + "void f()\n" + "{\n" + " struct Foo x;\n" + " strcat(x.a, \"aa\");\n" + " strcat(x.a, \"aa\");\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:6]: (error) Buffer access out-of-bounds\n", errout.str()); + } + + void strcat2() + { + check("struct Foo { char a[5]; };\n" + "void f()\n" + "{\n" + " struct Foo x;\n" + " strcat(x.a, \"aa\");\n" + " strcat(x.a, \"aa\");\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } // memchr/memset/memcpy/etc