Rewrote the charvar checker. It now uses valueflow also to limit false negatives.
This commit is contained in:
parent
88b3d90505
commit
f5d9ba9cf3
|
@ -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)
|
||||
|
|
|
@ -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..
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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());
|
||||
}
|
||||
|
|
Loading…
Reference in New Issue