From 15fc9a622de53f24f9d25b6ff71807ea714e8807 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Sat, 23 Mar 2019 08:36:10 +0100 Subject: [PATCH] CheckBufferOverrun: Add CTU analysis --- lib/checkbufferoverrun.cpp | 103 +++++++++++++++++++++++++++++++++++++ lib/checkbufferoverrun.h | 25 +++++++++ lib/checknullpointer.cpp | 3 +- lib/checkuninitvar.cpp | 3 +- lib/ctu.cpp | 49 ++++++++++++------ lib/ctu.h | 7 +-- test/testbufferoverrun.cpp | 51 +++++++++++++++++- 7 files changed, 218 insertions(+), 23 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 16f37e11c..32b3cf99c 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -636,3 +636,106 @@ void CheckBufferOverrun::bufferNotZeroTerminatedError(const Token *tok, const st } + +//--------------------------------------------------------------------------- +// CTU.. +//--------------------------------------------------------------------------- + +std::string CheckBufferOverrun::MyFileInfo::toString() const OVERRIDE +{ + return CTU::toString(unsafeUsage); +} + +bool CheckBufferOverrun::isCtuUnsafeBufferUsage(const Check *check, const Token *argtok, MathLib::bigint *offset) +{ + const CheckBufferOverrun *c = dynamic_cast(check); + if (!c) + return false; + if (!argtok->valueType()) + return false; + if (!Token::Match(argtok, "%name% [") || argtok->astParent() != argtok->next() || Token::simpleMatch(argtok->linkAt(1), "] [")) + return false; + if (!argtok->next()->astOperand2()) + return false; + if (!argtok->next()->astOperand2()->hasKnownIntValue()) + return false; + if (!offset) + return false; + *offset = argtok->next()->astOperand2()->getKnownIntValue() * argtok->valueType()->typeSize(*c->mSettings); + return true; +} + +/** @brief Parse current TU and extract file info */ +Check::FileInfo *CheckBufferOverrun::getFileInfo(const Tokenizer *tokenizer, const Settings *settings) const OVERRIDE +{ + const std::list &unsafeUsage = CTU::getUnsafeUsage(tokenizer, settings, this, isCtuUnsafeBufferUsage); + if (unsafeUsage.empty()) + return nullptr; + + MyFileInfo *fileInfo = new MyFileInfo; + fileInfo->unsafeUsage = unsafeUsage; + return fileInfo; +} + +Check::FileInfo * CheckBufferOverrun::loadFileInfoFromXml(const tinyxml2::XMLElement *xmlElement) const OVERRIDE +{ + const std::list &unsafeUsage = CTU::loadUnsafeUsageListFromXml(xmlElement); + if (unsafeUsage.empty()) + return nullptr; + + MyFileInfo *fileInfo = new MyFileInfo; + fileInfo->unsafeUsage = unsafeUsage; + return fileInfo; +} + +/** @brief Analyse all file infos for all TU */ +bool CheckBufferOverrun::analyseWholeProgram(const CTU::FileInfo *ctu, const std::list &fileInfo, const Settings& settings, ErrorLogger &errorLogger) OVERRIDE { + if (!ctu) + return false; + bool foundErrors = false; + (void)settings; // This argument is unused + + const std::map> callsMap = ctu->getCallsMap(); + + for (Check::FileInfo *fi1 : fileInfo) + { + const MyFileInfo *fi = dynamic_cast(fi1); + if (!fi) + continue; + for (const CTU::FileInfo::UnsafeUsage &unsafeUsage : fi->unsafeUsage) { + const CTU::FileInfo::FunctionCall *functionCall = nullptr; + + const std::list &locationList = + ctu->getErrorPath(CTU::FileInfo::InvalidValueType::bufferOverflow, + unsafeUsage, + callsMap, + "Using argument ARG", + &functionCall, + false); + if (locationList.empty()) + continue; + + if (unsafeUsage.value > 0) { + const ErrorLogger::ErrorMessage errmsg(locationList, + emptyString, + Severity::error, + "Buffer access out of bounds; '" + unsafeUsage.myArgumentName + "' buffer size is " + MathLib::toString(functionCall->callArgValue) + " and it is accessed at offset " + MathLib::toString(unsafeUsage.value) + ".", + "ctubufferoverrun", + CWE788, false); + errorLogger.reportErr(errmsg); + } else { + const ErrorLogger::ErrorMessage errmsg(locationList, + emptyString, + Severity::error, + "Buffer access out of bounds; buffer '" + unsafeUsage.myArgumentName + "' is accessed at offset " + MathLib::toString(unsafeUsage.value) + ".", + "ctubufferunderrun", + CWE786, false); + errorLogger.reportErr(errmsg); + } + + foundErrors = true; + } + } + return foundErrors; +} + diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index 712ea3658..397c72f95 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -24,6 +24,7 @@ #include "check.h" #include "config.h" +#include "ctu.h" #include "errorlogger.h" #include "mathlib.h" #include "tokenize.h" @@ -76,6 +77,13 @@ public: c.bufferOverflowError(nullptr, nullptr); } + /** @brief Parse current TU and extract file info */ + Check::FileInfo *getFileInfo(const Tokenizer *tokenizer, const Settings *settings) const OVERRIDE; + + /** @brief Analyse all file infos for all TU */ + bool analyseWholeProgram(const CTU::FileInfo *ctu, const std::list &fileInfo, const Settings& settings, ErrorLogger &errorLogger) OVERRIDE; + + private: void arrayIndex(); @@ -94,6 +102,23 @@ private: ValueFlow::Value getBufferSize(const Token *bufTok) const; + // CTU + + /** data for multifile checking */ + class MyFileInfo : public Check::FileInfo { + public: + /** function arguments that data are unconditionally read */ + std::list unsafeUsage; + + /** Convert MyFileInfo data into xml string */ + std::string toString() const OVERRIDE; + }; + + static bool isCtuUnsafeBufferUsage(const Check *check, const Token *argtok, MathLib::bigint *value); + + Check::FileInfo * loadFileInfoFromXml(const tinyxml2::XMLElement *xmlElement) const OVERRIDE; + + static std::string myName() { return "Bounds checking"; } diff --git a/lib/checknullpointer.cpp b/lib/checknullpointer.cpp index 1df01b469..9a94c813c 100644 --- a/lib/checknullpointer.cpp +++ b/lib/checknullpointer.cpp @@ -594,8 +594,9 @@ std::string CheckNullPointer::MyFileInfo::toString() const return CTU::toString(unsafeUsage); } -static bool isUnsafeUsage(const Check *check, const Token *vartok) +static bool isUnsafeUsage(const Check *check, const Token *vartok, MathLib::bigint *value) { + (void)value; const CheckNullPointer *checkNullPointer = dynamic_cast(check); bool unknown = false; return checkNullPointer && checkNullPointer->isPointerDeRef(vartok, unknown); diff --git a/lib/checkuninitvar.cpp b/lib/checkuninitvar.cpp index 2b6a027c5..ff10b8150 100644 --- a/lib/checkuninitvar.cpp +++ b/lib/checkuninitvar.cpp @@ -1342,8 +1342,9 @@ Check::FileInfo *CheckUninitVar::getFileInfo(const Tokenizer *tokenizer, const S return checker.getFileInfo(); } -static bool isVariableUsage(const Check *check, const Token *vartok) +static bool isVariableUsage(const Check *check, const Token *vartok, MathLib::bigint *value) { + (void)value; const CheckUninitVar *c = dynamic_cast(check); return c && c->isVariableUsage(vartok, true, CheckUninitVar::Alloc::ARRAY); } diff --git a/lib/ctu.cpp b/lib/ctu.cpp index 9bcf8a6e6..8160fcbdb 100644 --- a/lib/ctu.cpp +++ b/lib/ctu.cpp @@ -37,6 +37,7 @@ static const char ATTR_INFO[] = "info"; static const char ATTR_MY_ID[] = "my-id"; static const char ATTR_MY_ARGNR[] = "my-argnr"; static const char ATTR_MY_ARGNAME[] = "my-argname"; +static const char ATTR_VALUE[] = "value"; int CTU::maxCtuDepth = 2; @@ -123,6 +124,7 @@ std::string CTU::FileInfo::UnsafeUsage::toString() const << " " << ATTR_MY_ARGNAME << "=\"" << myArgumentName << '\"' << " " << ATTR_LOC_FILENAME << "=\"" << location.fileName << '\"' << " " << ATTR_LOC_LINENR << "=\"" << location.linenr << '\"' + << " " << ATTR_VALUE << "=\"" << value << "\"" << "/>\n"; return out.str(); } @@ -246,6 +248,8 @@ std::list CTU::loadUnsafeUsageListFromXml(const tiny unsafeUsage.myArgumentName = readAttrString(e, ATTR_MY_ARGNAME, &error); unsafeUsage.location.fileName = readAttrString(e, ATTR_LOC_FILENAME, &error); unsafeUsage.location.linenr = readAttrInt(e, ATTR_LOC_LINENR, &error); + unsafeUsage.value = readAttrInt(e, ATTR_VALUE, &error); + if (!error) ret.push_back(unsafeUsage); } @@ -306,10 +310,10 @@ CTU::FileInfo *CTU::getFileInfo(const Tokenizer *tokenizer) if (!argtok) continue; for (const ValueFlow::Value &value : argtok->values()) { - if (!value.isIntValue() || value.intvalue != 0 || value.isInconclusive()) + if ((!value.isIntValue() || value.intvalue != 0 || value.isInconclusive()) && !value.isBufferSizeValue()) continue; FileInfo::FunctionCall functionCall; - functionCall.callValueType = ValueFlow::Value::INT; + functionCall.callValueType = value.valueType; functionCall.callId = getFunctionId(tokenizer, tok->astOperand1()->function()); functionCall.callFunctionName = tok->astOperand1()->expressionString(); functionCall.location.fileName = tokenizer->list.file(tok); @@ -369,18 +373,19 @@ CTU::FileInfo *CTU::getFileInfo(const Tokenizer *tokenizer) return fileInfo; } -static bool isUnsafeFunction(const Tokenizer *tokenizer, const Settings *settings, const Scope *scope, int argnr, const Token **tok, const Check *check, bool (*isUnsafeUsage)(const Check *check, const Token *argtok)) +static std::list> getUnsafeFunction(const Tokenizer *tokenizer, const Settings *settings, const Scope *scope, int argnr, const Check *check, bool (*isUnsafeUsage)(const Check *check, const Token *argtok, MathLib::bigint *value)) { + std::list> ret; const Variable * const argvar = scope->function->getArgumentVar(argnr); if (!argvar->isPointer()) - return false; + return ret; for (const Token *tok2 = scope->bodyStart; tok2 != scope->bodyEnd; tok2 = tok2->next()) { if (Token::Match(tok2, ")|else {")) { tok2 = tok2->linkAt(1); if (Token::findmatch(tok2->link(), "return|throw", tok2)) - return false; + return ret; if (isVariableChanged(tok2->link(), tok2, argvar->declarationId(), false, settings, tokenizer->isCPP())) - return false; + return ret; } if (Token::Match(tok2, "%oror%|&&|?")) { tok2 = tok2->findExpressionStartEndTokens().second; @@ -388,15 +393,16 @@ static bool isUnsafeFunction(const Tokenizer *tokenizer, const Settings *setting } if (tok2->variable() != argvar) continue; - if (!isUnsafeUsage(check, tok2)) - return false; - *tok = tok2; - return true; + MathLib::bigint value = 0; + if (!isUnsafeUsage(check, tok2, &value)) + return ret; // TODO: Is this a read? then continue.. + ret.emplace_back(tok2, value); + return ret; } - return false; + return ret; } -std::list CTU::getUnsafeUsage(const Tokenizer *tokenizer, const Settings *settings, const Check *check, bool (*isUnsafeUsage)(const Check *check, const Token *argtok)) +std::list CTU::getUnsafeUsage(const Tokenizer *tokenizer, const Settings *settings, const Check *check, bool (*isUnsafeUsage)(const Check *check, const Token *argtok, MathLib::bigint *_value)) { std::list unsafeUsage; @@ -410,9 +416,11 @@ std::list CTU::getUnsafeUsage(const Tokenizer *token // "Unsafe" functions unconditionally reads data before it is written.. for (int argnr = 0; argnr < function->argCount(); ++argnr) { - const Token *tok; - if (isUnsafeFunction(tokenizer, settings, &scope, argnr, &tok, check, isUnsafeUsage)) - unsafeUsage.push_back(CTU::FileInfo::UnsafeUsage(CTU::getFunctionId(tokenizer, function), argnr+1, tok->str(), CTU::FileInfo::Location(tokenizer,tok))); + for (const std::pair &v : getUnsafeFunction(tokenizer, settings, &scope, argnr, check, isUnsafeUsage)) { + const Token *tok = v.first; + MathLib::bigint value = v.second; + unsafeUsage.emplace_back(CTU::getFunctionId(tokenizer, function), argnr+1, tok->str(), CTU::FileInfo::Location(tokenizer,tok), value); + } } } @@ -421,6 +429,7 @@ std::list CTU::getUnsafeUsage(const Tokenizer *token static bool findPath(const std::string &callId, unsigned int callArgNr, + MathLib::bigint unsafeValue, CTU::FileInfo::InvalidValueType invalidValue, const std::map> &callsMap, const CTU::FileInfo::CallBase *path[10], @@ -451,6 +460,12 @@ static bool findPath(const std::string &callId, if (functionCall->callValueType != ValueFlow::Value::UNINIT) continue; break; + case CTU::FileInfo::InvalidValueType::bufferOverflow: + if (functionCall->callValueType != ValueFlow::Value::BUFFER_SIZE) + continue; + if (unsafeValue < 0 || unsafeValue >= functionCall->callArgValue) + break; + continue; }; path[index] = functionCall; return true; @@ -460,7 +475,7 @@ static bool findPath(const std::string &callId, if (!nestedCall) continue; - if (findPath(nestedCall->myId, nestedCall->myArgNr, invalidValue, callsMap, path, index + 1, warning)) { + if (findPath(nestedCall->myId, nestedCall->myArgNr, unsafeValue, invalidValue, callsMap, path, index + 1, warning)) { path[index] = nestedCall; return true; } @@ -480,7 +495,7 @@ std::list CTU::FileInfo::getErrorPath(I const CTU::FileInfo::CallBase *path[10] = {0}; - if (!findPath(unsafeUsage.myId, unsafeUsage.myArgNr, invalidValue, callsMap, path, 0, warning)) + if (!findPath(unsafeUsage.myId, unsafeUsage.myArgNr, unsafeUsage.value, invalidValue, callsMap, path, 0, warning)) return locationList; const std::string value1 = (invalidValue == InvalidValueType::null) ? "null" : "uninitialized"; diff --git a/lib/ctu.h b/lib/ctu.h index 65686e078..491337791 100644 --- a/lib/ctu.h +++ b/lib/ctu.h @@ -33,7 +33,7 @@ namespace CTU { class CPPCHECKLIB FileInfo : public Check::FileInfo { public: - enum InvalidValueType { null, uninit }; + enum InvalidValueType { null, uninit, bufferOverflow }; std::string toString() const OVERRIDE; @@ -47,11 +47,12 @@ namespace CTU { struct UnsafeUsage { UnsafeUsage() = default; - UnsafeUsage(const std::string &myId, unsigned int myArgNr, const std::string &myArgumentName, const Location &location) : myId(myId), myArgNr(myArgNr), myArgumentName(myArgumentName), location(location) {} + UnsafeUsage(const std::string &myId, unsigned int myArgNr, const std::string &myArgumentName, const Location &location, MathLib::bigint value) : myId(myId), myArgNr(myArgNr), myArgumentName(myArgumentName), location(location), value(value) {} std::string myId; unsigned int myArgNr; std::string myArgumentName; Location location; + MathLib::bigint value; std::string toString() const; }; @@ -126,7 +127,7 @@ namespace CTU { /** @brief Parse current TU and extract file info */ CPPCHECKLIB FileInfo *getFileInfo(const Tokenizer *tokenizer); - CPPCHECKLIB std::list getUnsafeUsage(const Tokenizer *tokenizer, const Settings *settings, const Check *check, bool (*isUnsafeUsage)(const Check *check, const Token *argtok)); + CPPCHECKLIB std::list getUnsafeUsage(const Tokenizer *tokenizer, const Settings *settings, const Check *check, bool (*isUnsafeUsage)(const Check *check, const Token *argtok, MathLib::bigint *value)); CPPCHECKLIB std::list loadUnsafeUsageListFromXml(const tinyxml2::XMLElement *xmlElement); } diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 97af90313..8df2473da 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -89,7 +89,7 @@ private: // TODO string TEST_CASE(array_index_4); TEST_CASE(array_index_6); TEST_CASE(array_index_7); - // TODO CTU TEST_CASE(array_index_9); + // TODO ctu TEST_CASE(array_index_9); TEST_CASE(array_index_11); TEST_CASE(array_index_12); TEST_CASE(array_index_13); @@ -245,6 +245,8 @@ private: TEST_CASE(negativeArraySize); // TODO TEST_CASE(pointerAddition1); + + TEST_CASE(ctu_1); } @@ -4114,6 +4116,53 @@ private: "\n"); ASSERT_EQUALS("error", errout.str()); } + + void ctu(const char code[]) { + // Clear the error buffer.. + errout.str(""); + + // Tokenize.. + Tokenizer tokenizer(&settings0, this); + std::istringstream istr(code); + tokenizer.tokenize(istr, "test.cpp"); + + CTU::FileInfo *ctu = CTU::getFileInfo(&tokenizer); + + // Check code.. + std::list fileInfo; + CheckBufferOverrun check(&tokenizer, &settings0, this); + fileInfo.push_back(check.getFileInfo(&tokenizer, &settings0)); + check.analyseWholeProgram(ctu, fileInfo, settings0, *this); + while (!fileInfo.empty()) { + delete fileInfo.back(); + fileInfo.pop_back(); + } + delete ctu; + } + + void ctu_1() { + ctu("void dostuff(const char *p) {\n" + " p[-3] = 0;\n" + "}\n" + "\n" + "int main() {\n" + " char *s = malloc(4);\n" + " dostuff(s);\n" + " return 0;\n" + "}"); + ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:7] -> [test.cpp:2]: (error) Buffer access out of bounds; buffer 'p' is accessed at offset -3.\n", errout.str()); + + ctu("void dostuff(const char *p) {\n" + " p[4] = 0;\n" + "}\n" + "\n" + "int main() {\n" + " char *s = malloc(4);\n" + " dostuff(s);\n" + " return 0;\n" + "}"); + ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:7] -> [test.cpp:2]: (error) Buffer access out of bounds; 'p' buffer size is 4 and it is accessed at offset 4.\n", errout.str()); + } }; REGISTER_TEST(TestBufferOverrun)