From 5f7b4ad0aeffa48790837d89f29b3691bc0ba0d3 Mon Sep 17 00:00:00 2001 From: PKEuS Date: Wed, 6 Aug 2014 14:20:46 +0200 Subject: [PATCH] Added several new va_arg related checks: - Wrong parameter passed to va_start() (#3850) - Reference passed to va_start() (#3849) - Missing va_end() (#3295) - Using va_list before it is opened (#3295) - Subsequent calls to va_start/va_copy() --- lib/checkvaarg.cpp | 142 ++++++++++++++++++++++++ lib/checkvaarg.h | 86 +++++++++++++++ lib/cppcheck.vcxproj | 2 + lib/cppcheck.vcxproj.filters | 6 ++ test/testrunner.vcxproj | 1 + test/testrunner.vcxproj.filters | 3 + test/testvaarg.cpp | 184 ++++++++++++++++++++++++++++++++ 7 files changed, 424 insertions(+) create mode 100644 lib/checkvaarg.cpp create mode 100644 lib/checkvaarg.h create mode 100644 test/testvaarg.cpp diff --git a/lib/checkvaarg.cpp b/lib/checkvaarg.cpp new file mode 100644 index 000000000..1fd3d5e9c --- /dev/null +++ b/lib/checkvaarg.cpp @@ -0,0 +1,142 @@ +/* + * Cppcheck - A tool for static C/C++ code analysis + * Copyright (C) 2007-2014 Daniel Marjamäki and Cppcheck team. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#include "checkvaarg.h" +#include "symboldatabase.h" + +//--------------------------------------------------------------------------- + +// Register this check class (by creating a static instance of it) +namespace { + CheckVaarg instance; +} + + +//--------------------------------------------------------------------------- +// Ensure that correct parameter is passed to va_start() +//--------------------------------------------------------------------------- + +void CheckVaarg::va_start_argument() +{ + 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* scope = symbolDatabase->functionScopes[i]; + const Function* function = scope->function; + for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) { + if (Token::simpleMatch(tok, "va_start (")) { + const Token* param2 = tok->tokAt(2)->nextArgument(); + const Variable* var = param2->variable(); + if (var && var->isReference()) + referenceAs_va_start_error(param2, var->name()); + if (var->index() + 2 < function->argCount() && _settings->isEnabled("warning")) { + std::list::const_reverse_iterator it = function->argumentList.rbegin(); + it++; + wrongParameterTo_va_start_error(tok, var->name(), it->name()); + } + tok = tok->linkAt(1); + } + } + } +} + +void CheckVaarg::wrongParameterTo_va_start_error(const Token *tok, const std::string& paramIsName, const std::string& paramShouldName) +{ + reportError(tok, Severity::warning, + "va_start_wrongParameter", "'" + paramIsName + "' given to va_start() is not last named argument of the function. Did you intend to pass '" + paramShouldName + "'?"); +} + +void CheckVaarg::referenceAs_va_start_error(const Token *tok, const std::string& paramName) +{ + reportError(tok, Severity::error, + "va_start_referencePassed", "Using reference '" + paramName + "' as parameter for va_start() results in undefined behaviour."); +} + +//--------------------------------------------------------------------------- +// Detect missing va_end() if va_start() was used +// Detect va_list usage after va_end() +//--------------------------------------------------------------------------- + +void CheckVaarg::va_list_usage() +{ + const SymbolDatabase* const symbolDatabase = _tokenizer->getSymbolDatabase(); + for (unsigned int varid = 1; varid < symbolDatabase->getVariableListSize(); varid++) { + const Variable* var = symbolDatabase->getVariableFromVarId(varid); + if (!var || var->isPointer() || var->isReference() || var->isArray() || !var->scope() || var->typeStartToken()->str() != "va_list") + continue; + if (!var->isLocal() && !var->isArgument()) // Check only local variables and arguments + continue; + + bool open = var->isArgument(); // va_list passed as argument are opened + bool exitOnEndOfStatement = false; + + const Token* tok = var->nameToken()->next(); + for (const Token* tok = var->nameToken()->next(); tok != var->scope()->classEnd; tok = tok->next()) { + if (Token::Match(tok, "va_start ( %varid% ,", var->declarationId())) { + if (open) + va_start_subsequentCallsError(tok, var->name()); + open = true; + tok = tok->linkAt(1); + } else if (Token::Match(tok, "va_end ( %varid% )", var->declarationId())) { + if (!open) + va_list_usedBeforeStartedError(tok, var->name()); + open = false; + tok = tok->linkAt(1); + } else if (Token::simpleMatch(tok, "va_copy (")) { + bool nopen = open; + if (tok->linkAt(1)->previous()->varId() == var->declarationId()) { // Source + if (!open) + va_list_usedBeforeStartedError(tok, var->name()); + nopen = false; + } + if (tok->tokAt(2)->varId() == var->declarationId()) { // Destination + if (open) + va_start_subsequentCallsError(tok, var->name()); + nopen = true; + } + open = nopen; + tok = tok->linkAt(1); + } else if (Token::Match(tok, "throw|return")) + exitOnEndOfStatement = true; + else if (!open && tok->varId() == var->declarationId()) + va_list_usedBeforeStartedError(tok, var->name()); + else if (exitOnEndOfStatement && tok->str() == ";") + break; + } + if (open && !var->isArgument()) + va_end_missingError(tok, var->name()); + } +} + +void CheckVaarg::va_end_missingError(const Token *tok, const std::string& varname) +{ + reportError(tok, Severity::error, + "va_end_missing", "va_list '" + varname + "' was opened but not closed by va_end()."); +} + +void CheckVaarg::va_list_usedBeforeStartedError(const Token *tok, const std::string& varname) +{ + reportError(tok, Severity::error, + "va_list_usedBeforeStarted", "va_list '" + varname + "' used before va_start() was called."); +} + +void CheckVaarg::va_start_subsequentCallsError(const Token *tok, const std::string& varname) +{ + reportError(tok, Severity::error, + "va_start_subsequentCalls", "va_start() or va_copy() called subsequently on '" + varname + "' without va_end() inbetween."); +} diff --git a/lib/checkvaarg.h b/lib/checkvaarg.h new file mode 100644 index 000000000..fb7dced9d --- /dev/null +++ b/lib/checkvaarg.h @@ -0,0 +1,86 @@ +/* + * Cppcheck - A tool for static C/C++ code analysis + * Copyright (C) 2007-2014 Daniel Marjamäki and Cppcheck team. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + + +//--------------------------------------------------------------------------- +#ifndef checkvaargtH +#define checkvaargtH +//--------------------------------------------------------------------------- + +#include "config.h" +#include "check.h" + +/// @addtogroup Checks +/// @{ + +/** + * @brief Checking for misusage of variable argument lists + */ + +class CPPCHECKLIB CheckVaarg : public Check { +public: + CheckVaarg() : Check(myName()) { + } + + CheckVaarg(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) + : Check(myName(), tokenizer, settings, errorLogger) { + } + + virtual void runSimplifiedChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) { + CheckVaarg check(tokenizer, settings, errorLogger); + check.va_start_argument(); + check.va_list_usage(); + } + + void va_start_argument(); + void va_list_usage(); + +private: + void wrongParameterTo_va_start_error(const Token *tok, const std::string& paramIsName, const std::string& paramShouldName); + void referenceAs_va_start_error(const Token *tok, const std::string& paramName); + void va_end_missingError(const Token *tok, const std::string& varname); + void va_list_usedBeforeStartedError(const Token *tok, const std::string& varname); + void va_start_subsequentCallsError(const Token *tok, const std::string& varname); + + void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { + CheckVaarg c(0, settings, errorLogger); + c.wrongParameterTo_va_start_error(0, "arg1", "arg2"); + c.referenceAs_va_start_error(0, "arg1"); + c.va_end_missingError(0, "vl"); + c.va_list_usedBeforeStartedError(0, "vl"); + c.va_start_subsequentCallsError(0, "vl"); + } + + static std::string myName() { + return "CheckVaarg"; + } + + std::string classInfo() const { + return "Check for misusage of variable argument lists:\n" + "* Wrong parameter passed to va_start()\n" + "* Reference passed to va_start()\n" + "* Missing va_end()\n" + "* Using va_list before it is opened\n" + "* Subsequent calls to va_start/va_copy()\n"; + } +}; + +/// @} + +//--------------------------------------------------------------------------- +#endif // checkvaargtH diff --git a/lib/cppcheck.vcxproj b/lib/cppcheck.vcxproj index e10a02654..77bef2fbe 100644 --- a/lib/cppcheck.vcxproj +++ b/lib/cppcheck.vcxproj @@ -60,6 +60,7 @@ + @@ -103,6 +104,7 @@ + diff --git a/lib/cppcheck.vcxproj.filters b/lib/cppcheck.vcxproj.filters index fea6e0f79..634799577 100644 --- a/lib/cppcheck.vcxproj.filters +++ b/lib/cppcheck.vcxproj.filters @@ -134,6 +134,9 @@ Source Files + + Source Files + @@ -265,6 +268,9 @@ Header Files + + Header Files + diff --git a/test/testrunner.vcxproj b/test/testrunner.vcxproj index 574e3bfe8..eec9d63a4 100644 --- a/test/testrunner.vcxproj +++ b/test/testrunner.vcxproj @@ -83,6 +83,7 @@ + diff --git a/test/testrunner.vcxproj.filters b/test/testrunner.vcxproj.filters index a7a1a5f9c..36f53488d 100644 --- a/test/testrunner.vcxproj.filters +++ b/test/testrunner.vcxproj.filters @@ -184,6 +184,9 @@ Source Files + + Source Files + diff --git a/test/testvaarg.cpp b/test/testvaarg.cpp new file mode 100644 index 000000000..635dbe3a6 --- /dev/null +++ b/test/testvaarg.cpp @@ -0,0 +1,184 @@ +/* + * Cppcheck - A tool for static C/C++ code analysis + * Copyright (C) 2007-2014 Daniel Marjamäki and Cppcheck team. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#include "testsuite.h" +#include "checkvaarg.h" +#include "tokenize.h" +#include + +extern std::ostringstream errout; + +class TestVaarg : public TestFixture { +public: + TestVaarg() : TestFixture("TestVaarg") {} + +private: + void check(const char code[]) { + // Clear the error buffer.. + errout.str(""); + + Settings settings; + settings.addEnabled("warning"); + + // Tokenize.. + Tokenizer tokenizer(&settings, this); + std::istringstream istr(code); + tokenizer.tokenize(istr, "test.cpp"); + + // Check.. + CheckVaarg checkVaarg(&tokenizer, &settings, this); + checkVaarg.runSimplifiedChecks(&tokenizer, &settings, this); + } + + void run() { + TEST_CASE(wrongParameterTo_va_start); + TEST_CASE(referenceAs_va_start); + TEST_CASE(va_end_missing); + TEST_CASE(va_list_usedBeforeStarted); + TEST_CASE(va_start_subsequentCalls); + } + + void wrongParameterTo_va_start() { + check("void Format(char* szFormat, char* szBuffer, size_t nSize, ...) {\n" + " va_list arg_ptr;\n" + " va_start(arg_ptr, szFormat);\n" + " va_end(arg_ptr);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (warning) 'szFormat' given to va_start() is not last named argument of the function. Did you intend to pass 'nSize'?\n", errout.str()); + + check("void Format(char* szFormat, char* szBuffer, size_t nSize, ...) {\n" + " va_list arg_ptr;\n" + " va_start(arg_ptr, szBuffer);\n" + " va_end(arg_ptr);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (warning) 'szBuffer' given to va_start() is not last named argument of the function. Did you intend to pass 'nSize'?\n", errout.str()); + + check("void Format(char* szFormat, char* szBuffer, size_t nSize, ...) {\n" + " va_list arg_ptr;\n" + " va_start(arg_ptr, nSize);\n" + " va_end(arg_ptr);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void referenceAs_va_start() { + check("void Format(char* szFormat, char (&szBuffer)[_Size], ...) {\n" + " va_list arg_ptr;\n" + " va_start(arg_ptr, szBuffer);\n" + " va_end(arg_ptr);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Using reference 'szBuffer' as parameter for va_start() results in undefined behaviour.\n", errout.str()); + + check("void Format(char* szFormat, char (*szBuffer)[_Size], ...) {\n" + " va_list arg_ptr;\n" + " va_start(arg_ptr, szBuffer);\n" + " va_end(arg_ptr);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void va_end_missing() { + check("void Format(char* szFormat, char (*szBuffer)[_Size], ...) {\n" + " va_list arg_ptr;\n" + " va_start(arg_ptr, szBuffer);\n" + " va_end(arg_ptr);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void Format(char* szFormat, char (*szBuffer)[_Size], ...) {\n" + " va_list arg_ptr;\n" + " va_start(arg_ptr, szBuffer);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (error) va_list 'arg_ptr' was opened but not closed by va_end().\n", errout.str()); + + check("void Format(char* szFormat, char (*szBuffer)[_Size], ...) {\n" + " va_list arg_ptr;\n" + " va_start(arg_ptr, szBuffer);\n" + " if(sth) return;\n" + " va_end(arg_ptr);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (error) va_list 'arg_ptr' was opened but not closed by va_end().\n", errout.str()); + } + + void va_list_usedBeforeStarted() { + check("void Format(char* szFormat, char (*szBuffer)[_Size], ...) {\n" + " va_list arg_ptr;\n" + " return va_arg(arg_ptr, float);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) va_list 'arg_ptr' used before va_start() was called.\n", errout.str()); + + check("void Format(char* szFormat, char (*szBuffer)[_Size], ...) {\n" + " va_list arg_ptr;\n" + " foo(arg_ptr);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) va_list 'arg_ptr' used before va_start() was called.\n", errout.str()); + + check("void Format(char* szFormat, char (*szBuffer)[_Size], ...) {\n" + " va_list arg_ptr;\n" + " va_copy(f, arg_ptr);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) va_list 'arg_ptr' used before va_start() was called.\n", errout.str()); + + check("void Format(char* szFormat, char (*szBuffer)[_Size], ...) {\n" + " va_list arg_ptr;\n" + " va_start(arg_ptr, szBuffer);\n" + " va_end(arg_ptr);\n" + " return va_arg(arg_ptr, float);\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (error) va_list 'arg_ptr' used before va_start() was called.\n", errout.str()); + + check("void Format(char* szFormat, char (*szBuffer)[_Size], ...) {\n" + " va_list arg_ptr;\n" + " va_start(arg_ptr, szBuffer);\n" + " va_end(arg_ptr);\n" + " va_start(arg_ptr, szBuffer);\n" + " foo(va_arg(arg_ptr, float));\n" + " va_end(arg_ptr);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void va_start_subsequentCalls() { + check("void Format(char* szFormat, char (*szBuffer)[_Size], ...) {\n" + " va_list arg_ptr;\n" + " va_start(arg_ptr, szBuffer);\n" + " va_start(arg_ptr, szBuffer);\n" + " va_end(arg_ptr);\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (error) va_start() or va_copy() called subsequently on 'arg_ptr' without va_end() inbetween.\n", errout.str()); + + check("void Format(char* szFormat, char (*szBuffer)[_Size], ...) {\n" + " va_list vl1;\n" + " va_start(vl1, szBuffer);\n" + " va_copy(vl1, vl1);\n" + " va_end(vl1);\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (error) va_start() or va_copy() called subsequently on 'vl1' without va_end() inbetween.\n", errout.str()); + + check("void Format(char* szFormat, char (*szBuffer)[_Size], ...) {\n" + " va_list arg_ptr;\n" + " va_start(arg_ptr, szBuffer);\n" + " va_end(arg_ptr);\n" + " va_start(arg_ptr, szBuffer);\n" + " va_end(arg_ptr);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } +}; + +REGISTER_TEST(TestVaarg)