From 1747813a8b4ab490ad45e62b2499f4a150ed8245 Mon Sep 17 00:00:00 2001 From: PKEuS Date: Sun, 26 Feb 2012 11:56:32 +0100 Subject: [PATCH] Added check for invalid pointer casts (#1255) Detect sign extension problems when variable is a reference (#3637) Refactorizations: - Tokenizer::getFiles returns a reference instead of a pointer, because its guaranteed that no nullpointer is returned - Remove signed/unsigned in one step for "%type% signed|unsigned" - Fixed recently introduced compiler warning in symboldatabase.cpp --- lib/check.h | 4 +- lib/checkbufferoverrun.cpp | 2 +- lib/checkother.cpp | 107 +++++++++++++++++++++++++++++++++---- lib/checkother.h | 7 +++ lib/cppcheck.cpp | 4 +- lib/symboldatabase.cpp | 4 +- lib/tokenize.cpp | 15 +++--- lib/tokenize.h | 4 +- test/testcharvar.cpp | 8 +++ test/testother.cpp | 80 +++++++++++++++++++++++++++ 10 files changed, 208 insertions(+), 27 deletions(-) diff --git a/lib/check.h b/lib/check.h index d3e283cf5..134c52880 100644 --- a/lib/check.h +++ b/lib/check.h @@ -161,8 +161,8 @@ private: } ErrorLogger::ErrorMessage errmsg(locationList, severity, msg, id, inconclusive); - if (_tokenizer && _tokenizer->getFiles() && !_tokenizer->getFiles()->empty()) - errmsg.file0 = _tokenizer->getFiles()->at(0); + if (_tokenizer && !_tokenizer->getFiles().empty()) + errmsg.file0 = _tokenizer->getFiles()[0]; if (_errorLogger) _errorLogger->reportErr(errmsg); else diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 0db5adfcf..bd666f6ae 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -1319,7 +1319,7 @@ void CheckBufferOverrun::checkGlobalAndLocalVariable() if (tok->previous() && (!tok->previous()->isName() && !Token::Match(tok->previous(), "[;{}]"))) continue; - _errorLogger->reportProgress(_tokenizer->getFiles()->front(), + _errorLogger->reportProgress(_tokenizer->getFiles().front(), "Check (BufferOverrun::checkGlobalAndLocalVariable)", tok->progressValue()); diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 1cd62a57a..a5e3e2f38 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -322,6 +322,97 @@ void CheckOther::cstyleCastError(const Token *tok) reportError(tok, Severity::style, "cstyleCast", "C-style pointer casting"); } +//--------------------------------------------------------------------------- +// float* f; double* d = (double*)f; <-- Pointer cast to a type with an incompatible binary data representation +//--------------------------------------------------------------------------- + +static std::string analyzeType(const Token* tok) +{ + if (tok->str() == "double") + if (tok->isLong()) + return "long double"; + else + return "double"; + if (tok->str() == "float") + return "float"; + if (Token::Match(tok, "unsigned| int|long|short|char|size_t")) + return "integer"; + return ""; +} + +void CheckOther::invalidPointerCast() +{ + if (!_settings->isEnabled("style") && !_settings->isEnabled("portability")) + return; + + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + + for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { + const Token* toTok = 0; + const Token* nextTok = 0; + // Find cast + if (Token::Match(tok, "( const| %type% const| * )") || Token::Match(tok, "( const| %type% %type% const| * )")) { + toTok = tok->next(); + nextTok = tok->link()->next(); + if (nextTok->str() == "(") + nextTok = nextTok->next(); + } else if (Token::Match(tok, "reinterpret_cast < const| %type% const| * > (") || Token::Match(tok, "reinterpret_cast < const| %type% %type% const| * > (")) { + nextTok = tok->tokAt(5); + while (nextTok->str() != "(") + nextTok = nextTok->next(); + nextTok = nextTok->next(); + toTok = tok->tokAt(2); + } + if (toTok && toTok->str() == "const") + toTok = toTok->next(); + + if (!nextTok || !toTok || !toTok->isStandardType()) + continue; + + // Find casted variable + unsigned int varid = 0; + bool allocation = false; + bool ref = false; + if (Token::Match(nextTok, "new %type%")) + allocation = true; + else if (Token::Match(nextTok, "%var% !![")) + varid = nextTok->varId(); + else if (Token::Match(nextTok, "& %var%") && !Token::Match(nextTok->tokAt(2), "(|[")) { + varid = nextTok->next()->varId(); + ref = true; + } + + const Token* fromTok = 0; + + if (allocation) { + fromTok = nextTok->next(); + } else { + const Variable* var = symbolDatabase->getVariableFromVarId(varid); + if (!var || (!ref && !var->isPointer() && !var->isArray()) || (ref && (var->isPointer() || var->isArray()))) + continue; + fromTok = var->typeStartToken(); + } + + while (Token::Match(fromTok, "static|const")) + fromTok = fromTok->next(); + if (!fromTok->isStandardType()) + continue; + + std::string fromType = analyzeType(fromTok); + std::string toType = analyzeType(toTok); + if (fromType != toType && !fromType.empty() && !toType.empty() && (toType != "integer" || _settings->isEnabled("portability"))) + invalidPointerCastError(tok, fromType, toType); + } +} + +void CheckOther::invalidPointerCastError(const Token* tok, const std::string& from, const std::string& to) +{ + if (to == "integer") // If we cast something to int*, this can be useful to play with its binary data representation + reportError(tok, Severity::portability, "invalidPointerCast", "Casting from " + from + "* to integer* is not portable due to different binary data representations on different platforms"); + else + reportError(tok, Severity::warning, "invalidPointerCast", "Casting between " + from + "* and " + to + "* which have an incompatible binary data representation"); +} + //--------------------------------------------------------------------------- // fflush(stdin) <- fflush only applies to output streams in ANSI C //--------------------------------------------------------------------------- @@ -1944,7 +2035,7 @@ void CheckOther::passedByValueError(const Token *tok, const std::string &parname //--------------------------------------------------------------------------- static bool isChar(const Variable* var) { - return(var && !var->isPointer() && !var->isArray() && (var->typeEndToken()->str() == "char" || (var->typeEndToken()->previous() && var->typeEndToken()->previous()->str() == "char"))); + return(var && !var->isPointer() && !var->isArray() && Token::Match(var->typeStartToken(), "static| const| char")); } static bool isSignedChar(const Variable* var) @@ -1982,11 +2073,8 @@ void CheckOther::checkCharVariable() // is the result stored in a short|int|long? const Variable *var = symbolDatabase->getVariableFromVarId(tok->next()->varId()); - if (!(var && Token::Match(var->typeEndToken(), "short|int|long"))) - continue; - - // This is an error.. - charBitOpError(tok->tokAt(4)); + if (var && Token::Match(var->typeStartToken(), "static| const| short|int|long") && !var->isPointer() && !var->isArray()) + charBitOpError(tok->tokAt(4)); // This is an error.. } else if (Token::Match(tok, "[;{}] %var% = %any% [&|] ( * %var% ) ;")) { @@ -1999,11 +2087,8 @@ void CheckOther::checkCharVariable() // is the result stored in a short|int|long? var = symbolDatabase->getVariableFromVarId(tok->next()->varId()); - if (!(var && Token::Match(var->typeEndToken(), "short|int|long"))) - continue; - - // This is an error.. - charBitOpError(tok->tokAt(4)); + if (var && Token::Match(var->typeStartToken(), "static| const| short|int|long") && !var->isPointer() && !var->isArray()) + charBitOpError(tok->tokAt(4)); // This is an error.. } } } diff --git a/lib/checkother.h b/lib/checkother.h index f09e7d5bb..f9c9d4809 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -51,6 +51,7 @@ public: // Coding style checks checkOther.warningOldStylePointerCast(); + checkOther.invalidPointerCast(); checkOther.checkUnsignedDivision(); checkOther.checkCharVariable(); checkOther.strPlusChar(); @@ -120,6 +121,9 @@ public: /** @brief Are there C-style pointer casts in a c++ file? */ void warningOldStylePointerCast(); + /** @brief Check for pointer casts to a type with an incompatible binary data representation */ + void invalidPointerCast(); + /** * @brief Invalid function usage (invalid radix / overlapping data) * @@ -263,6 +267,7 @@ public: // Error messages.. void cstyleCastError(const Token *tok); + void invalidPointerCastError(const Token* tok, const std::string& from, const std::string& to); void dangerousUsageStrtolError(const Token *tok); void sprintfOverlappingDataError(const Token *tok, const std::string &varname); void udivError(const Token *tok, bool inconclusive); @@ -325,6 +330,7 @@ public: c.sizeofForNumericParameterError(0); c.coutCerrMisusageError(0, "cout"); c.doubleFreeError(0, "varname"); + c.invalidPointerCastError(0, "float", "double"); // style/warning c.cstyleCastError(0); @@ -396,6 +402,7 @@ public: // style "* C-style pointer cast in cpp file\n" + "* casting between incompatible pointer types\n" "* redundant if\n" "* bad usage of the function 'strtol'\n" "* [[CheckUnsignedDivision|unsigned division]]\n" diff --git a/lib/cppcheck.cpp b/lib/cppcheck.cpp index 3e2f90ad8..89764fe46 100644 --- a/lib/cppcheck.cpp +++ b/lib/cppcheck.cpp @@ -333,8 +333,8 @@ void CppCheck::checkFile(const std::string &code, const char FileName[]) } // Update the _dependencies.. - if (_tokenizer.getFiles()->size() >= 2) - _dependencies.insert(_tokenizer.getFiles()->begin()+1, _tokenizer.getFiles()->end()); + if (_tokenizer.getFiles().size() >= 2) + _dependencies.insert(_tokenizer.getFiles().begin()+1, _tokenizer.getFiles().end()); // call all "runChecks" in all registered Check classes for (std::list::iterator it = Check::instances().begin(); it != Check::instances().end(); ++it) { diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index 9299212f8..603b758fc 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -551,12 +551,12 @@ SymbolDatabase::SymbolDatabase(const Tokenizer *tokenizer, const Settings *setti for (std::list::iterator i = it->friendList.begin(); i != it->friendList.end(); ++i) { for (std::list::iterator j = scopeList.begin(); j != scopeList.end(); ++j) { // check scope for match - const Scope *scope = j->findQualifiedScope(i->name); + scope = const_cast(j->findQualifiedScope(i->name)); // found match? if (scope && scope->isClassOrStruct()) { // set found scope - i->scope = const_cast(scope); + i->scope = scope; break; } } diff --git a/lib/tokenize.cpp b/lib/tokenize.cpp index 3ef9ba253..75fb2a4c4 100644 --- a/lib/tokenize.cpp +++ b/lib/tokenize.cpp @@ -82,9 +82,9 @@ const Token *Tokenizer::tokens() const } -const std::vector *Tokenizer::getFiles() const +const std::vector& Tokenizer::getFiles() const { - return &_files; + return _files; } //--------------------------------------------------------------------------- @@ -5522,12 +5522,13 @@ void Tokenizer::simplifyStdType() for (Token *tok = _tokens; tok; tok = tok->next()) { // long unsigned => unsigned long if (Token::Match(tok, "char|short|int|long|__int8|__int16|__int32|__int64 unsigned|signed")) { - std::string temp = tok->str(); - tok->str(tok->next()->str()); - tok->next()->str(temp); + bool isUnsigned = tok->next()->str() == "unsigned"; + tok->deleteNext(); + tok->isUnsigned(isUnsigned); + tok->isSigned(!isUnsigned); } - if (!Token::Match(tok, "unsigned|signed|char|short|int|long|__int8|__int16|__int32|__int64")) + else if (!Token::Match(tok, "unsigned|signed|char|short|int|long|__int8|__int16|__int32|__int64")) continue; // check if signed or unsigned specified @@ -7397,7 +7398,7 @@ void Tokenizer::simplifyEnum() } if (enumType) { - const std::string pattern(className.empty() ? "" : (className + " :: " + enumType->str()).c_str()); + const std::string pattern(className.empty() ? std::string("") : (className + " :: " + enumType->str())); // count { and } for tok2 int level = 0; diff --git a/lib/tokenize.h b/lib/tokenize.h index cc6c83d88..80543e480 100644 --- a/lib/tokenize.h +++ b/lib/tokenize.h @@ -150,7 +150,7 @@ public: * Simplify '* & ( %var% ) =' or any combination of '* &' and '()' * parentheses around '%var%' to '%var% =' */ - void simplifyMulAndParens(void); + void simplifyMulAndParens(); /** * Get parameter name of function @@ -181,7 +181,7 @@ public: * The first filename is the filename for the sourcefile * @return vector with filenames */ - const std::vector *getFiles() const; + const std::vector& getFiles() const; /** * Get function token by function name diff --git a/test/testcharvar.cpp b/test/testcharvar.cpp index f5690f832..b2cc85b0b 100644 --- a/test/testcharvar.cpp +++ b/test/testcharvar.cpp @@ -37,6 +37,7 @@ private: TEST_CASE(array_index_2); TEST_CASE(bitop1); TEST_CASE(bitop2); + TEST_CASE(bitop3); TEST_CASE(return1); TEST_CASE(assignChar); TEST_CASE(and03); @@ -124,6 +125,13 @@ private: ASSERT_EQUALS("", errout.str()); } + void bitop3() { + check("void f(int& i, char& c) {\n" + " i &= c;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2]: (warning) When using char variables in bit operations, sign extension can generate unexpected results.\n", errout.str()); + } + void return1() { check("void foo()\n" "{\n" diff --git a/test/testother.cpp b/test/testother.cpp index fbd416446..be1c77ffc 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -65,6 +65,7 @@ private: TEST_CASE(varScope12); // variable usage in inner loop TEST_CASE(oldStylePointerCast); + TEST_CASE(invalidPointerCast); TEST_CASE(dangerousStrolUsage); @@ -780,6 +781,85 @@ private: ASSERT_EQUALS("", errout.str()); } + void checkInvalidPointerCast(const char code[], bool portability = false) { + // Clear the error buffer.. + errout.str(""); + + Settings settings; + settings.addEnabled("style"); + if (portability) + settings.addEnabled("portability"); + + // Tokenize.. + Tokenizer tokenizer(&settings, this); + std::istringstream istr(code); + tokenizer.tokenize(istr, "test.cpp"); + + CheckOther checkOtherCpp(&tokenizer, &settings, this); + checkOtherCpp.invalidPointerCast(); + } + + + void invalidPointerCast() { + checkInvalidPointerCast("void test() {\n" + " float *f = new float[10];\n" + " delete [] (double*)f;\n" + " delete [] (long double const*)(new float[10]);\n" + "}"); + TODO_ASSERT_EQUALS("[test.cpp:3]: (warning) Casting between float* and double* which have an incompatible binary data representation\n" + "[test.cpp:4]: (warning) Casting between float* and long double* which have an incompatible binary data representation\n", + "[test.cpp:3]: (warning) Casting between float* and double* which have an incompatible binary data representation\n" + "[test.cpp:4]: (warning) Casting between float* and double* which have an incompatible binary data representation\n", errout.str()); + + checkInvalidPointerCast("void test(const float* f) {\n" + " double *d = (double*)f;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Casting between float* and double* which have an incompatible binary data representation\n", errout.str()); + + checkInvalidPointerCast("void test(double* d1) {\n" + " long double *ld = (long double*)d1;\n" + " double *d2 = (double*)ld;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Casting between double* and long double* which have an incompatible binary data representation\n" + "[test.cpp:3]: (warning) Casting between long double* and double* which have an incompatible binary data representation\n", errout.str()); + + checkInvalidPointerCast("char* test(int* i) {\n" + " long double *d = (long double*)(i);\n" + " double *d = (double*)(i);\n" + " float *f = reinterpret_cast(i);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Casting between integer* and long double* which have an incompatible binary data representation\n" + "[test.cpp:3]: (warning) Casting between integer* and double* which have an incompatible binary data representation\n" + "[test.cpp:4]: (warning) Casting between integer* and float* which have an incompatible binary data representation\n", errout.str()); + + checkInvalidPointerCast("float* test(unsigned int* i) {\n" + " return (float*)i;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Casting between integer* and float* which have an incompatible binary data representation\n", errout.str()); + + checkInvalidPointerCast("float* test(unsigned int* i) {\n" + " return (float*)i[0];\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + checkInvalidPointerCast("float* test(double& d) {\n" + " return (float*)&d;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Casting between double* and float* which have an incompatible binary data representation\n", errout.str()); + + + checkInvalidPointerCast("long long* test(float* f) {\n" + " return (long long*)f;\n" + "}", false); + ASSERT_EQUALS("", errout.str()); + + checkInvalidPointerCast("long long* test(float* f, char* c) {\n" + " foo((long long*)f);\n" + " return reinterpret_cast(c);\n" + "}", true); + ASSERT_EQUALS("[test.cpp:2]: (portability) Casting from float* to integer* is not portable due to different binary data representations on different platforms\n", errout.str()); + } + void dangerousStrolUsage() { { sprintfUsage("int f(const char *num)\n"