From 58066b1f0c67aaff5a5a045653f389defc24cfaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Tue, 6 Feb 2018 14:56:17 +0100 Subject: [PATCH] Remove whole program analysis from 'uninitialized variables' and 'null pointer dereference' checkers. I think this logic can more or less be added in ValueFlow instead and then all ValueFlow checkers should get whole program analysis. --- lib/checknullpointer.cpp | 24 --- lib/checknullpointer.h | 2 - lib/checkuninitvar.cpp | 326 --------------------------------------- lib/checkuninitvar.h | 58 ------- test/testnullpointer.cpp | 63 -------- test/testuninitvar.cpp | 68 -------- 6 files changed, 541 deletions(-) diff --git a/lib/checknullpointer.cpp b/lib/checknullpointer.cpp index df4aaf14a..44e08a728 100644 --- a/lib/checknullpointer.cpp +++ b/lib/checknullpointer.cpp @@ -20,7 +20,6 @@ //--------------------------------------------------------------------------- #include "checknullpointer.h" -#include "astutils.h" #include "errorlogger.h" #include "library.h" #include "settings.h" @@ -548,26 +547,3 @@ void CheckNullPointer::arithmeticError(const Token *tok, const ValueFlow::Value CWE682, // unknown - pointer overflow value && value->isInconclusive()); } - -bool CheckNullPointer::isUnsafeFunction(const Scope *scope, int argnr, const Token **tok) const -{ - const Variable * const argvar = scope->function->getArgumentVar(argnr); - if (!argvar->isPointer()) - return false; - for (const Token *tok2 = scope->classStart; tok2 != scope->classEnd; tok2 = tok2->next()) { - if (Token::simpleMatch(tok2, ") {")) { - tok2 = tok2->linkAt(1); - if (Token::findmatch(tok2->link(), "return|throw", tok2)) - return false; - if (isVariableChanged(tok2->link(), tok2, argvar->declarationId(), false, _settings)) - return false; - } - if (tok2->variable() != argvar) - continue; - if (!Token::Match(tok2->astParent(), "*|[")) - return false; - *tok = tok2; - return true; - } - return false; -} diff --git a/lib/checknullpointer.h b/lib/checknullpointer.h index ff89c8aac..430e05059 100644 --- a/lib/checknullpointer.h +++ b/lib/checknullpointer.h @@ -99,8 +99,6 @@ public: nullPointerError(tok, "", &v, false); } void nullPointerError(const Token *tok, const std::string &varname, const ValueFlow::Value* value, bool inconclusive); - - bool isUnsafeFunction(const Scope *scope, int argnr, const Token **tok) const; private: /** Get error messages. Used by --errorlist */ diff --git a/lib/checkuninitvar.cpp b/lib/checkuninitvar.cpp index 54729dbaf..0019fba7c 100644 --- a/lib/checkuninitvar.cpp +++ b/lib/checkuninitvar.cpp @@ -1282,329 +1282,3 @@ void CheckUninitVar::deadPointerError(const Token *pointer, const Token *alias) "deadpointer", "Dead pointer usage. Pointer '" + strpointer + "' is dead if it has been assigned '" + stralias + "' at line " + MathLib::toString(alias ? alias->linenr() : 0U) + ".", CWE825, false); } - -static void writeFunctionArgsXml(const std::list &list, const std::string &elementName, std::ostream &out) -{ - for (std::list::const_iterator it = list.begin(); it != list.end(); ++it) - out << " <" << elementName - << " id=\"" << it->id << '\"' - << " functionName=\"" << it->functionName << '\"' - << " argnr=\"" << it->argnr << '\"' - << " variableName=\"" << it->variableName << "\"" - << " fileName=\"" << it->location.fileName << '\"' - << " linenr=\"" << it->location.linenr << '\"' - << "/>\n"; -} - -std::string CheckUninitVar::MyFileInfo::toString() const -{ - std::ostringstream ret; - writeFunctionArgsXml(uninitialized, "uninitialized", ret); - writeFunctionArgsXml(readData, "readData", ret); - writeFunctionArgsXml(nullPointer, "nullPointer", ret); - writeFunctionArgsXml(dereferenced, "dereferenced", ret); - writeFunctionArgsXml(nestedCall, "nestedCall", ret); - return ret.str(); -} - -#define FUNCTION_ID(function) _tokenizer->list.file(function->tokenDef) + ':' + MathLib::toString(function->tokenDef->linenr()) - -CheckUninitVar::MyFileInfo::FunctionArg::FunctionArg(const Tokenizer *_tokenizer, const Scope *scope, unsigned int argnr_, const Token *tok) - : - id(FUNCTION_ID(scope->function)), - functionName(scope->className), - argnr(argnr_), - argnr2(0), - variableName(scope->function->getArgumentVar(argnr-1)->name()) -{ - location.fileName = _tokenizer->list.file(tok); - location.linenr = tok->linenr(); -} - -bool CheckUninitVar::isUnsafeFunction(const Scope *scope, int argnr, const Token **tok) const -{ - const Variable * const argvar = scope->function->getArgumentVar(argnr); - if (!argvar->isPointer()) - return false; - for (const Token *tok2 = scope->classStart; tok2 != scope->classEnd; tok2 = tok2->next()) { - if (Token::simpleMatch(tok2, ") {")) { - tok2 = tok2->linkAt(1); - if (Token::findmatch(tok2->link(), "return|throw", tok2)) - return false; - if (isVariableChanged(tok2->link(), tok2, argvar->declarationId(), false, _settings)) - return false; - } - if (tok2->variable() != argvar) - continue; - if (!isVariableUsage(tok2, true, Alloc::ARRAY)) - return false; - *tok = tok2; - return true; - } - return false; -} - -static int isCallFunction(const Scope *scope, int argnr, const Token **tok) -{ - const Variable * const argvar = scope->function->getArgumentVar(argnr); - if (!argvar->isPointer()) - return -1; - for (const Token *tok2 = scope->classStart; tok2 != scope->classEnd; tok2 = tok2->next()) { - if (tok2->variable() != argvar) - continue; - if (!Token::Match(tok2->previous(), "[(,] %var% [,)]")) - break; - int argnr2 = 1; - const Token *prev = tok2; - while (prev && prev->str() != "(") { - if (Token::Match(prev,"]|)")) - prev = prev->link(); - else if (prev->str() == ",") - ++argnr2; - prev = prev->previous(); - } - if (!prev || !Token::Match(prev->previous(), "%name% (")) - break; - if (!prev->astOperand1() || !prev->astOperand1()->function()) - break; - *tok = prev->previous(); - return argnr2; - } - return -1; -} - -Check::FileInfo *CheckUninitVar::getFileInfo(const Tokenizer *tokenizer, const Settings *settings) const -{ - const CheckUninitVar checker(tokenizer, settings, nullptr); - return checker.getFileInfo(); -} - -Check::FileInfo *CheckUninitVar::getFileInfo() const -{ - const SymbolDatabase * const symbolDatabase = _tokenizer->getSymbolDatabase(); - std::list::const_iterator scope; - - MyFileInfo *fileInfo = new MyFileInfo; - - // Parse all functions in TU - for (scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) { - if (!scope->isExecutable() || scope->type != Scope::eFunction || !scope->function) - continue; - const Function *const function = scope->function; - - // function calls where uninitialized data is passed by address - for (const Token *tok = scope->classStart; tok != scope->classEnd; tok = tok->next()) { - if (tok->str() != "(" || !tok->astOperand1() || !tok->astOperand2()) - continue; - if (!tok->astOperand1()->function()) - continue; - const std::vector args(getArguments(tok->previous())); - for (int argnr = 0; argnr < args.size(); ++argnr) { - const Token *argtok = args[argnr]; - if (!argtok) - continue; - if (argtok->valueType() && argtok->valueType()->pointer > 0) { - // null pointer.. - const ValueFlow::Value *value = argtok->getValue(0); - if (value && !value->isInconclusive()) - fileInfo->nullPointer.push_back(MyFileInfo::FunctionArg(FUNCTION_ID(tok->astOperand1()->function()), tok->astOperand1()->str(), argnr+1, _tokenizer->list.file(argtok), argtok->linenr(), argtok->str())); - } - // pointer to uninitialized data.. - if (argtok->str() != "&" || argtok->astOperand2()) - continue; - argtok = argtok->astOperand1(); - if (!argtok || !argtok->valueType() || argtok->valueType()->pointer != 0) - continue; - if (argtok->values().size() != 1U) - continue; - const ValueFlow::Value &v = argtok->values().front(); - if (v.valueType != ValueFlow::Value::UNINIT || v.isInconclusive()) - continue; - fileInfo->uninitialized.push_back(MyFileInfo::FunctionArg(FUNCTION_ID(tok->astOperand1()->function()), tok->astOperand1()->str(), argnr+1, _tokenizer->list.file(argtok), argtok->linenr(), argtok->str())); - } - } - - // "Unsafe" functions unconditionally reads data before it is written.. - CheckNullPointer checkNullPointer(_tokenizer, _settings, _errorLogger); - for (int argnr = 0; argnr < function->argCount(); ++argnr) { - const Token *tok; - if (isUnsafeFunction(&*scope, argnr, &tok)) - fileInfo->readData.push_back(MyFileInfo::FunctionArg(_tokenizer, &*scope, argnr+1, tok)); - if (checkNullPointer.isUnsafeFunction(&*scope, argnr, &tok)) - fileInfo->dereferenced.push_back(MyFileInfo::FunctionArg(_tokenizer, &*scope, argnr+1, tok)); - } - - // Nested function calls - for (int argnr = 0; argnr < function->argCount(); ++argnr) { - const Token *tok; - int argnr2 = isCallFunction(&*scope, argnr, &tok); - if (argnr2 > 0) { - MyFileInfo::FunctionArg fa(_tokenizer, &*scope, argnr+1, tok); - fa.id = FUNCTION_ID(function); - fa.id2 = FUNCTION_ID(tok->function()); - fa.argnr2 = argnr2; - fileInfo->nestedCall.push_back(fa); - } - } - } - - return fileInfo; -} - -Check::FileInfo * CheckUninitVar::loadFileInfoFromXml(const tinyxml2::XMLElement *xmlElement) const -{ - MyFileInfo *fileInfo = nullptr; - for (const tinyxml2::XMLElement *e = xmlElement->FirstChildElement(); e; e = e->NextSiblingElement()) { - const char *id = e->Attribute("id"); - if (!id) - continue; - const char *functionName = e->Attribute("functionName"); - if (!functionName) - continue; - const char *argnr = e->Attribute("argnr"); - if (!argnr || !MathLib::isInt(argnr)) - continue; - const char *fileName = e->Attribute("fileName"); - if (!fileName) - continue; - const char *linenr = e->Attribute("linenr"); - if (!linenr || !MathLib::isInt(linenr)) - continue; - const char *variableName = e->Attribute("variableName"); - if (!variableName) - continue; - const MyFileInfo::FunctionArg fa(id, functionName, MathLib::toLongNumber(argnr), fileName, MathLib::toLongNumber(linenr), variableName); - if (!fileInfo) - fileInfo = new MyFileInfo; - if (std::strcmp(e->Name(), "uninitialized") == 0) - fileInfo->uninitialized.push_back(fa); - else if (std::strcmp(e->Name(), "readData") == 0) - fileInfo->readData.push_back(fa); - else if (std::strcmp(e->Name(), "nullPointer") == 0) - fileInfo->nullPointer.push_back(fa); - else if (std::strcmp(e->Name(), "dereferenced") == 0) - fileInfo->dereferenced.push_back(fa); - else if (std::strcmp(e->Name(), "nestedCall") == 0) - fileInfo->nestedCall.push_back(fa); - else - throw InternalError(nullptr, "Wrong analyze info"); - } - return fileInfo; -} - -static bool findPath(const CheckUninitVar::MyFileInfo::FunctionArg &from, - const CheckUninitVar::MyFileInfo::FunctionArg &to, - const std::map> &nestedCalls) -{ - if (from.id == to.id && from.argnr == to.argnr) - return true; - - const std::map>::const_iterator nc = nestedCalls.find(from.id); - if (nc == nestedCalls.end()) - return false; - - for (std::list::const_iterator it = nc->second.begin(); it != nc->second.end(); ++it) { - if (from.id == it->id && from.argnr == it->argnr && it->id2 == to.id && it->argnr2 == to.argnr) - return true; - } - - return false; -} - -bool CheckUninitVar::analyseWholeProgram(const std::list &fileInfo, const Settings& settings, ErrorLogger &errorLogger) -{ - (void)settings; // This argument is unused - - // Merge all fileinfo.. - MyFileInfo all; - for (std::list::const_iterator it = fileInfo.begin(); it != fileInfo.end(); ++it) { - const MyFileInfo *fi = dynamic_cast(*it); - if (!fi) - continue; - all.uninitialized.insert(all.uninitialized.end(), fi->uninitialized.begin(), fi->uninitialized.end()); - all.readData.insert(all.readData.end(), fi->readData.begin(), fi->readData.end()); - all.nullPointer.insert(all.nullPointer.end(), fi->nullPointer.begin(), fi->nullPointer.end()); - all.dereferenced.insert(all.dereferenced.end(), fi->dereferenced.begin(), fi->dereferenced.end()); - all.nestedCall.insert(all.nestedCall.end(), fi->nestedCall.begin(), fi->nestedCall.end()); - } - - bool foundErrors = false; - - std::map> nestedCalls; - for (std::list::const_iterator it = all.nestedCall.begin(); it != all.nestedCall.end(); ++it) { - std::list &list = nestedCalls[it->id]; - list.push_back(*it); - } - - for (std::list::const_iterator it1 = all.uninitialized.begin(); it1 != all.uninitialized.end(); ++it1) { - const CheckUninitVar::MyFileInfo::FunctionArg &uninitialized = *it1; - for (std::list::const_iterator it2 = all.readData.begin(); it2 != all.readData.end(); ++it2) { - const CheckUninitVar::MyFileInfo::FunctionArg &readData = *it2; - - if (!findPath(*it1, *it2, nestedCalls)) - continue; - - ErrorLogger::ErrorMessage::FileLocation fileLoc1; - fileLoc1.setfile(uninitialized.location.fileName); - fileLoc1.line = uninitialized.location.linenr; - fileLoc1.setinfo("Calling function " + uninitialized.functionName + ", variable " + uninitialized.variableName + " is uninitialized"); - - ErrorLogger::ErrorMessage::FileLocation fileLoc2; - fileLoc2.setfile(readData.location.fileName); - fileLoc2.line = readData.location.linenr; - fileLoc2.setinfo("Using argument " + readData.variableName); - - std::list locationList; - locationList.push_back(fileLoc1); - locationList.push_back(fileLoc2); - - const ErrorLogger::ErrorMessage errmsg(locationList, - emptyString, - Severity::error, - "using argument " + readData.variableName + " that points at uninitialized variable " + uninitialized.variableName, - "ctuuninitvar", - CWE908, false); - errorLogger.reportErr(errmsg); - - foundErrors = true; - } - } - - // Null pointer checking - // TODO: This does not belong here. - for (std::list::const_iterator it1 = all.nullPointer.begin(); it1 != all.nullPointer.end(); ++it1) { - const CheckUninitVar::MyFileInfo::FunctionArg &nullPointer = *it1; - for (std::list::const_iterator it2 = all.dereferenced.begin(); it2 != all.dereferenced.end(); ++it2) { - const CheckUninitVar::MyFileInfo::FunctionArg &dereference = *it2; - - if (!findPath(*it1, *it2, nestedCalls)) - continue; - - ErrorLogger::ErrorMessage::FileLocation fileLoc1; - fileLoc1.setfile(nullPointer.location.fileName); - fileLoc1.line = nullPointer.location.linenr; - fileLoc1.setinfo("Calling function " + nullPointer.functionName + ", " + MathLib::toString(nullPointer.argnr) + getOrdinalText(nullPointer.argnr) + " argument is null"); - - ErrorLogger::ErrorMessage::FileLocation fileLoc2; - fileLoc2.setfile(dereference.location.fileName); - fileLoc2.line = dereference.location.linenr; - fileLoc2.setinfo("Dereferencing argument " + dereference.variableName + " that is null"); - - std::list locationList; - locationList.push_back(fileLoc1); - locationList.push_back(fileLoc2); - - const ErrorLogger::ErrorMessage errmsg(locationList, - emptyString, - Severity::error, - "Null pointer dereference: " + dereference.variableName, - "ctunullpointer", - CWE476, false); - errorLogger.reportErr(errmsg); - - foundErrors = true; - } - } - - return foundErrors; -} diff --git a/lib/checkuninitvar.h b/lib/checkuninitvar.h index 5f9e817ab..bb0bb59bb 100644 --- a/lib/checkuninitvar.h +++ b/lib/checkuninitvar.h @@ -88,61 +88,6 @@ public: /** ValueFlow-based checking for uninitialized variables */ void valueFlowUninit(); - /* data for multifile checking */ - class MyFileInfo : public Check::FileInfo { - public: - struct FunctionArg { - FunctionArg(const std::string &id_, const std::string &functionName_, unsigned int argnr_, const std::string &fileName, unsigned int linenr, const std::string &varname) - : id(id_), - functionName(functionName_), - argnr(argnr_), - argnr2(0), - variableName(varname) { - location.fileName = fileName; - location.linenr = linenr; - } - - FunctionArg(const Tokenizer *tokenizer, const Scope *scope, unsigned int argnr_, const Token *tok); - - std::string id; - std::string id2; - std::string functionName; - unsigned int argnr; - unsigned int argnr2; - std::string variableName; - struct { - std::string fileName; - unsigned int linenr; - } location; - }; - - /** uninitialized function args */ - std::list uninitialized; - - /** function arguments that data are unconditionally read */ - std::list readData; - - /** null pointer function args */ - std::list nullPointer; - - /** function arguments that are unconditionally dereferenced */ - std::list dereferenced; - - /** function calls other function */ - std::list nestedCall; - - /** Convert MyFileInfo data into xml string */ - std::string toString() const; - }; - - /** @brief Parse current TU and extract file info */ - Check::FileInfo *getFileInfo(const Tokenizer *tokenizer, const Settings *settings) const; - - Check::FileInfo * loadFileInfoFromXml(const tinyxml2::XMLElement *xmlElement) const; - - /** @brief Analyse all file infos for all TU */ - bool analyseWholeProgram(const std::list &fileInfo, const Settings& settings, ErrorLogger &errorLogger); - void uninitstringError(const Token *tok, const std::string &varname, bool strncpy_); void uninitdataError(const Token *tok, const std::string &varname); void uninitvarError(const Token *tok, const std::string &varname); @@ -155,9 +100,6 @@ public: void uninitStructMemberError(const Token *tok, const std::string &membername); private: - Check::FileInfo *getFileInfo() const; - bool isUnsafeFunction(const Scope *scope, int argnr, const Token **tok) const; - void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckUninitVar c(nullptr, settings, errorLogger); diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 388b0e36a..4eb269f83 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -17,7 +17,6 @@ */ #include "checknullpointer.h" -#include "checkuninitvar.h" #include "library.h" #include "settings.h" #include "testsuite.h" @@ -106,7 +105,6 @@ private: TEST_CASE(nullpointer_internal_error); // #5080 TEST_CASE(ticket6505); TEST_CASE(subtract); - TEST_CASE(ctu); } void check(const char code[], bool inconclusive = false, const char filename[] = "test.cpp") { @@ -2574,67 +2572,6 @@ private: "}\n"); ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (warning) Either the condition '!s' is redundant or there is overflow in pointer subtraction.\n", errout.str()); } - - void ctu(const char code[]) { - // Clear the error buffer.. - errout.str(""); - - // Tokenize.. - Tokenizer tokenizer(&settings, this); - std::istringstream istr(code); - tokenizer.tokenize(istr, "test.cpp"); - tokenizer.simplifyTokenList2(); - - // Check code.. - std::list fileInfo; - CheckUninitVar check(&tokenizer, &settings, this); - fileInfo.push_back(check.getFileInfo(&tokenizer, &settings)); - check.analyseWholeProgram(fileInfo, settings, *this); - while (!fileInfo.empty()) { - delete fileInfo.back(); - fileInfo.pop_back(); - } - } - - void ctu() { - ctu("void f(int *fp) {\n" - " a = *fp;\n" - "}\n" - "int main() {\n" - " int *p = 0;\n" - " f(p);\n" - "}"); - ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:2]: (error) Null pointer dereference: fp\n", errout.str()); - - ctu("void use(int *p) { a = *p + 3; }\n" - "void call(int x, int *p) { x++; use(p); }\n" - "int main() {\n" - " call(4,0);\n" - "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:1]: (error) Null pointer dereference: p\n", errout.str()); - - ctu("void dostuff(int *x, int *y) {\n" - " if (!var)\n" - " return -1;\n" // <- early return - " *x = *y;\n" - "}\n" - "\n" - "void f() {\n" - " dostuff(a, 0);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - ctu("void dostuff(int *x, int *y) {\n" - " if (cond)\n" - " *y = -1;\n" // <- conditionally written - " *x = *y;\n" - "}\n" - "\n" - "void f() {\n" - " dostuff(a, 0);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } }; REGISTER_TEST(TestNullPointer) diff --git a/test/testuninitvar.cpp b/test/testuninitvar.cpp index 196026771..cc3c5459d 100644 --- a/test/testuninitvar.cpp +++ b/test/testuninitvar.cpp @@ -84,9 +84,6 @@ private: // dead pointer TEST_CASE(deadPointer); - - // whole program analysis - TEST_CASE(ctu); } void checkUninitVar(const char code[], const char fname[] = "test.cpp", bool debugwarnings = false) { @@ -3979,71 +3976,6 @@ private: "}"); ASSERT_EQUALS("", errout.str()); } - - void ctu(const char code[]) { - // Clear the error buffer.. - errout.str(""); - - // Tokenize.. - Tokenizer tokenizer(&settings, this); - std::istringstream istr(code); - tokenizer.tokenize(istr, "test.cpp"); - tokenizer.simplifyTokenList2(); - - // Check code.. - std::list fileInfo; - CheckUninitVar check(&tokenizer, &settings, this); - fileInfo.push_back(check.getFileInfo(&tokenizer, &settings)); - check.analyseWholeProgram(fileInfo, settings, *this); - while (!fileInfo.empty()) { - delete fileInfo.back(); - fileInfo.pop_back(); - } - } - - void ctu() { - ctu("void f(int *p) {\n" - " a = *p;\n" - "}\n" - "int main() {\n" - " int x;\n" - " f(&x);\n" - "}"); - ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:2]: (error) using argument p that points at uninitialized variable x\n", errout.str()); - - ctu("void use(int *p) { a = *p + 3; }\n" - "void call(int x, int *p) { x++; use(p); }\n" - "int main() {\n" - " 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()); - - ctu("void dostuff(int *x, int *y) {\n" - " if (!var)\n" - " return -1;\n" // <- early return - " *x = *y;\n" - "}\n" - "\n" - "void f() {\n" - " int x;\n" - " dostuff(a, &x);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - ctu("void dostuff(int *x, int *y) {\n" - " if (cond)\n" - " *y = -1;\n" // <- conditionally written - " *x = *y;\n" - "}\n" - "\n" - "void f() {\n" - " int x;\n" - " dostuff(a, &x);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - } }; REGISTER_TEST(TestUninitVar)