From 351b382a7bbae480457b0f2f5e6e950bd7e02b79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Sun, 21 Jan 2018 19:51:15 +0100 Subject: [PATCH] Null pointers: Whole program analysis --- lib/checkuninitvar.cpp | 128 ++++++++++++++++++++++++++++------------- lib/checkuninitvar.h | 15 +++-- 2 files changed, 98 insertions(+), 45 deletions(-) diff --git a/lib/checkuninitvar.cpp b/lib/checkuninitvar.cpp index ad4c98ceb..96955cd6e 100644 --- a/lib/checkuninitvar.cpp +++ b/lib/checkuninitvar.cpp @@ -49,13 +49,11 @@ namespace { //--------------------------------------------------------------------------- // CWE ids used: +static const struct CWE CWE476(476U); // NULL Pointer Dereference static const struct CWE CWE676(676U); static const struct CWE CWE908(908U); static const struct CWE CWE825(825U); -static const std::string UNSAFE_FUNCTION_ARG("unsafeFunctionArg"); -static const std::string UNINITIALIZED_FUNCTION_ARG("uninitializedFunctionArg"); - void CheckUninitVar::check() { const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); @@ -1285,27 +1283,26 @@ void CheckUninitVar::deadPointerError(const Token *pointer, const Token *alias) "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 writeFunctionArgXml(const CheckUninitVar::MyFileInfo::FunctionArg &fa, const std::string &elementName, std::ostream &out) +static void writeFunctionArgsXml(const std::list &list, const std::string &elementName, std::ostream &out) { - out << " <" << elementName - << " id=\"" << fa.id << '\"' - << " functionName=\"" << fa.functionName << '\"' - << " argnr=\"" << fa.argnr << '\"' - << " variableName=\"" << fa.variableName << "\"" - << " fileName=\"" << fa.location.fileName << '\"' - << " linenr=\"" << fa.location.linenr << '\"' - << "/>\n"; + 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; - for (std::list::const_iterator it = unsafeFunctionArgs.begin(); it != unsafeFunctionArgs.end(); ++it) { - writeFunctionArgXml(*it, UNSAFE_FUNCTION_ARG, ret); - } - for (std::list::const_iterator it = uninitializedFunctionArgs.begin(); it != uninitializedFunctionArgs.end(); ++it) { - writeFunctionArgXml(*it, UNINITIALIZED_FUNCTION_ARG, ret); - } + writeFunctionArgsXml(uninitialized, "uninitialized", ret); + writeFunctionArgsXml(readData, "readData", ret); + writeFunctionArgsXml(nullPointer, "nullPointer", ret); + writeFunctionArgsXml(dereferenced, "dereferenced", ret); return ret.str(); } @@ -1334,10 +1331,11 @@ Check::FileInfo *CheckUninitVar::getFileInfo(const Tokenizer *tokenizer, const S continue; if (!Token::Match(tok->astParent(), "*|[")) break; + fileInfo->dereferenced.push_back(MyFileInfo::FunctionArg(FUNCTION_ID(function), scope->className, argnr+1, tokenizer->list.file(tok), tok->linenr(), argvar->name())); while (Token::Match(tok->astParent(), "*|[")) tok = tok->astParent(); if (Token::Match(tok->astParent(),"%cop%")) - fileInfo->unsafeFunctionArgs.push_back(MyFileInfo::FunctionArg(FUNCTION_ID(function), scope->className, argnr+1, tokenizer->list.file(tok), tok->linenr(), argvar->name())); + fileInfo->readData.push_back(MyFileInfo::FunctionArg(FUNCTION_ID(function), scope->className, argnr+1, tokenizer->list.file(tok), tok->linenr(), argvar->name())); break; } } @@ -1351,7 +1349,16 @@ Check::FileInfo *CheckUninitVar::getFileInfo(const Tokenizer *tokenizer, const S const std::vector args(getArguments(tok->previous())); for (int argnr = 0; argnr < args.size(); ++argnr) { const Token *argtok = args[argnr]; - if (!argtok || argtok->str() != "&" || argtok->astOperand2()) + 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) @@ -1361,7 +1368,7 @@ Check::FileInfo *CheckUninitVar::getFileInfo(const Tokenizer *tokenizer, const S const ValueFlow::Value &v = argtok->values().front(); if (v.valueType != ValueFlow::Value::UNINIT || v.isInconclusive()) continue; - fileInfo->uninitializedFunctionArgs.push_back(MyFileInfo::FunctionArg(FUNCTION_ID(tok->astOperand1()->function()), tok->astOperand1()->str(), argnr+1, tokenizer->list.file(argtok), argtok->linenr(), argtok->str())); + fileInfo->uninitialized.push_back(MyFileInfo::FunctionArg(FUNCTION_ID(tok->astOperand1()->function()), tok->astOperand1()->str(), argnr+1, tokenizer->list.file(argtok), argtok->linenr(), argtok->str())); } } } @@ -1373,8 +1380,6 @@ Check::FileInfo * CheckUninitVar::loadFileInfoFromXml(const tinyxml2::XMLElement { MyFileInfo *fileInfo = nullptr; for (const tinyxml2::XMLElement *e = xmlElement->FirstChildElement(); e; e = e->NextSiblingElement()) { - if (e->Name() != UNSAFE_FUNCTION_ARG && e->Name() != UNINITIALIZED_FUNCTION_ARG) - continue; const char *id = e->Attribute("id"); if (!id) continue; @@ -1396,10 +1401,14 @@ Check::FileInfo * CheckUninitVar::loadFileInfoFromXml(const tinyxml2::XMLElement const MyFileInfo::FunctionArg fa(id, functionName, MathLib::toLongNumber(argnr), fileName, MathLib::toLongNumber(linenr), variableName); if (!fileInfo) fileInfo = new MyFileInfo; - if (e->Name() == UNSAFE_FUNCTION_ARG) - fileInfo->unsafeFunctionArgs.push_back(fa); - else if (e->Name() == UNINITIALIZED_FUNCTION_ARG) - fileInfo->uninitializedFunctionArgs.push_back(fa); + 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 throw InternalError(nullptr, "Wrong analyze info"); } @@ -1416,28 +1425,30 @@ bool CheckUninitVar::analyseWholeProgram(const std::list &file const MyFileInfo *fi = dynamic_cast(*it); if (!fi) continue; - all.unsafeFunctionArgs.insert(all.unsafeFunctionArgs.end(), fi->unsafeFunctionArgs.begin(), fi->unsafeFunctionArgs.end()); - all.uninitializedFunctionArgs.insert(all.uninitializedFunctionArgs.end(), fi->uninitializedFunctionArgs.begin(), fi->uninitializedFunctionArgs.end()); + 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()); } bool foundErrors = false; - for (std::list::const_iterator it1 = all.uninitializedFunctionArgs.begin(); it1 != all.uninitializedFunctionArgs.end(); ++it1) { - const CheckUninitVar::MyFileInfo::FunctionArg &uninitializedFunctionArg = *it1; - for (std::list::const_iterator it2 = all.unsafeFunctionArgs.begin(); it2 != all.unsafeFunctionArgs.end(); ++it2) { - const CheckUninitVar::MyFileInfo::FunctionArg &unsafeFunctionArg = *it2; - if (uninitializedFunctionArg.id != unsafeFunctionArg.id || uninitializedFunctionArg.argnr != unsafeFunctionArg.argnr) + 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 (uninitialized.id != readData.id || uninitialized.argnr != readData.argnr) continue; ErrorLogger::ErrorMessage::FileLocation fileLoc1; - fileLoc1.setfile(uninitializedFunctionArg.location.fileName); - fileLoc1.line = uninitializedFunctionArg.location.linenr; - fileLoc1.setinfo("Calling function " + uninitializedFunctionArg.functionName + ", variable " + uninitializedFunctionArg.variableName + " is uninitialized"); + 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(unsafeFunctionArg.location.fileName); - fileLoc2.line = unsafeFunctionArg.location.linenr; - fileLoc2.setinfo("Using argument " + unsafeFunctionArg.variableName); + fileLoc2.setfile(readData.location.fileName); + fileLoc2.line = readData.location.linenr; + fileLoc2.setinfo("Using argument " + readData.variableName); std::list locationList; locationList.push_back(fileLoc1); @@ -1446,7 +1457,7 @@ bool CheckUninitVar::analyseWholeProgram(const std::list &file const ErrorLogger::ErrorMessage errmsg(locationList, emptyString, Severity::error, - "using argument " + unsafeFunctionArg.variableName + " that points at uninitialized variable " + uninitializedFunctionArg.variableName, + "using argument " + readData.variableName + " that points at uninitialized variable " + uninitialized.variableName, "uninitvar", CWE908, false); errorLogger.reportErr(errmsg); @@ -1455,6 +1466,41 @@ bool CheckUninitVar::analyseWholeProgram(const std::list &file } } + // 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 (nullPointer.id != dereference.id || nullPointer.argnr != dereference.argnr) + 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, + "uninitvar", + CWE476, false); + errorLogger.reportErr(errmsg); + + foundErrors = true; + } + } + return foundErrors; } diff --git a/lib/checkuninitvar.h b/lib/checkuninitvar.h index 817a5c8b5..1c77f6b14 100644 --- a/lib/checkuninitvar.h +++ b/lib/checkuninitvar.h @@ -110,12 +110,19 @@ public: } location; }; - /* function arguments that must be initialized */ - std::list unsafeFunctionArgs; + /** uninitialized function args */ + std::list uninitialized; - /* uninitialized function args */ - std::list uninitializedFunctionArgs; + /** 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; + + /** Convert MyFileInfo data into xml string */ std::string toString() const; };