From 0fd334911a081cd818d16f3986e64e077ceadfad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Sun, 6 Jul 2014 08:41:39 +0200 Subject: [PATCH] Fixed #5257 (Check memcpy size for string literals) --- lib/checkbufferoverrun.cpp | 125 +++++++++++++++++++++++++------------ lib/checkbufferoverrun.h | 6 +- test/testbufferoverrun.cpp | 6 ++ 3 files changed, 93 insertions(+), 44 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 4c9f5e920..900eb2702 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -264,11 +264,62 @@ 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) +{ + if (charSizeToken) + *charSizeToken = nullptr; + + if (minsizes.empty()) + return false; + + // All conditions must be true + bool error = true; + for (std::list::const_iterator minsize = minsizes.begin(); minsize != minsizes.end(); ++minsize) { + if (!error) + return false; + error = false; + const Token *argtok = ftok->tokAt(2); + for (int argnum = 1; argtok && argnum < minsize->arg; argnum++) + argtok = argtok->nextArgument(); + if (!argtok) + return false; + switch (minsize->type) { + case Library::ArgumentChecks::MinSize::ARGVALUE: + if (Token::Match(argtok, "%num% ,|)")) { + const MathLib::bigint sz = MathLib::toLongNumber(argtok->str()); + if (sz > arraySize) + error = true; + } else if (argtok->type() == Token::eChar && Token::Match(argtok->next(), ",|)") && charSizeToken) + *charSizeToken = argtok; //sizeArgumentAsCharError(argtok); + break; + case Library::ArgumentChecks::MinSize::MUL: + // TODO: handle arbitrary arg2 + if (minsize->arg2 == minsize->arg+1 && Token::Match(argtok, "%num% , %num% ,|)")) { + const MathLib::bigint sz = MathLib::toLongNumber(argtok->str()) * MathLib::toLongNumber(argtok->strAt(2)); + if (sz > arraySize) + error = true; + } + break; + case Library::ArgumentChecks::MinSize::STRLEN: + if (argtok->type() == Token::eString && Token::getStrLength(argtok) >= arraySize) + error = true; + break; + case Library::ArgumentChecks::MinSize::SIZEOF: + if (argtok->type() == Token::eString && Token::getStrLength(argtok) >= arraySize) + error = true; + break; + case Library::ArgumentChecks::MinSize::NONE: + return false; + }; + } + return error; +} + void CheckBufferOverrun::checkFunctionParameter(const Token &ftok, unsigned int par, const ArrayInfo &arrayInfo, const std::list& callstack) { const std::list * const minsizes = _settings->library.argminsizes(ftok.str(),par); - if (minsizes && !minsizes->empty() && (!(Token::simpleMatch(ftok.previous(), ".") || Token::Match(ftok.tokAt(-2), "!!std ::")))) { + if (minsizes && (!(Token::simpleMatch(ftok.previous(), ".") || Token::Match(ftok.tokAt(-2), "!!std ::")))) { if (arrayInfo.element_size() == 0) return; @@ -276,47 +327,11 @@ void CheckBufferOverrun::checkFunctionParameter(const Token &ftok, unsigned int for (unsigned int i = 0; i < arrayInfo.num().size(); ++i) arraySize *= arrayInfo.num(i); - bool error = true; - for (std::list::const_iterator minsize = minsizes->begin(); minsize != minsizes->end(); ++minsize) { - if (!error) - break; - error = false; - const Token *argtok = ftok.tokAt(2); - for (int argnum = 1; argtok && argnum < minsize->arg; argnum++) - argtok = argtok->nextArgument(); - if (!argtok) - break; - switch (minsize->type) { - case Library::ArgumentChecks::MinSize::ARGVALUE: - if (Token::Match(argtok, "%num% ,|)")) { - const MathLib::bigint sz = MathLib::toLongNumber(argtok->str()); - if (sz > arraySize) - error = true; - } else if (argtok->type() == Token::eChar && Token::Match(argtok->next(), ",|)")) - sizeArgumentAsCharError(argtok); - break; - case Library::ArgumentChecks::MinSize::MUL: - // TODO: handle arbitrary arg2 - if (minsize->arg2 == minsize->arg+1 && Token::Match(argtok, "%num% , %num% ,|)")) { - const MathLib::bigint sz = MathLib::toLongNumber(argtok->str()) * MathLib::toLongNumber(argtok->strAt(2)); - if (sz > arraySize) - error = true; - } - break; - case Library::ArgumentChecks::MinSize::STRLEN: - if (argtok->type() == Token::eString && Token::getStrLength(argtok) >= arraySize) - error = true; - break; - case Library::ArgumentChecks::MinSize::SIZEOF: - if (argtok->type() == Token::eString && Token::getStrLength(argtok) >= arraySize) - error = true; - break; - case Library::ArgumentChecks::MinSize::NONE: - break; - }; - } - if (error) + const Token *charSizeToken = nullptr; + if (checkMinSizes(*minsizes, &ftok, arraySize, &charSizeToken)) bufferOverrunError(callstack, arrayInfo.varname()); + if (charSizeToken) + sizeArgumentAsCharError(charSizeToken); } // Calling a user function? @@ -1277,6 +1292,7 @@ void CheckBufferOverrun::bufferOverrun() checkGlobalAndLocalVariable(); checkStructVariable(); checkBufferAllocatedWithStrlen(); + checkStringArgument(); checkInsecureCmdLineArgs(); } //--------------------------------------------------------------------------- @@ -1504,6 +1520,33 @@ void CheckBufferOverrun::checkBufferAllocatedWithStrlen() } } +//--------------------------------------------------------------------------- +// memcpy(temp, "hello world", 50); +//--------------------------------------------------------------------------- +void CheckBufferOverrun::checkStringArgument() +{ + const SymbolDatabase* const symbolDatabase = _tokenizer->getSymbolDatabase(); + std::size_t functions = symbolDatabase->functionScopes.size(); + for (std::size_t functionIndex = 0; functionIndex < functions; ++functionIndex) { + const Scope * const scope = symbolDatabase->functionScopes[functionIndex]; + for (const Token *tok = scope->classStart; tok != scope->classEnd; tok = tok->next()) { + if (!Token::Match(tok, "%var% (") || !_settings->library.hasminsize(tok->str())) + continue; + + unsigned int argnr = 1; + for (const Token *argtok = tok->tokAt(2); argtok; argtok = argtok->nextArgument(), argnr++) { + if (!Token::Match(argtok, "%str% ,|)")) + continue; + const std::list *minsizes = _settings->library.argminsizes(tok->str(), argnr); + if (!minsizes) + continue; + if (checkMinSizes(*minsizes, tok, Token::getStrLength(argtok), nullptr)) + bufferOverrunError(argtok, argtok->str()); + } + } + } +} + //--------------------------------------------------------------------------- // Checking for buffer overflow caused by copying command line arguments // into fixed-sized buffers without checking to make sure that the command diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index da8e3f1a7..2b02f1946 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -103,12 +103,12 @@ public: /** Check for buffer overruns due to allocating strlen(src) bytes instead of (strlen(src)+1) bytes before copying a string */ void checkBufferAllocatedWithStrlen(); + /** Check string argument buffer overruns */ + void checkStringArgument(); + /** Check for buffer overruns due to copying command-line args to fixed-sized buffers without bounds checking */ void checkInsecureCmdLineArgs(); - /** Check for negative index */ - void negativeIndex(); - /** Information about N-dimensional array */ class CPPCHECKLIB ArrayInfo { private: diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index efbc3a9be..9afdd155c 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -2240,6 +2240,12 @@ private: " std::memset(str, 0, 100);\n" "}"); ASSERT_EQUALS("[test.cpp:5]: (error) Buffer is accessed out of bounds: str\n", errout.str()); + + // #5257 - check strings + checkstd("void f() {\n" + " memcpy(temp, \"hello world\", 20);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (error) Buffer is accessed out of bounds: \"helloworld\"\n", errout.str()); }