From f5d9ba9cf3b3427f5020d40bf336b29b1b0817ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Tue, 6 Oct 2015 17:30:51 +0200 Subject: [PATCH] Rewrote the charvar checker. It now uses valueflow also to limit false negatives. --- lib/astutils.cpp | 2 +- lib/checkother.cpp | 58 ++++++++---------- test/testcharvar.cpp | 143 ++++++++----------------------------------- 3 files changed, 52 insertions(+), 151 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 34d26643b..d7360b72f 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -26,7 +26,7 @@ bool astIsSignedChar(const Token *tok) { - return tok && tok->valueType() && tok->valueType()->sign != ValueType::Sign::UNSIGNED && tok->valueType()->type == ValueType::Type::CHAR && tok->valueType()->pointer == 0U; + return tok && tok->valueType() && tok->valueType()->sign == ValueType::Sign::SIGNED && tok->valueType()->type == ValueType::Type::CHAR && tok->valueType()->pointer == 0U; } bool astIsIntegral(const Token *tok, bool unknown) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index d871ef7f5..f1fc764f9 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1475,42 +1475,38 @@ void CheckOther::checkCharVariable() for (std::size_t i = 0; i < functions; ++i) { const Scope * scope = symbolDatabase->functionScopes[i]; for (const Token* tok = scope->classStart; tok != scope->classEnd; tok = tok->next()) { - if (Token::Match(tok, "%var% [") && astIsSignedChar(tok->next()->astOperand2())) { - const Variable* arrayvar = tok->variable(); - const MathLib::bigint arraysize = (arrayvar && arrayvar->isArray()) ? arrayvar->dimension(0U) : 0; - if (arraysize > 0x80) + if (Token::Match(tok, "%var% [")) { + if (!tok->variable() || !tok->variable()->isArray()) + continue; + const Token *index = tok->next()->astOperand2(); + if (astIsSignedChar(index) && index->getValueGE(0x80, _settings)) charArrayIndexError(tok); } - - else if (Token::Match(tok, "[&|^]")) { - // Don't care about address-of operator - if (!tok->astOperand2()) - continue; - - const Token *tok2; - if (tok->astOperand1() && astIsSignedChar(tok->astOperand1())) - tok2 = tok->astOperand2(); - else if (astIsSignedChar(tok->astOperand2())) - tok2 = tok->astOperand1(); - else - continue; - - // it's ok with a bitwise and where the other operand is 0xff or less.. - if (tok->str() == "&" && tok2 && tok2->isNumber() && MathLib::isGreater("0x100", tok2->str())) + if (Token::Match(tok, "[&|^]") && tok->astOperand2()) { + bool warn = false; + if (astIsSignedChar(tok->astOperand1())) { + const ValueFlow::Value *v1 = tok->astOperand1()->getValueLE(-1, _settings); + const ValueFlow::Value *v2 = tok->astOperand2()->getMaxValue(false); + if (!v1) + v1 = tok->astOperand1()->getValueGE(0x80, _settings); + if (v1 && !(tok->str() == "&" && v2 && v2->isKnown() && v2->intvalue >= 0 && v2->intvalue < 0x100)) + warn = true; + } + if (!warn && astIsSignedChar(tok->astOperand2())) { + const ValueFlow::Value *v1 = tok->astOperand2()->getValueLE(-1, _settings); + const ValueFlow::Value *v2 = tok->astOperand1()->getMaxValue(false); + if (!v1) + v1 = tok->astOperand2()->getValueGE(0x80, _settings); + if (v1 && !(tok->str() == "&" && v2 && v2->isKnown() && v2->intvalue >= 0 && v2->intvalue < 0x100)) + warn = true; + } + if (!warn) continue; // is the result stored in a short|int|long? - if (tok->astParent() && tok->astParent()->str() == "=") { - const Token *eq = tok->astParent(); - const Token *lhs = eq->astOperand1(); - if (lhs && lhs->str() == "*" && !lhs->astOperand2()) - lhs = lhs->astOperand1(); - while (lhs && (lhs->str() == "." || lhs->str() == "::")) - lhs = lhs->astOperand2(); - if (!lhs || !lhs->isName()) - continue; - const Variable *var = lhs->variable(); - if (var && var->isIntegralType() && var->typeStartToken()->str() != "char") + if (Token::simpleMatch(tok->astParent(), "=")) { + const Token *lhs = tok->astParent()->astOperand1(); + if (lhs && lhs->valueType() && lhs->valueType()->type >= ValueType::Type::SHORT) charBitOpError(tok); // This is an error.. } } diff --git a/test/testcharvar.cpp b/test/testcharvar.cpp index 930095f2c..88c820f01 100644 --- a/test/testcharvar.cpp +++ b/test/testcharvar.cpp @@ -32,15 +32,7 @@ private: void run() { TEST_CASE(array_index_1); TEST_CASE(array_index_2); - TEST_CASE(array_index_3); - TEST_CASE(bitop1); - TEST_CASE(bitop2); - TEST_CASE(bitop3); - TEST_CASE(bitop4); // (long)&c - TEST_CASE(return1); - TEST_CASE(assignChar); - TEST_CASE(and03); - TEST_CASE(pointer); + TEST_CASE(bitop); } void check(const char code[]) { @@ -75,7 +67,7 @@ private: " char ch = 0x80;\n" " buf[ch] = 0;\n" "}"); - ASSERT_EQUALS("[test.cpp:5]: (warning) Signed 'char' type used as array index.\n", errout.str()); + ASSERT_EQUALS("", errout.str()); check("int buf[256];\n" "void foo()\n" @@ -86,11 +78,11 @@ private: ASSERT_EQUALS("[test.cpp:5]: (warning) Signed 'char' type used as array index.\n", errout.str()); check("int buf[256];\n" - "void foo(char ch)\n" + "void foo(signed char ch)\n" "{\n" " buf[ch] = 0;\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (warning) Signed 'char' type used as array index.\n", errout.str()); + ASSERT_EQUALS("", errout.str()); check("void foo(const char str[])\n" "{\n" @@ -109,122 +101,35 @@ private: ASSERT_EQUALS("", errout.str()); } - void array_index_3() { - // only write error message when array is more than - // 0x80 elements in size. Otherwise the full valid - // range is accessible with a char. - - check("char buf[0x81];\n" - "void bar(char c) {\n" - " buf[c] = 0;\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Signed 'char' type used as array index.\n", errout.str()); - - check("char buf[0x80];\n" - "void bar(char c) {\n" - " buf[c] = 0;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void bar(char c) {\n" - " buf[c] = 0;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void bitop1() { - check("void foo()\n" - "{\n" - " int result = 0;\n" - " char ch;\n" - " result = a | ch;\n" - "}"); - ASSERT_EQUALS("[test.cpp:5]: (warning) When using 'char' variables in bit operations, sign extension can generate unexpected results.\n", errout.str()); - } - - void bitop2() { - check("void foo()\n" - "{\n" - " char ch;\n" - " func(&ch);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void bitop3() { - check("void f(int& i, char& c) {\n" - " i &= c;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) When using 'char' variables in bit operations, sign extension can generate unexpected results.\n", errout.str()); - } - - void bitop4() { - check("long f(char c) {\n" - " long a;\n" - " a = (long)&c;\n" - " return a;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void return1() { - check("void foo()\n" - "{\n" - " char c;\n" - " return &c;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - - void assignChar() { - check("void foo()\n" - "{\n" - " char c;\n" - " c = c & 0x123;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void and03() { - check("void foo()\n" - "{\n" - " char c;\n" - " int i = c & 0x03;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - - void pointer() { - // ticket #2866 - check("void f(char *p) {\n" - " int ret = 0;\n" - " ret |= *p;\n" - " return ret;\n" + void bitop() { + check("void foo(int *result) {\n" + " signed char ch = -1;\n" + " *result = a | ch;\n" "}"); ASSERT_EQUALS("[test.cpp:3]: (warning) When using 'char' variables in bit operations, sign extension can generate unexpected results.\n", errout.str()); - // fixed code - check("void f(char *p) {\n" - " int ret = 0;\n" - " ret |= (unsigned char)*p;\n" - " return ret;\n" + check("void foo(int *result) {\n" + " unsigned char ch = -1;\n" + " *result = a | ch;\n" "}"); ASSERT_EQUALS("", errout.str()); - // #3872 - false positive - check("int f(int *p) {\n" - " int ret = a();\n" - " ret |= *p;\n" - " return ret;\n" + check("void foo(char *result) {\n" + " signed char ch = -1;\n" + " *result = a | ch;\n" "}"); ASSERT_EQUALS("", errout.str()); - // #3878 - false positive - check("int f(unsigned char *p) {\n" - " int ret = a();\n" - " ret |= *p;\n" - " return ret;\n" + // 0x03 & .. + check("void foo(int *result) {\n" + " signed char ch = -1;\n" + " *result = 0x03 | ch;\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (warning) When using 'char' variables in bit operations, sign extension can generate unexpected results.\n", errout.str()); + + check("void foo(int *result) {\n" + " signed char ch = -1;\n" + " *result = 0x03 & ch;\n" "}"); ASSERT_EQUALS("", errout.str()); }