From de7e9223b84dd8d9ce52f3af294c3a6087dc5980 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Sat, 15 Nov 2014 10:43:49 +0100 Subject: [PATCH] Fixed #6272 (Improve check: multifile checking in checkbufferoverrun) --- cli/cppcheckexecutor.cpp | 1 + lib/check.h | 37 +++++++--------- lib/checkbufferoverrun.cpp | 91 ++++++++++++++++++++++++++++++++++++++ lib/checkbufferoverrun.h | 22 +++++++++ lib/checkmemoryleak.cpp | 2 +- lib/checkuninitvar.cpp | 36 +++++++-------- lib/checkuninitvar.h | 24 ++++++---- lib/cppcheck.cpp | 32 ++++++++------ lib/cppcheck.h | 7 +++ test/testuninitvar.cpp | 2 +- 10 files changed, 191 insertions(+), 63 deletions(-) diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp index c564d1aa9..eef85ea3b 100644 --- a/cli/cppcheckexecutor.cpp +++ b/cli/cppcheckexecutor.cpp @@ -786,6 +786,7 @@ int CppCheckExecutor::check_internal(CppCheck& cppcheck, int /*argc*/, const cha } cppcheck.checkFunctionUsage(); + cppcheck.analyseWholeProgram(); } else if (!ThreadExecutor::isEnabled()) { std::cout << "No thread support yet implemented for this platform." << std::endl; } else { diff --git a/lib/check.h b/lib/check.h index 08faa81e9..af8f46a16 100644 --- a/lib/check.h +++ b/lib/check.h @@ -58,26 +58,6 @@ public: return _instances; } - /** - * analyse code - must be thread safe - * @param tokens The tokens to analyse - * @param result container where results are stored - */ - virtual void analyse(const Token *tokens, std::set &result) const { - // suppress compiler warnings - (void)tokens; - (void)result; - } - - /** - * Save analysis data - the caller ensures thread safety - * @param data The data where the results are saved - */ - virtual void saveAnalysisData(const std::set &data) const { - // suppress compiler warnings - (void)data; - } - /** run checks, the token list is not simplified */ virtual void runChecks(const Tokenizer *, const Settings *, ErrorLogger *) { } @@ -107,6 +87,23 @@ public: return _settings && _settings->inconclusive; } + /** Base class used for whole-program analysis */ + class FileInfo { + public: + FileInfo() {} + virtual ~FileInfo() {} + }; + + virtual FileInfo * getFileInfo(const Tokenizer *tokenizer) const { + (void)tokenizer; + return nullptr; + } + + virtual void analyseWholeProgram(const std::list &fileInfo, ErrorLogger &errorLogger) { + (void)fileInfo; + (void)errorLogger; + } + protected: const Tokenizer * const _tokenizer; const Settings * const _settings; diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 10a234da2..72243ae41 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -1804,3 +1804,94 @@ void CheckBufferOverrun::writeOutsideBufferSizeError(const Token *tok, const std "The number of bytes to write (" + MathLib::toString(writeLength) + " bytes) are bigger than the source buffer (" +MathLib::toString(stringLength)+ " bytes)." " Please check the second and the third parameter of the function '"+strFunctionName+"'."); } + +Check::FileInfo* CheckBufferOverrun::getFileInfo(const Tokenizer *tokenizer) const +{ + MyFileInfo *fileInfo = new MyFileInfo; + + // Array usage.. + const SymbolDatabase* const symbolDatabase = tokenizer->getSymbolDatabase(); + const std::size_t functions = symbolDatabase->functionScopes.size(); + for (std::size_t i = 0; i < functions; ++i) { + const Scope * const scope = symbolDatabase->functionScopes[i]; + for (const Token *tok = scope->classStart; tok && tok != scope->classEnd; tok = tok->next()) { + if (Token::Match(tok, "%var% [") && + Token::Match(tok->linkAt(1), "] !![") && + tok->variable() && + tok->variable()->isExtern() && + tok->variable()->isGlobal() && + tok->next()->astOperand2()) { + const ValueFlow::Value *value = tok->next()->astOperand2()->getMaxValue(false); + if (value && value->intvalue > 0) { + struct MyFileInfo::ArrayUsage arrayUsage; + arrayUsage.index = value->intvalue; + arrayUsage.fileName = tokenizer->list.file(tok); + arrayUsage.linenr = tok->linenr(); + std::map::iterator it = fileInfo->arrayUsage.find(tok->str()); + if (it == fileInfo->arrayUsage.end() || it->second.index < arrayUsage.index) + fileInfo->arrayUsage[tok->str()] = arrayUsage; + } + } + } + } + + // Arrays.. + const std::list &varlist = symbolDatabase->scopeList.front().varlist; + for (std::list::const_iterator it = varlist.begin(); it != varlist.end(); ++it) { + const Variable &var = *it; + if (!var.isStatic() && var.isArray() && var.dimensions().size() == 1U) + fileInfo->arraySize[var.name()] = var.dimension(0U); + } + + return fileInfo; +} + +void CheckBufferOverrun::analyseWholeProgram(const std::list &fileInfo, ErrorLogger &errorLogger) +{ + // 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; + + // merge array usage + for (std::map::const_iterator it2 = fi->arrayUsage.begin(); it2 != fi->arrayUsage.end(); ++it2) { + std::map::const_iterator allit = all.arrayUsage.find(it2->first); + if (allit == all.arrayUsage.end() || it2->second.index > allit->second.index) + all.arrayUsage[it2->first] = it2->second; + } + + // merge array info + for (std::map::const_iterator it2 = fi->arraySize.begin(); it2 != fi->arraySize.end(); ++it2) { + std::map::const_iterator allit = all.arraySize.find(it2->first); + if (allit == all.arraySize.end()) + all.arraySize[it2->first] = it2->second; + else + all.arraySize[it2->first] = -1; + } + } + + // Check buffer usage + for (std::map::const_iterator it = all.arrayUsage.begin(); it != all.arrayUsage.end(); ++it) { + std::map::const_iterator sz = all.arraySize.find(it->first); + if (sz != all.arraySize.end() && sz->second > 0 && sz->second < it->second.index) { + ErrorLogger::ErrorMessage::FileLocation fileLoc; + fileLoc.setfile(it->second.fileName); + fileLoc.line = it->second.linenr; + + std::list locationList; + locationList.push_back(fileLoc); + + std::ostringstream ostr; + ostr << "Array " << it->first << '[' << sz->second << "] accessed at index " << it->second.index << " which is out of bounds"; + + const ErrorLogger::ErrorMessage errmsg(locationList, + Severity::error, + ostr.str(), + "arrayIndexOutOfBounds", + false); + errorLogger.reportErr(errmsg); + } + } +} diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index 0b4ea8dd3..f9ba2c343 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -203,6 +203,28 @@ public: void arrayIndexOutOfBoundsError(const Token *tok, const ArrayInfo &arrayInfo, const std::vector &index); void arrayIndexOutOfBoundsError(const Token *tok, const ArrayInfo &arrayInfo, const std::vector &index); + /* data for multifile checking */ + class MyFileInfo : public Check::FileInfo { + public: + struct ArrayUsage { + MathLib::bigint index; + std::string fileName; + unsigned int linenr; + }; + + /* key:arrayName */ + std::map arrayUsage; + + /* key:arrayName, data:arraySize */ + std::map arraySize; + }; + + /** @brief Parse current TU and extract file info */ + Check::FileInfo *getFileInfo(const Tokenizer *tokenizer) const; + + /** @brief Analyse all file infos for all TU */ + virtual void analyseWholeProgram(const std::list &fileInfo, ErrorLogger &errorLogger); + private: static bool isArrayOfStruct(const Token* tok, int &position); diff --git a/lib/checkmemoryleak.cpp b/lib/checkmemoryleak.cpp index f262b5684..023900823 100644 --- a/lib/checkmemoryleak.cpp +++ b/lib/checkmemoryleak.cpp @@ -2678,7 +2678,7 @@ void CheckMemoryLeakNoVar::check() std::set uvarFunctions; { const CheckUninitVar c(_tokenizer, _settings, _errorLogger); - c.analyse(_tokenizer->tokens(), uvarFunctions); + c.analyseFunctions(_tokenizer, uvarFunctions); } const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); diff --git a/lib/checkuninitvar.cpp b/lib/checkuninitvar.cpp index e68ef85a1..9a290da2e 100644 --- a/lib/checkuninitvar.cpp +++ b/lib/checkuninitvar.cpp @@ -601,7 +601,7 @@ private: } } - if (Token::Match(&tok, "%var% (") && uvarFunctions.find(tok.str()) == uvarFunctions.end()) { + if (Token::Match(&tok, "%var% (")) { // sizeof/typeof doesn't dereference. A function name that is all uppercase // might be an unexpanded macro that uses sizeof/typeof if (Token::Match(&tok, "sizeof|typeof (")) @@ -920,9 +920,6 @@ private: public: - /** Functions that don't handle uninitialized variables well */ - static std::set uvarFunctions; - static void analyseFunctions(const Token * const tokens, std::set &func) { for (const Token *tok = tokens; tok; tok = tok->next()) { if (tok->str() == "{") { @@ -1013,34 +1010,33 @@ public: } }; -/** Functions that don't handle uninitialized variables well */ -std::set UninitVar::uvarFunctions; - - /// @} -void CheckUninitVar::analyse(const Token * tokens, std::set &func) const +Check::FileInfo *CheckUninitVar::getFileInfo(const Tokenizer *tokenizer) const { - UninitVar::analyseFunctions(tokens, func); + MyFileInfo * mfi = new MyFileInfo; + analyseFunctions(tokenizer, mfi->uvarFunctions); + // TODO: add suspicious function calls + return mfi; } -void CheckUninitVar::saveAnalysisData(const std::set &data) const +void CheckUninitVar::analyseWholeProgram(const std::list &fileInfo, ErrorLogger &errorLogger) { - UninitVar::uvarFunctions.insert(data.begin(), data.end()); + (void)fileInfo; + (void)errorLogger; +} + +void CheckUninitVar::analyseFunctions(const Tokenizer *tokenizer, std::set &f) const +{ + UninitVar::analyseFunctions(tokenizer->tokens(), f); } void CheckUninitVar::executionPaths() { // check if variable is accessed uninitialized.. - { - // no writing if multiple threads are used (TODO: thread safe analysis?) - if (_settings->_jobs == 1) - UninitVar::analyseFunctions(_tokenizer->tokens(), UninitVar::uvarFunctions); - - UninitVar c(this, _tokenizer->getSymbolDatabase(), &_settings->library, _tokenizer->isC()); - checkExecutionPaths(_tokenizer->getSymbolDatabase(), &c); - } + UninitVar c(this, _tokenizer->getSymbolDatabase(), &_settings->library, _tokenizer->isC()); + checkExecutionPaths(_tokenizer->getSymbolDatabase(), &c); } diff --git a/lib/checkuninitvar.h b/lib/checkuninitvar.h index de107c4c8..eb8eba297 100644 --- a/lib/checkuninitvar.h +++ b/lib/checkuninitvar.h @@ -69,15 +69,23 @@ public: void deadPointer(); void deadPointerError(const Token *pointer, const Token *alias); - /** - * @brief Uninitialized variables: analyse functions to see how they work with uninitialized variables - * @param tokens [in] the token list - * @param func [out] names of functions that don't handle uninitialized variables well. the function names are added to the set. No clearing is made. - */ - void analyse(const Token * tokens, std::set &func) const; + /* data for multifile checking */ + class MyFileInfo : public Check::FileInfo { + public: + /* functions that must have initialized data */ + std::set uvarFunctions; - /** Save analysis results */ - void saveAnalysisData(const std::set &data) const; + /* functions calls with uninitialized data */ + std::set functionCalls; + }; + + /** @brief Parse current TU and extract file info */ + Check::FileInfo *getFileInfo(const Tokenizer *tokenizer) const; + + /** @brief Analyse all file infos for all TU */ + virtual void analyseWholeProgram(const std::list &fileInfo, ErrorLogger &errorLogger); + + void analyseFunctions(const Tokenizer *tokenizer, std::set &f) const; /** @brief new type of check: check execution paths */ void executionPaths(); diff --git a/lib/cppcheck.cpp b/lib/cppcheck.cpp index d62b76ab0..ef36338a4 100644 --- a/lib/cppcheck.cpp +++ b/lib/cppcheck.cpp @@ -48,6 +48,10 @@ CppCheck::CppCheck(ErrorLogger &errorLogger, bool useGlobalSuppressions) CppCheck::~CppCheck() { + while (!fileInfo.empty()) { + delete fileInfo.back(); + fileInfo.pop_back(); + } S_timerResults.ShowResults(_settings._showtime); } @@ -309,19 +313,6 @@ void CppCheck::analyseFile(std::istream &fin, const std::string &filename) std::istringstream istr(code); tokenizer.tokenize(istr, filename.c_str()); tokenizer.simplifyTokenList2(); - - // Analyse the tokens.. - std::set data; - for (std::list::const_iterator it = Check::instances().begin(); it != Check::instances().end(); ++it) { - (*it)->analyse(tokenizer.tokens(), data); - } - - // Save analysis results.. - // TODO: This loop should be protected by a mutex or something like that - // The saveAnalysisData must _not_ be called from many threads at the same time. - for (std::list::const_iterator it = Check::instances().begin(); it != Check::instances().end(); ++it) { - (*it)->saveAnalysisData(data); - } } //--------------------------------------------------------------------------- @@ -411,6 +402,13 @@ bool CppCheck::checkFile(const std::string &code, const char FileName[], std::se (*it)->runSimplifiedChecks(&_tokenizer, &_settings, this); } + // Analyse the tokens.. + for (std::list::const_iterator it = Check::instances().begin(); it != Check::instances().end(); ++it) { + Check::FileInfo *fi = (*it)->getFileInfo(&_tokenizer); + if (fi != nullptr) + fileInfo.push_back(fi); + } + if (_settings.terminated()) return true; @@ -690,3 +688,11 @@ void CppCheck::getErrorMessages() Tokenizer::getErrorMessages(this, &_settings); Preprocessor::getErrorMessages(this, &_settings); } + +void CppCheck::analyseWholeProgram() +{ + // Analyse the tokens.. + for (std::list::const_iterator it = Check::instances().begin(); it != Check::instances().end(); ++it) + (*it)->analyseWholeProgram(fileInfo, *this); +} + diff --git a/lib/cppcheck.h b/lib/cppcheck.h index 7ab93b626..ee3f95c42 100644 --- a/lib/cppcheck.h +++ b/lib/cppcheck.h @@ -24,6 +24,7 @@ #include "config.h" #include "settings.h" #include "errorlogger.h" +#include "check.h" #include #include @@ -134,6 +135,9 @@ public: _simplify = false; } + /** analyse whole program, run this after all TUs has been scanned. */ + void analyseWholeProgram(); + private: /** @brief There has been a internal error => Report information message */ @@ -210,6 +214,9 @@ private: /** Simplify code? true by default */ bool _simplify; + + /** File info used for whole program analysis */ + std::list fileInfo; }; /// @} diff --git a/test/testuninitvar.cpp b/test/testuninitvar.cpp index 0b16df101..4c18b716c 100644 --- a/test/testuninitvar.cpp +++ b/test/testuninitvar.cpp @@ -1735,7 +1735,7 @@ private: std::set f; const CheckUninitVar check((const Tokenizer *)0, (const Settings *)0, (ErrorLogger *)0); - check.analyse(tokenizer.tokens(), f); + check.analyseFunctions(&tokenizer, f); std::string ret; for (std::set::const_iterator it = f.begin(); it != f.end(); ++it)