diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 59330843c..7fe38bc14 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -24,9 +24,24 @@ #include "tokenize.h" #include +static bool astIsCharWithSign(const Token *tok, ValueType::Sign sign) +{ + if (!tok) + return false; + const ValueType *valueType = tok->valueType(); + if (!valueType) + return false; + return valueType->type == ValueType::Type::CHAR && valueType->pointer == 0U && valueType->sign == sign; +} + bool astIsSignedChar(const Token *tok) { - return tok && tok->valueType() && tok->valueType()->sign == ValueType::Sign::SIGNED && tok->valueType()->type == ValueType::Type::CHAR && tok->valueType()->pointer == 0U; + return astIsCharWithSign(tok, ValueType::Sign::SIGNED); +} + +bool astIsUnknownSignChar(const Token *tok) +{ + return astIsCharWithSign(tok, ValueType::Sign::UNKNOWN_SIGN); } bool astIsIntegral(const Token *tok, bool unknown) diff --git a/lib/astutils.h b/lib/astutils.h index bd0ff2392..3046c940a 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -29,6 +29,8 @@ class Token; /** Is expression a 'signed char' if no promotion is used */ bool astIsSignedChar(const Token *tok); +/** Is expression a 'char' if no promotion is used? */ +bool astIsUnknownSignChar(const Token *tok); /** Is expression of integral type? */ bool astIsIntegral(const Token *tok, bool unknown); /** Is expression of floating point type? */ diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 4f6b472b3..0311875b3 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1459,7 +1459,9 @@ void CheckOther::passedByValueError(const Token *tok, const std::string &parname void CheckOther::checkCharVariable() { - if (!_settings->isEnabled("warning")) + const bool warning = _settings->isEnabled("warning"); + const bool portability = _settings->isEnabled("portability"); + if (!warning && !portability) return; const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); @@ -1468,12 +1470,16 @@ void CheckOther::checkCharVariable() const Scope * scope = symbolDatabase->functionScopes[i]; for (const Token* tok = scope->classStart; tok != scope->classEnd; tok = tok->next()) { if (Token::Match(tok, "%var% [")) { - if (!tok->variable() || !tok->variable()->isArray()) + if (!tok->variable()) + continue; + if (!tok->variable()->isArray() && !tok->variable()->isPointer()) continue; const Token *index = tok->next()->astOperand2(); - if (astIsSignedChar(index) && index->getValueGE(0x80, _settings)) - charArrayIndexError(tok); - } else if (Token::Match(tok, "[&|^]") && tok->astOperand2() && tok->astOperand1()) { + if (warning && tok->variable()->isArray() && astIsSignedChar(index) && index->getValueGE(0x80, _settings)) + signedCharArrayIndexError(tok); + if (portability && astIsUnknownSignChar(index) && index->getValueGE(0x80, _settings)) + unknownSignCharArrayIndexError(tok); + } else if (warning && Token::Match(tok, "[&|^]") && tok->astOperand2() && tok->astOperand1()) { bool warn = false; if (astIsSignedChar(tok->astOperand1())) { const ValueFlow::Value *v1 = tok->astOperand1()->getValueLE(-1, _settings); @@ -1502,17 +1508,27 @@ void CheckOther::checkCharVariable() } } -void CheckOther::charArrayIndexError(const Token *tok) +void CheckOther::signedCharArrayIndexError(const Token *tok) { reportError(tok, Severity::warning, - "charArrayIndex", + "signedCharArrayIndex", "Signed 'char' type used as array index.\n" "Signed 'char' type used as array index. If the value " "can be greater than 127 there will be a buffer underflow " "because of sign extension."); } +void CheckOther::unknownSignCharArrayIndexError(const Token *tok) +{ + reportError(tok, + Severity::portability, + "unknownSignCharArrayIndex", + "'char' type used as array index.\n" + "'char' type used as array index. Values greater that 127 will be " + "treated depending on whether 'char' is signed or unsigned on target platform."); +} + void CheckOther::charBitOpError(const Token *tok) { reportError(tok, diff --git a/lib/checkother.h b/lib/checkother.h index 7883e7108..40e6cac51 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -218,7 +218,8 @@ private: void invalidPointerCastError(const Token* tok, const std::string& from, const std::string& to, bool inconclusive); void passedByValueError(const Token *tok, const std::string &parname); void constStatementError(const Token *tok, const std::string &type); - void charArrayIndexError(const Token *tok); + void signedCharArrayIndexError(const Token *tok); + void unknownSignCharArrayIndexError(const Token *tok); void charBitOpError(const Token *tok); void variableScopeError(const Token *tok, const std::string &varname); void zerodivError(const Token *tok, bool inconclusive); @@ -280,7 +281,8 @@ private: c.cstyleCastError(0); c.passedByValueError(0, "parametername"); c.constStatementError(0, "type"); - c.charArrayIndexError(0); + c.signedCharArrayIndexError(0); + c.unknownSignCharArrayIndexError(0); c.charBitOpError(0); c.variableScopeError(0, "varname"); c.redundantAssignmentInSwitchError(0, 0, "var"); diff --git a/test/testcharvar.cpp b/test/testcharvar.cpp index 9dc31d202..b473de299 100644 --- a/test/testcharvar.cpp +++ b/test/testcharvar.cpp @@ -32,6 +32,7 @@ private: void run() { settings.platform(Settings::Unspecified); settings.addEnabled("warning"); + settings.addEnabled("portability"); TEST_CASE(array_index_1); TEST_CASE(array_index_2); @@ -67,6 +68,14 @@ private: " char ch = 0x80;\n" " buf[ch] = 0;\n" "}"); + ASSERT_EQUALS("[test.cpp:5]: (portability) 'char' type used as array index.\n", errout.str()); + + check("int buf[256];\n" + "void foo()\n" + "{\n" + " char ch = 0;\n" + " buf[ch] = 0;\n" + "}"); ASSERT_EQUALS("", errout.str()); check("int buf[256];\n" @@ -85,6 +94,14 @@ private: "}"); ASSERT_EQUALS("", errout.str()); + check("int buf[256];\n" + "void foo()\n" + "{\n" + " char ch = 0x80;\n" + " buf[ch] = 0;\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (portability) 'char' type used as array index.\n", errout.str()); + check("int buf[256];\n" "void foo(signed char ch)\n" "{\n" @@ -92,6 +109,53 @@ private: "}"); ASSERT_EQUALS("", errout.str()); + check("int buf[256];\n" + "void foo(char ch)\n" + "{\n" + " buf[ch] = 0;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(char* buf)\n" + "{\n" + " char ch = 0x80;" + " buf[ch] = 0;\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (portability) 'char' type used as array index.\n", errout.str()); + + check("void foo(char* buf)\n" + "{\n" + " char ch = 0;" + " buf[ch] = 0;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(char* buf)\n" + "{\n" + " buf['A'] = 0;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(char* buf, char ch)\n" + "{\n" + " buf[ch] = 0;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("int flags[256];\n" + "void foo(const char* str)\n" + "{\n" + " flags[*str] = 0;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("int flags[256];\n" + "void foo(const char* str)\n" + "{\n" + " flags[(unsigned char)*str] = 0;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check("void foo(const char str[])\n" "{\n" " map[str] = 0;\n"