From c8901e9bab4c75303cca74ee5706f6ff438ce992 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Sun, 30 Dec 2018 16:23:25 +0100 Subject: [PATCH] CTU: Find paths better --- lib/checknullpointer.cpp | 4 +- lib/checkuninitvar.cpp | 4 +- lib/ctu.cpp | 116 ++++++++++++++++++++++----------------- lib/ctu.h | 9 +-- test/testnullpointer.cpp | 1 + test/testuninitvar.cpp | 2 +- 6 files changed, 78 insertions(+), 58 deletions(-) diff --git a/lib/checknullpointer.cpp b/lib/checknullpointer.cpp index 7f865cadd..fd9168f9a 100644 --- a/lib/checknullpointer.cpp +++ b/lib/checknullpointer.cpp @@ -653,7 +653,7 @@ bool CheckNullPointer::analyseWholeProgram(const CTU::FileInfo *ctu, const std:: bool foundErrors = false; (void)settings; // This argument is unused - const std::map> nestedCallsMap = ctu->getNestedCallsMap(); + const std::map> callsMap = ctu->getCallsMap(); for (Check::FileInfo *fi1 : fileInfo) { const MyFileInfo *fi = dynamic_cast(fi1); @@ -663,7 +663,7 @@ bool CheckNullPointer::analyseWholeProgram(const CTU::FileInfo *ctu, const std:: const std::list &locationList = ctu->getErrorPath(CTU::FileInfo::InvalidValueType::null, unsafeUsage, - nestedCallsMap, + callsMap, "Dereferencing argument ARG that is null", nullptr); if (locationList.empty()) diff --git a/lib/checkuninitvar.cpp b/lib/checkuninitvar.cpp index c980ffd9a..846c4612c 100644 --- a/lib/checkuninitvar.cpp +++ b/lib/checkuninitvar.cpp @@ -1325,7 +1325,7 @@ bool CheckUninitVar::analyseWholeProgram(const CTU::FileInfo *ctu, const std::li bool foundErrors = false; (void)settings; // This argument is unused - const std::map> nestedCallsMap = ctu->getNestedCallsMap(); + const std::map> callsMap = ctu->getCallsMap(); for (Check::FileInfo *fi1 : fileInfo) { const MyFileInfo *fi = dynamic_cast(fi1); @@ -1337,7 +1337,7 @@ bool CheckUninitVar::analyseWholeProgram(const CTU::FileInfo *ctu, const std::li const std::list &locationList = ctu->getErrorPath(CTU::FileInfo::InvalidValueType::uninit, unsafeUsage, - nestedCallsMap, + callsMap, "Using argument ARG", &functionCall); if (locationList.empty()) diff --git a/lib/ctu.cpp b/lib/ctu.cpp index a847fbe0f..8eaae5b3e 100644 --- a/lib/ctu.cpp +++ b/lib/ctu.cpp @@ -198,11 +198,13 @@ void CTU::FileInfo::loadFromXml(const tinyxml2::XMLElement *xmlElement) } } -std::map> CTU::FileInfo::getNestedCallsMap() const +std::map> CTU::FileInfo::getCallsMap() const { - std::map> ret; + std::map> ret; for (const CTU::FileInfo::NestedCall &nc : nestedCalls) - ret[nc.myId].push_back(nc); + ret[nc.callId].push_back(&nc); + for (const CTU::FileInfo::FunctionCall &fc : functionCalls) + ret[fc.callId].push_back(&fc); return ret; } @@ -378,20 +380,48 @@ std::list CTU::getUnsafeUsage(const Tokenizer *token return unsafeUsage; } -static bool findPath(const CTU::FileInfo::FunctionCall &from, - const CTU::FileInfo::UnsafeUsage &to, - const std::map> &nestedCalls) +static bool findPath(const std::string &callId, + unsigned int callArgNr, + CTU::FileInfo::InvalidValueType invalidValue, + const std::map> &callsMap, + const CTU::FileInfo::CallBase *path[10], + int index) { - if (from.callId == to.myId && from.callArgNr == to.myArgNr) - return true; - - const std::map>::const_iterator nc = nestedCalls.find(from.callId); - if (nc == nestedCalls.end()) + if (index >= 10) return false; - for (const CTU::FileInfo::NestedCall &nestedCall : nc->second) { - if (from.callId == nestedCall.myId && from.callArgNr == nestedCall.myArgNr && nestedCall.callId == to.myId && nestedCall.callArgNr == to.myArgNr) + const std::map>::const_iterator it = callsMap.find(callId); + if (it == callsMap.end()) + return false; + + for (const CTU::FileInfo::CallBase *c : it->second) { + if (c->callArgNr != callArgNr) + continue; + + const CTU::FileInfo::FunctionCall *functionCall = dynamic_cast(c); + if (functionCall) { + switch (invalidValue) { + case CTU::FileInfo::InvalidValueType::null: + if (functionCall->callValueType != ValueFlow::Value::INT || functionCall->callArgValue != 0) + continue; + break; + case CTU::FileInfo::InvalidValueType::uninit: + if (functionCall->callValueType != ValueFlow::Value::UNINIT) + continue; + break; + }; + path[index] = functionCall; return true; + } + + const CTU::FileInfo::NestedCall *nestedCall = dynamic_cast(c); + if (!nestedCall) + continue; + + if (findPath(nestedCall->myId, nestedCall->myArgNr, invalidValue, callsMap, path, index + 1)) { + path[index] = nestedCall; + return true; + } } return false; @@ -399,50 +429,38 @@ static bool findPath(const CTU::FileInfo::FunctionCall &from, std::list CTU::FileInfo::getErrorPath(InvalidValueType invalidValue, const CTU::FileInfo::UnsafeUsage &unsafeUsage, - const std::map> &nestedCallsMap, + const std::map> &callsMap, const char info[], const FunctionCall * * const functionCallPtr) const { std::list locationList; - for (const FunctionCall &functionCall : functionCalls) { - - if (invalidValue == CTU::FileInfo::InvalidValueType::null && - (functionCall.callValueType != ValueFlow::Value::ValueType::INT || functionCall.callArgValue != 0)) { - continue; - } - if (invalidValue == CTU::FileInfo::InvalidValueType::uninit && - functionCall.callValueType != ValueFlow::Value::ValueType::UNINIT) { - continue; - } - - if (!findPath(functionCall, unsafeUsage, nestedCallsMap)) - continue; - - if (functionCallPtr) - *functionCallPtr = &functionCall; - - std::string value1; - if (functionCall.callValueType == ValueFlow::Value::ValueType::INT) - value1 = "null"; - else if (functionCall.callValueType == ValueFlow::Value::ValueType::UNINIT) - value1 = "uninitialized"; - - ErrorLogger::ErrorMessage::FileLocation fileLoc1; - fileLoc1.setfile(functionCall.location.fileName); - fileLoc1.line = functionCall.location.linenr; - fileLoc1.setinfo("Calling function " + functionCall.callFunctionName + ", " + MathLib::toString(functionCall.callArgNr) + getOrdinalText(functionCall.callArgNr) + " argument is " + value1); - - ErrorLogger::ErrorMessage::FileLocation fileLoc2; - fileLoc2.setfile(unsafeUsage.location.fileName); - fileLoc2.line = unsafeUsage.location.linenr; - fileLoc2.setinfo(replaceStr(info, "ARG", unsafeUsage.myArgumentName)); - - locationList.push_back(fileLoc1); - locationList.push_back(fileLoc2); + const CTU::FileInfo::CallBase *path[10] = {0}; + if (!findPath(unsafeUsage.myId, unsafeUsage.myArgNr, invalidValue, callsMap, path, 0)) return locationList; + + const std::string value1 = (invalidValue == InvalidValueType::null) ? "null" : "uninitialized"; + + for (int index = 9; index >= 0; index--) { + if (!path[index]) + continue; + + if (functionCallPtr && !*functionCallPtr) + *functionCallPtr = dynamic_cast(path[index]); + + ErrorLogger::ErrorMessage::FileLocation fileLoc; + fileLoc.setfile(path[index]->location.fileName); + fileLoc.line = path[index]->location.linenr; + fileLoc.setinfo("Calling function " + path[index]->callFunctionName + ", " + MathLib::toString(path[index]->callArgNr) + getOrdinalText(path[index]->callArgNr) + " argument is " + value1); + locationList.push_back(fileLoc); } + ErrorLogger::ErrorMessage::FileLocation fileLoc2; + fileLoc2.setfile(unsafeUsage.location.fileName); + fileLoc2.line = unsafeUsage.location.linenr; + fileLoc2.setinfo(replaceStr(info, "ARG", unsafeUsage.myArgumentName)); + locationList.push_back(fileLoc2); + return locationList; } diff --git a/lib/ctu.h b/lib/ctu.h index c476ea91e..7f493dd4b 100644 --- a/lib/ctu.h +++ b/lib/ctu.h @@ -33,6 +33,8 @@ namespace CTU { class CPPCHECKLIB FileInfo : public Check::FileInfo { public: + enum InvalidValueType { null, uninit }; + std::string toString() const override; struct Location { @@ -60,6 +62,7 @@ namespace CTU { : callId(callId), callArgNr(callArgNr), callFunctionName(callFunctionName), location(loc) {} CallBase(const Tokenizer *tokenizer, const Token *callToken); + virtual ~CallBase() {} std::string callId; int callArgNr; std::string callFunctionName; @@ -102,13 +105,11 @@ namespace CTU { std::list nestedCalls; void loadFromXml(const tinyxml2::XMLElement *xmlElement); - std::map> getNestedCallsMap() const; - - enum InvalidValueType { null, uninit }; + std::map> getCallsMap() const; std::list getErrorPath(InvalidValueType invalidValue, const UnsafeUsage &unsafeUsage, - const std::map> &nestedCallsMap, + const std::map> &callsMap, const char info[], const FunctionCall * * const functionCallPtr) const; }; diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 300aca001..627edd936 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -2715,6 +2715,7 @@ private: "}"); ASSERT_EQUALS("test.cpp:1:error:Null pointer dereference: p\n" "test.cpp:4:note:Calling function call, 2nd argument is null\n" + "test.cpp:2:note:Calling function use, 1st argument is null\n" "test.cpp:1:note:Dereferencing argument p that is null\n", errout.str()); ctu("void dostuff(int *x, int *y) {\n" diff --git a/test/testuninitvar.cpp b/test/testuninitvar.cpp index 322417377..61bf41d17 100644 --- a/test/testuninitvar.cpp +++ b/test/testuninitvar.cpp @@ -4056,7 +4056,7 @@ private: " int x;\n" " call(4,&x);\n" "}"); - ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:1]: (error) Using argument p that points at uninitialized variable x\n", errout.str()); + ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:2] -> [test.cpp:1]: (error) Using argument p that points at uninitialized variable x\n", errout.str()); ctu("void dostuff(int *x, int *y) {\n" " if (!var)\n"