walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

Use valueflow in unsigned less than zero checker (#1630) 

The unsigned less than zero checker looked for patterns like "<= 0".
Switching to use valueflow improves the checker in a few aspects.

First, it removes false positives where instead of 0, the code is using
0L, 0U, etc. Instead of having to hard code the different variants of 0,
valueflow handles this automatically. This fixes FPs on the form

	uint32_t value = 0xFUL;
	void f() {
  		if (value < 0u)
		{
			value = 0u;
		}
	}

where 0u was previously not recognized by the checker. This fixes #8836.

Morover, it makes it possible to handle templates properly. In commit
fa076598ad, all warnings inside templates
were made inconclusive, since the checker had no idea if "0" came from
a template parameter or not.

This makes it possible to not warn for the following case which was
reported as a FP in #3233

	template<int n> void foo(unsigned int x) {
	if (x <= n);
	}
	foo<0>();

but give a warning for the following case

	template<int n> void foo(unsigned int x) {
	if (x <= 0);
	}

Previously, both these cases gave inconclusive warnings.

Finally, it makes it possible to give warnings for the following code:

	void f(unsigned x) {
		int y = 0;
		if (x <= y) {}
	}

Also, previously, the checker for unsigned variables larger than 0, the
checker used the string of the astoperand. This meant that for code like
the following:

	void f(unsigned x, unsigned y) {
		if (x -y >= 0) {}
	}

cppcheck would output

	[unsigned-expression-positive.c] (style) Unsigned variable '-' can't be negative so it is unnecessary to test it.

using expressionString() instead gives a better error message

        [unsigned-expression-positive.c] (style) Unsigned expression 'x-z' can't be negative so it is unnecessary to test it.
This commit is contained in:
rikardfalkeborn 2019-01-31 09:30:29 +01:00 committed by Daniel Marjamäki
parent 77b23a5555
commit 7779a9186e
3 changed files with 188 additions and 78 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

82
lib/checkother.cpp
View File

@ -2012,10 +2012,6 @@ void CheckOther::checkSignOfUnsignedVariable()
if (!mSettings->isEnabled(Settings::STYLE))
return;
const bool inconclusive = mTokenizer->codeWithTemplates();
if (inconclusive && !mSettings->inconclusive)
return;
const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
for (const Scope * scope : symbolDatabase->functionScopes) {
@ -2024,80 +2020,64 @@ void CheckOther::checkSignOfUnsignedVariable()
if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2())
continue;
if (Token::Match(tok, "<|<= 0") && tok->next() == tok->astOperand2()) {
const ValueFlow::Value *v1 = tok->astOperand1()->getValue(0);
const ValueFlow::Value *v2 = tok->astOperand2()->getValue(0);
if (Token::Match(tok, "<|<=") && v2 && v2->isKnown()) {
const ValueType* vt = tok->astOperand1()->valueType();
if (vt && vt->pointer)
pointerLessThanZeroError(tok, inconclusive);
pointerLessThanZeroError(tok, v2);
if (vt && vt->sign == ValueType::UNSIGNED)
unsignedLessThanZeroError(tok, tok->astOperand1()->expressionString(), inconclusive);
} else if (Token::Match(tok->previous(), "0 >|>=") && tok->previous() == tok->astOperand1()) {
unsignedLessThanZeroError(tok, v2, tok->astOperand1()->expressionString());
} else if (Token::Match(tok, ">|>=") && v1 && v1->isKnown()) {
const ValueType* vt = tok->astOperand2()->valueType();
if (vt && vt->pointer)
pointerLessThanZeroError(tok, inconclusive);
pointerLessThanZeroError(tok, v1);
if (vt && vt->sign == ValueType::UNSIGNED)
unsignedLessThanZeroError(tok, tok->astOperand2()->expressionString(), inconclusive);
} else if (Token::simpleMatch(tok, ">= 0") && tok->next() == tok->astOperand2()) {
unsignedLessThanZeroError(tok, v1, tok->astOperand2()->expressionString());
} else if (Token::simpleMatch(tok, ">=") && v2 && v2->isKnown()) {
const ValueType* vt = tok->astOperand1()->valueType();
if (vt && vt->pointer)
pointerPositiveError(tok, inconclusive);
pointerPositiveError(tok, v2);
if (vt && vt->sign == ValueType::UNSIGNED)
unsignedPositiveError(tok, tok->astOperand1()->str(), inconclusive);
} else if (Token::simpleMatch(tok->previous(), "0 <=") && tok->previous() == tok->astOperand1()) {
unsignedPositiveError(tok, v2, tok->astOperand1()->expressionString());
} else if (Token::simpleMatch(tok, "<=") && v1 && v1->isKnown()) {
const ValueType* vt = tok->astOperand2()->valueType();
if (vt && vt->pointer)
pointerPositiveError(tok, inconclusive);
pointerPositiveError(tok, v1);
if (vt && vt->sign == ValueType::UNSIGNED)
unsignedPositiveError(tok, tok->astOperand2()->str(), inconclusive);
unsignedPositiveError(tok, v1, tok->astOperand2()->expressionString());
}
}
}
}
void CheckOther::unsignedLessThanZeroError(const Token *tok, const std::string &varname, bool inconclusive)
void CheckOther::unsignedLessThanZeroError(const Token *tok, const ValueFlow::Value * v, const std::string &varname)
{
if (inconclusive) {
reportError(tok, Severity::style, "unsignedLessThanZero",
"$symbol:" + varname + "\n"
"Checking if unsigned expression '$symbol' is less than zero. This might be a false warning.\n"
"Checking if unsigned expression '$symbol' is less than zero. An unsigned "
"variable will never be negative so it is either pointless or an error to check if it is. "
"It's not known if the used constant is a template parameter or not and therefore "
"this message might be a false warning.", CWE570, true);
} else {
reportError(tok, Severity::style, "unsignedLessThanZero",
"$symbol:" + varname + "\n"
"Checking if unsigned expression '$symbol' is less than zero.\n"
"The unsigned expression '$symbol' will never be negative so it "
"is either pointless or an error to check if it is.", CWE570, false);
}
reportError(getErrorPath(tok, v, "Unsigned less than zero"), Severity::style, "unsignedLessThanZero",
"$symbol:" + varname + "\n"
"Checking if unsigned expression '$symbol' is less than zero.\n"
"The unsigned expression '$symbol' will never be negative so it "
"is either pointless or an error to check if it is.", CWE570, false);
}
void CheckOther::pointerLessThanZeroError(const Token *tok, bool inconclusive)
void CheckOther::pointerLessThanZeroError(const Token *tok, const ValueFlow::Value *v)
{
reportError(tok, Severity::style, "pointerLessThanZero",
"A pointer can not be negative so it is either pointless or an error to check if it is.", CWE570, inconclusive);
reportError(getErrorPath(tok, v, "Pointer less than zero"), Severity::style, "pointerLessThanZero",
"A pointer can not be negative so it is either pointless or an error to check if it is.", CWE570, false);
}
void CheckOther::unsignedPositiveError(const Token *tok, const std::string &varname, bool inconclusive)
void CheckOther::unsignedPositiveError(const Token *tok, const ValueFlow::Value * v, const std::string &varname)
{
if (inconclusive) {
reportError(tok, Severity::style, "unsignedPositive",
"$symbol:" + varname + "\n"
"Unsigned variable '$symbol' can't be negative so it is unnecessary to test it.\n"
"The unsigned variable '$symbol' can't be negative so it is unnecessary to test it. "
"It's not known if the used constant is a "
"template parameter or not and therefore this message might be a false warning", CWE570, true);
} else {
reportError(tok, Severity::style, "unsignedPositive",
"$symbol:" + varname + "\n"
"Unsigned variable '$symbol' can't be negative so it is unnecessary to test it.", CWE570, false);
}
reportError(getErrorPath(tok, v, "Unsigned positive"), Severity::style, "unsignedPositive",
"$symbol:" + varname + "\n"
"Unsigned expression '$symbol' can't be negative so it is unnecessary to test it.", CWE570, false);
}
void CheckOther::pointerPositiveError(const Token *tok, bool inconclusive)
void CheckOther::pointerPositiveError(const Token *tok, const ValueFlow::Value * v)
{
reportError(tok, Severity::style, "pointerPositive",
"A pointer can not be negative so it is either pointless or an error to check if it is not.", CWE570, inconclusive);
reportError(getErrorPath(tok, v, "Pointer positive"), Severity::style, "pointerPositive",
"A pointer can not be negative so it is either pointless or an error to check if it is not.", CWE570, false);
}
/* check if a constructor in given class scope takes a reference */

16
lib/checkother.h
View File

@ -252,10 +252,10 @@ private:
void duplicateExpressionTernaryError(const Token *tok, ErrorPath errors);
void duplicateBreakError(const Token *tok, bool inconclusive);
void unreachableCodeError(const Token* tok, bool inconclusive);
void unsignedLessThanZeroError(const Token *tok, const std::string &varname, bool inconclusive);
void pointerLessThanZeroError(const Token *tok, bool inconclusive);
void unsignedPositiveError(const Token *tok, const std::string &varname, bool inconclusive);
void pointerPositiveError(const Token *tok, bool inconclusive);
void unsignedLessThanZeroError(const Token *tok, const ValueFlow::Value *v, const std::string &varname);
void pointerLessThanZeroError(const Token *tok, const ValueFlow::Value *v);
void unsignedPositiveError(const Token *tok, const ValueFlow::Value *v, const std::string &varname);
void pointerPositiveError(const Token *tok, const ValueFlow::Value *v);
void SuspiciousSemicolonError(const Token *tok);
void negativeBitwiseShiftError(const Token *tok, int op);
void redundantCopyError(const Token *tok, const std::string &varname);
@ -318,10 +318,10 @@ private:
c.duplicateExpressionTernaryError(nullptr, errorPath);
c.duplicateBreakError(nullptr, false);
c.unreachableCodeError(nullptr, false);
c.unsignedLessThanZeroError(nullptr, "varname", false);
c.unsignedPositiveError(nullptr, "varname", false);
c.pointerLessThanZeroError(nullptr, false);
c.pointerPositiveError(nullptr, false);
c.unsignedLessThanZeroError(nullptr, nullptr, "varname");
c.unsignedPositiveError(nullptr, nullptr, "varname");
c.pointerLessThanZeroError(nullptr, nullptr);
c.pointerPositiveError(nullptr, nullptr);
c.SuspiciousSemicolonError(nullptr);
c.incompleteArrayFillError(nullptr, "buffer", "memset", false);
c.varFuncNullUBError(nullptr);

168
test/testother.cpp
View File

@ -224,7 +224,7 @@ private:
TEST_CASE(constArgument);
}
void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, Settings* settings = 0) {
void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, bool verbose=false, Settings* settings = 0) {
// Clear the error buffer..
errout.str("");
@ -239,6 +239,7 @@ private:
settings->standards.cpp = Standards::CPPLatest;
settings->inconclusive = inconclusive;
settings->experimental = experimental;
settings->verbose = verbose;
// Tokenize..
Tokenizer tokenizer(settings, this);
@ -256,7 +257,7 @@ private:
}
void check(const char code[], Settings *s) {
check(code,"test.cpp",false,true,true,s);
check(code,"test.cpp",false,true,true,false,s);
}
void checkP(const char code[], const char *filename = "test.cpp") {
@ -305,6 +306,7 @@ private:
false, // experimental
false, // inconclusive
true, // runSimpleChecks
false, // verbose
&settings);
}
@ -312,7 +314,7 @@ private:
static Settings settings;
settings.platformType = Settings::Win32A;
check(code, nullptr, false, false, true, &settings);
check(code, nullptr, false, false, true, false, &settings);
}
void emptyBrackets() {
@ -2512,7 +2514,7 @@ private:
check("void foo() {\n"
" exit(0);\n"
" break;\n"
"}", nullptr, false, false, false, &settings);
"}", nullptr, false, false, false, false, &settings);
ASSERT_EQUALS("[test.cpp:3]: (style) Consecutive return, break, continue, goto or throw statements are unnecessary.\n", errout.str());
check("class NeonSession {\n"
@ -2521,16 +2523,16 @@ private:
"void NeonSession::exit()\n"
"{\n"
" SAL_INFO(\"ucb.ucp.webdav\", \"neon commands cannot be aborted\");\n"
"}", nullptr, false, false, false, &settings);
"}", nullptr, false, false, false, false, &settings);
ASSERT_EQUALS("", errout.str());
check("void NeonSession::exit()\n"
"{\n"
" SAL_INFO(\"ucb.ucp.webdav\", \"neon commands cannot be aborted\");\n"
"}", nullptr, false, false, false, &settings);
"}", nullptr, false, false, false, false, &settings);
ASSERT_EQUALS("", errout.str());
check("void foo() { xResAccess->exit(); }", nullptr, false, false, false, &settings);
check("void foo() { xResAccess->exit(); }", nullptr, false, false, false, false, &settings);
ASSERT_EQUALS("", errout.str());
check("void foo(int a)\n"
@ -2773,20 +2775,20 @@ private:
check("void foo() {\n"
" (beat < 100) ? (void)0 : exit(0);\n"
" bar();\n"
"}", nullptr, false, false, false, &settings);
"}", nullptr, false, false, false, false, &settings);
ASSERT_EQUALS("", errout.str());
check("void foo() {\n"
" (beat < 100) ? exit(0) : (void)0;\n"
" bar();\n"
"}", nullptr, false, false, false, &settings);
"}", nullptr, false, false, false, false, &settings);
ASSERT_EQUALS("", errout.str());
// #8261
check("void foo() {\n"
" (beat < 100) ? (void)0 : throw(0);\n"
" bar();\n"
"}", nullptr, false, false, false, &settings);
"}", nullptr, false, false, false, false, &settings);
ASSERT_EQUALS("", errout.str());
}
@ -3696,6 +3698,7 @@ private:
false, // experimental
false, // inconclusive
false, // runSimpleChecks
false, // verbose
nullptr // settings
);
ASSERT_EQUALS("", errout.str());
@ -3885,7 +3888,7 @@ private:
check("void foo() {\n"
" if ((mystrcmp(a, b) == 0) || (mystrcmp(a, b) == 0)) {}\n"
"}", "test.cpp", false, false, true, &settings);
"}", "test.cpp", false, false, true, false, &settings);
ASSERT_EQUALS("[test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str());
check("void GetValue() { return rand(); }\n"
@ -4782,18 +4785,31 @@ private:
" for(unsigned char i = 10; i >= 0; i--)"
" printf(\"%u\", i);\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'i' can't be negative so it is unnecessary to test it.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'i' can't be negative so it is unnecessary to test it.\n", errout.str());
check(
"void foo(bool b) {\n"
" for(unsigned int i = 10; b || i >= 0; i--)"
" printf(\"%u\", i);\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'i' can't be negative so it is unnecessary to test it.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'i' can't be negative so it is unnecessary to test it.\n", errout.str());
{
const char code[] =
"bool foo(unsigned int x) {\n"
" if (x < 0)"
" return true;\n"
" return false;\n"
"}";
check(code, nullptr, false, false, true, false);
ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str());
check(code, nullptr, false, false, true, true);
ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str());
}
check(
"bool foo(unsigned int x) {\n"
" if (x < 0)"
" if (x < 0u)"
" return true;\n"
" return false;\n"
"}");
@ -4807,6 +4823,30 @@ private:
"}");
ASSERT_EQUALS("", errout.str());
{
const char code[] =
"bool foo(unsigned x) {\n"
" int y = 0;\n"
" if (x < y)"
" return true;\n"
" return false;\n"
"}";
check(code, nullptr, false, false, true, false);
ASSERT_EQUALS("[test.cpp:3]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str());
check(code, nullptr, false, false, true, true);
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str());
}
check(
"bool foo(unsigned x) {\n"
" int y = 0;\n"
" if (b)\n"
" y = 1;\n"
" if (x < y)"
" return true;\n"
" return false;\n"
"}");
ASSERT_EQUALS("", errout.str());
check(
"bool foo(unsigned int x) {\n"
" if (0 > x)"
@ -4815,6 +4855,14 @@ private:
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str());
check(
"bool foo(unsigned int x) {\n"
" if (0UL > x)"
" return true;\n"
" return false;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str());
check(
"bool foo(int x) {\n"
" if (0 > x)"
@ -4829,7 +4877,23 @@ private:
" return true;\n"
" return false;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'x' can't be negative so it is unnecessary to test it.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str());
check(
"bool foo(unsigned int x, unsigned y) {\n"
" if (x - y >= 0)"
" return true;\n"
" return false;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x-y' can't be negative so it is unnecessary to test it.\n", errout.str());
check(
"bool foo(unsigned int x) {\n"
" if (x >= 0ull)"
" return true;\n"
" return false;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str());
check(
"bool foo(int x) {\n"
@ -4839,6 +4903,29 @@ private:
"}");
ASSERT_EQUALS("", errout.str());
check(
"bool foo(unsigned int x) {\n"
" if (0 <= x)"
" return true;\n"
" return false;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str());
check(
"bool foo(unsigned int x) {\n"
" if (0ll <= x)"
" return true;\n"
" return false;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str());
check(
"bool foo(int x) {\n"
" if (0 <= x)"
" return true;\n"
" return false;\n"
"}");
ASSERT_EQUALS("", errout.str());
check(
"bool foo(unsigned int x, bool y) {\n"
@ -4878,7 +4965,7 @@ private:
" return true;\n"
" return false;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'x' can't be negative so it is unnecessary to test it.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str());
check(
"bool foo(int x, bool y) {\n"
@ -4927,7 +5014,7 @@ private:
" return true;\n"
" return false;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'x' can't be negative so it is unnecessary to test it.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str());
check(
"bool foo(int x, bool y) {\n"
@ -4976,7 +5063,7 @@ private:
" return true;\n"
" return false;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'x' can't be negative so it is unnecessary to test it.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str());
check(
"bool foo(int x, bool y) {\n"
@ -4996,8 +5083,25 @@ private:
check(code, nullptr, false, false);
ASSERT_EQUALS("", errout.str());
check(code, nullptr, false, true);
ASSERT_EQUALS("[test.cpp:2]: (style, inconclusive) Checking if unsigned expression 'x' is less than zero. This might be a false warning.\n", errout.str());
ASSERT_EQUALS("", errout.str());
}
check(
"template<int n> void foo(unsigned int x) {\n"
"if (x <= 0);\n"
"}\n");
ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str());
// #8836
check(
"uint32_t value = 0xFUL;\n"
"void f() {\n"
" if (value < 0u)\n"
" {\n"
" value = 0u;\n"
" }\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3]: (style) Checking if unsigned expression 'value' is less than zero.\n", errout.str());
}
void checkSignOfPointer() {
@ -5008,6 +5112,18 @@ private:
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) A pointer can not be negative so it is either pointless or an error to check if it is not.\n", errout.str());
{
const char code[] =
"bool foo(int* x) {\n"
" int y = 0;\n"
" if (x >= y)"
" bar();\n"
"}";
check(code, nullptr, false, false, true, false);
ASSERT_EQUALS("[test.cpp:3]: (style) A pointer can not be negative so it is either pointless or an error to check if it is not.\n", errout.str());
check(code, nullptr, false, false, true, true);
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) A pointer can not be negative so it is either pointless or an error to check if it is not.\n", errout.str());
}
check(
"bool foo(int* x) {\n"
" if (*x >= 0)"
@ -5022,6 +5138,20 @@ private:
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) A pointer can not be negative so it is either pointless or an error to check if it is.\n", errout.str());
{
const char code[] =
"bool foo(int* x) {\n"
" unsigned y = 0u;\n"
" if (x < y)"
" bar();\n"
"}";
check(code, nullptr, false, false, true, false);
ASSERT_EQUALS("[test.cpp:3]: (style) A pointer can not be negative so it is either pointless or an error to check if it is.\n", errout.str());
check(code, nullptr, false, false, true, true);
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) A pointer can not be negative so it is either pointless or an error to check if it is.\n", errout.str());
}
check(
"bool foo(int* x) {\n"
" if (*x < 0)"