From 0f63874c621e525536b1d03fb5a36d1db52de7a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Tue, 18 Dec 2018 07:56:33 +0100 Subject: [PATCH] Take back the whole program analysis for null pointers and uninitialized variables --- lib/checknullpointer.cpp | 24 +++ lib/checknullpointer.h | 2 + lib/checkuninitvar.cpp | 326 +++++++++++++++++++++++++++++++++++++++ lib/checkuninitvar.h | 58 +++++++ test/testnullpointer.cpp | 64 ++++++++ test/testuninitvar.cpp | 68 ++++++++ 6 files changed, 542 insertions(+) diff --git a/lib/checknullpointer.cpp b/lib/checknullpointer.cpp index 8e0b2c53a..50f221ced 100644 --- a/lib/checknullpointer.cpp +++ b/lib/checknullpointer.cpp @@ -20,6 +20,7 @@ //--------------------------------------------------------------------------- #include "checknullpointer.h" +#include "astutils.h" #include "errorlogger.h" #include "library.h" #include "settings.h" @@ -589,3 +590,26 @@ 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->bodyStart; tok2 != scope->bodyEnd; 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, mSettings, mTokenizer->isCPP())) + 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 d22e43549..698a3ae81 100644 --- a/lib/checknullpointer.h +++ b/lib/checknullpointer.h @@ -99,6 +99,8 @@ 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 2a62588bf..d90993663 100644 --- a/lib/checkuninitvar.cpp +++ b/lib/checkuninitvar.cpp @@ -1278,3 +1278,329 @@ void CheckUninitVar::deadPointerError(const Token *pointer, const Token *alias) "deadpointer", "$symbol:" + strpointer + "\nDead pointer usage. Pointer '$symbol' 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) mTokenizer->list.file(function->tokenDef) + ':' + MathLib::toString(function->tokenDef->linenr()) + +CheckUninitVar::MyFileInfo::FunctionArg::FunctionArg(const Tokenizer *mTokenizer, 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 = mTokenizer->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->bodyStart; tok2 != scope->bodyEnd; 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, mSettings, mTokenizer->isCPP())) + 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->bodyStart; tok2 != scope->bodyEnd; 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 = mTokenizer->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->bodyStart; tok != scope->bodyEnd; 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, mTokenizer->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, mTokenizer->list.file(argtok), argtok->linenr(), argtok->str())); + } + } + + // "Unsafe" functions unconditionally reads data before it is written.. + CheckNullPointer checkNullPointer(mTokenizer, mSettings, mErrorLogger); + for (int argnr = 0; argnr < function->argCount(); ++argnr) { + const Token *tok; + if (isUnsafeFunction(&*scope, argnr, &tok)) + fileInfo->readData.push_back(MyFileInfo::FunctionArg(mTokenizer, &*scope, argnr+1, tok)); + if (checkNullPointer.isUnsafeFunction(&*scope, argnr, &tok)) + fileInfo->dereferenced.push_back(MyFileInfo::FunctionArg(mTokenizer, &*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(mTokenizer, &*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 aaedb128a..4cfdc4de8 100644 --- a/lib/checkuninitvar.h +++ b/lib/checkuninitvar.h @@ -88,6 +88,61 @@ 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); @@ -100,6 +155,9 @@ 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 override { CheckUninitVar c(nullptr, settings, errorLogger); diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 68df44819..01fcfeffb 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -17,6 +17,7 @@ */ #include "checknullpointer.h" +#include "checkuninitvar.h" #include "library.h" #include "settings.h" #include "testsuite.h" @@ -108,6 +109,8 @@ private: TEST_CASE(ticket6505); TEST_CASE(subtract); TEST_CASE(addNull); + + TEST_CASE(ctu); } void check(const char code[], bool inconclusive = false, const char filename[] = "test.cpp") { @@ -2668,6 +2671,67 @@ private: "void f(foo x) { if (get()) x += get(); }\n"); 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 *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 85994eebe..768ae7c11 100644 --- a/test/testuninitvar.cpp +++ b/test/testuninitvar.cpp @@ -85,6 +85,9 @@ 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) { @@ -4013,6 +4016,71 @@ 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)