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

Add error path to more diagnostics that rely on isSameExpression (#1342)

This commit is contained in:
Paul Fultz II 2018-08-17 02:25:07 -05:00 committed by Daniel Marjamäki
parent 13617375df
commit bbf876256c
6 changed files with 50 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
lib/astutils.cpp
View File

@ -393,7 +393,7 @@ static bool isZeroBoundCond(const Token * const cond)
return false; return false;
} }
bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token * const cond2, const Library& library, bool pure) bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token * const cond2, const Library& library, bool pure, ErrorPath* errors)
{ {
if (!cond1 || !cond2) if (!cond1 || !cond2)
return false; return false;
@ -401,11 +401,11 @@ bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token
if (cond1->str() == "!") { if (cond1->str() == "!") {
if (cond2->str() == "!=") { if (cond2->str() == "!=") {
if (cond2->astOperand1() && cond2->astOperand1()->str() == "0") if (cond2->astOperand1() && cond2->astOperand1()->str() == "0")
return isSameExpression(cpp, true, cond1->astOperand1(), cond2->astOperand2(), library, pure); return isSameExpression(cpp, true, cond1->astOperand1(), cond2->astOperand2(), library, pure, errors);
if (cond2->astOperand2() && cond2->astOperand2()->str() == "0") if (cond2->astOperand2() && cond2->astOperand2()->str() == "0")
return isSameExpression(cpp, true, cond1->astOperand1(), cond2->astOperand1(), library, pure); return isSameExpression(cpp, true, cond1->astOperand1(), cond2->astOperand1(), library, pure, errors);
} }
return isSameExpression(cpp, true, cond1->astOperand1(), cond2, library, pure); return isSameExpression(cpp, true, cond1->astOperand1(), cond2, library, pure, errors);
} }
if (cond2->str() == "!") if (cond2->str() == "!")
@ -413,9 +413,9 @@ bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token
if (!isNot) { if (!isNot) {
if (cond1->str() == "==" && cond2->str() == "==") { if (cond1->str() == "==" && cond2->str() == "==") {
if (isSameExpression(cpp, true, cond1->astOperand1(), cond2->astOperand1(), library, pure)) if (isSameExpression(cpp, true, cond1->astOperand1(), cond2->astOperand1(), library, pure, errors))
return isDifferentKnownValues(cond1->astOperand2(), cond2->astOperand2()); return isDifferentKnownValues(cond1->astOperand2(), cond2->astOperand2());
if (isSameExpression(cpp, true, cond1->astOperand2(), cond2->astOperand2(), library, pure)) if (isSameExpression(cpp, true, cond1->astOperand2(), cond2->astOperand2(), library, pure, errors))
return isDifferentKnownValues(cond1->astOperand1(), cond2->astOperand1()); return isDifferentKnownValues(cond1->astOperand1(), cond2->astOperand1());
} }
// TODO: Handle reverse conditions // TODO: Handle reverse conditions
@ -480,7 +480,7 @@ bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token
if (!expr1 || !value1 || !expr2 || !value2) { if (!expr1 || !value1 || !expr2 || !value2) {
return false; return false;
} }
if (!isSameExpression(cpp, true, expr1, expr2, library, pure)) if (!isSameExpression(cpp, true, expr1, expr2, library, pure, errors))
return false; return false;
const ValueFlow::Value &rhsValue1 = value1->values().front(); const ValueFlow::Value &rhsValue1 = value1->values().front();
@ -508,16 +508,16 @@ bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token
))); )));
} }
bool isOppositeExpression(bool cpp, const Token * const tok1, const Token * const tok2, const Library& library, bool pure) bool isOppositeExpression(bool cpp, const Token * const tok1, const Token * const tok2, const Library& library, bool pure, ErrorPath* errors)
{ {
if (!tok1 || !tok2) if (!tok1 || !tok2)
return false; return false;
if (isOppositeCond(true, cpp, tok1, tok2, library, pure)) if (isOppositeCond(true, cpp, tok1, tok2, library, pure))
return true; return true;
if (tok1->isUnaryOp("-")) if (tok1->isUnaryOp("-"))
return isSameExpression(cpp, true, tok1->astOperand1(), tok2, library, pure); return isSameExpression(cpp, true, tok1->astOperand1(), tok2, library, pure, errors);
if (tok2->isUnaryOp("-")) if (tok2->isUnaryOp("-"))
return isSameExpression(cpp, true, tok2->astOperand1(), tok1, library, pure); return isSameExpression(cpp, true, tok2->astOperand1(), tok1, library, pure, errors);
return false; return false;
} }

4
lib/astutils.h
View File

@ -71,9 +71,9 @@ bool isDifferentKnownValues(const Token * const tok1, const Token * const tok2);
* @param library files data * @param library files data
* @param pure * @param pure
*/ */
bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token * const cond2, const Library& library, bool pure); bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token * const cond2, const Library& library, bool pure, ErrorPath* errors=nullptr);
bool isOppositeExpression(bool cpp, const Token * const tok1, const Token * const tok2, const Library& library, bool pure); bool isOppositeExpression(bool cpp, const Token * const tok1, const Token * const tok2, const Library& library, bool pure, ErrorPath* errors=nullptr);
bool isConstExpression(const Token *tok, const Library& library, bool pure); bool isConstExpression(const Token *tok, const Library& library, bool pure);

41
lib/checkcondition.cpp
View File

@ -565,6 +565,8 @@ void CheckCondition::multiCondition2()
// Condition.. // Condition..
const Token *cond2 = tok->next()->astOperand2(); const Token *cond2 = tok->next()->astOperand2();
ErrorPath errorPath;
if (type == MULTICONDITIONTYPE::INNER) { if (type == MULTICONDITIONTYPE::INNER) {
std::stack<const Token *> tokens1; std::stack<const Token *> tokens1;
tokens1.push(cond1); tokens1.push(cond1);
@ -576,11 +578,11 @@ void CheckCondition::multiCondition2()
if (firstCondition->str() == "&&") { if (firstCondition->str() == "&&") {
tokens1.push(firstCondition->astOperand1()); tokens1.push(firstCondition->astOperand1());
tokens1.push(firstCondition->astOperand2()); tokens1.push(firstCondition->astOperand2());
} else if (isOppositeCond(false, mTokenizer->isCPP(), firstCondition, cond2, mSettings->library, true)) { } else if (isOppositeCond(false, mTokenizer->isCPP(), firstCondition, cond2, mSettings->library, true, &errorPath)) {
if (!isAliased(vars)) if (!isAliased(vars))
oppositeInnerConditionError(firstCondition, cond2); oppositeInnerConditionError(firstCondition, cond2, errorPath);
} else if (isSameExpression(mTokenizer->isCPP(), true, firstCondition, cond2, mSettings->library, true)) { } else if (isSameExpression(mTokenizer->isCPP(), true, firstCondition, cond2, mSettings->library, true, &errorPath)) {
identicalInnerConditionError(firstCondition, cond2); identicalInnerConditionError(firstCondition, cond2, errorPath);
} }
} }
} else { } else {
@ -594,9 +596,9 @@ void CheckCondition::multiCondition2()
if (secondCondition->str() == "||" || secondCondition->str() == "&&") { if (secondCondition->str() == "||" || secondCondition->str() == "&&") {
tokens2.push(secondCondition->astOperand1()); tokens2.push(secondCondition->astOperand1());
tokens2.push(secondCondition->astOperand2()); tokens2.push(secondCondition->astOperand2());
} else if (isSameExpression(mTokenizer->isCPP(), true, cond1, secondCondition, mSettings->library, true)) { } else if (isSameExpression(mTokenizer->isCPP(), true, cond1, secondCondition, mSettings->library, true, &errorPath)) {
if (!isAliased(vars)) if (!isAliased(vars))
identicalConditionAfterEarlyExitError(cond1, secondCondition); identicalConditionAfterEarlyExitError(cond1, secondCondition, errorPath);
} }
} }
} }
@ -674,39 +676,36 @@ void CheckCondition::multiCondition2()
} }
} }
void CheckCondition::oppositeInnerConditionError(const Token *tok1, const Token* tok2) void CheckCondition::oppositeInnerConditionError(const Token *tok1, const Token* tok2, ErrorPath errorPath)
{ {
const std::string s1(tok1 ? tok1->expressionString() : "x"); const std::string s1(tok1 ? tok1->expressionString() : "x");
const std::string s2(tok2 ? tok2->expressionString() : "!x"); const std::string s2(tok2 ? tok2->expressionString() : "!x");
ErrorPath errorPath = { errorPath.emplace_back(ErrorPathItem(tok1, "outer condition: " + s1));
ErrorPathItem(tok1, "outer condition: " + s1), errorPath.emplace_back(ErrorPathItem(tok2, "opposite inner condition: " + s2));
ErrorPathItem(tok2, "opposite inner condition: " + s2)
};
const std::string msg("Opposite inner 'if' condition leads to a dead code block.\n" const std::string msg("Opposite inner 'if' condition leads to a dead code block.\n"
"Opposite inner 'if' condition leads to a dead code block (outer condition is '" + s1 + "' and inner condition is '" + s2 + "')."); "Opposite inner 'if' condition leads to a dead code block (outer condition is '" + s1 + "' and inner condition is '" + s2 + "').");
reportError(errorPath, Severity::warning, "oppositeInnerCondition", msg, CWE398, false); reportError(errorPath, Severity::warning, "oppositeInnerCondition", msg, CWE398, false);
} }
void CheckCondition::identicalInnerConditionError(const Token *tok1, const Token* tok2) void CheckCondition::identicalInnerConditionError(const Token *tok1, const Token* tok2, ErrorPath errorPath)
{ {
const std::string s1(tok1 ? tok1->expressionString() : "x"); const std::string s1(tok1 ? tok1->expressionString() : "x");
const std::string s2(tok2 ? tok2->expressionString() : "x"); const std::string s2(tok2 ? tok2->expressionString() : "x");
ErrorPath errorPath = { errorPath.emplace_back(ErrorPathItem(tok1, "outer condition: " + s1));
ErrorPathItem(tok1, "outer condition: " + s1), errorPath.emplace_back(ErrorPathItem(tok2, "identical inner condition: " + s2));
ErrorPathItem(tok2, "identical inner condition: " + s2)
};
const std::string msg("Identical inner 'if' condition is always true.\n" const std::string msg("Identical inner 'if' condition is always true.\n"
"Identical inner 'if' condition is always true (outer condition is '" + s1 + "' and inner condition is '" + s2 + "')."); "Identical inner 'if' condition is always true (outer condition is '" + s1 + "' and inner condition is '" + s2 + "').");
reportError(errorPath, Severity::warning, "identicalInnerCondition", msg, CWE398, false); reportError(errorPath, Severity::warning, "identicalInnerCondition", msg, CWE398, false);
} }
void CheckCondition::identicalConditionAfterEarlyExitError(const Token *cond1, const Token* cond2) void CheckCondition::identicalConditionAfterEarlyExitError(const Token *cond1, const Token* cond2, ErrorPath errorPath)
{ {
const std::string cond(cond1 ? cond1->expressionString() : "x"); const std::string cond(cond1 ? cond1->expressionString() : "x");
ErrorPath errorPath = { errorPath.emplace_back(ErrorPathItem(cond1, "first condition"));
ErrorPathItem(cond1, "first condition"), errorPath.emplace_back(ErrorPathItem(cond2, "second condition"));
ErrorPathItem(cond2, "second condition")
};
reportError(errorPath, Severity::warning, "identicalConditionAfterEarlyExit", "Identical condition '" + cond + "', second condition is always false", CWE398, false); reportError(errorPath, Severity::warning, "identicalConditionAfterEarlyExit", "Identical condition '" + cond + "', second condition is always false", CWE398, false);
} }

14
lib/checkcondition.h
View File

@ -131,11 +131,11 @@ private:
bool result); bool result);
void multiConditionError(const Token *tok, unsigned int line1); void multiConditionError(const Token *tok, unsigned int line1);
void oppositeInnerConditionError(const Token *tok1, const Token* tok2); void oppositeInnerConditionError(const Token *tok1, const Token* tok2, ErrorPath errorPath);
void identicalInnerConditionError(const Token *tok1, const Token* tok2); void identicalInnerConditionError(const Token *tok1, const Token* tok2, ErrorPath errorPath);
void identicalConditionAfterEarlyExitError(const Token *cond1, const Token *cond2); void identicalConditionAfterEarlyExitError(const Token *cond1, const Token *cond2, ErrorPath errorPath);
void incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always, bool inconclusive); void incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always, bool inconclusive);
void redundantConditionError(const Token *tok, const std::string &text, bool inconclusive); void redundantConditionError(const Token *tok, const std::string &text, bool inconclusive);
@ -152,14 +152,16 @@ private:
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const override { void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const override {
CheckCondition c(nullptr, settings, errorLogger); CheckCondition c(nullptr, settings, errorLogger);
ErrorPath errorPath;
c.assignIfError(nullptr, nullptr, emptyString, false); c.assignIfError(nullptr, nullptr, emptyString, false);
c.badBitmaskCheckError(nullptr); c.badBitmaskCheckError(nullptr);
c.comparisonError(nullptr, "&", 6, "==", 1, false); c.comparisonError(nullptr, "&", 6, "==", 1, false);
c.multiConditionError(nullptr,1); c.multiConditionError(nullptr,1);
c.mismatchingBitAndError(nullptr, 0xf0, nullptr, 1); c.mismatchingBitAndError(nullptr, 0xf0, nullptr, 1);
c.oppositeInnerConditionError(nullptr, nullptr); c.oppositeInnerConditionError(nullptr, nullptr, errorPath);
c.identicalInnerConditionError(nullptr, nullptr); c.identicalInnerConditionError(nullptr, nullptr, errorPath);
c.identicalConditionAfterEarlyExitError(nullptr, nullptr); c.identicalConditionAfterEarlyExitError(nullptr, nullptr, errorPath);
c.incorrectLogicOperatorError(nullptr, "foo > 3 && foo < 4", true, false); c.incorrectLogicOperatorError(nullptr, "foo > 3 && foo < 4", true, false);
c.redundantConditionError(nullptr, "If x > 11 the condition x > 10 is always true.", false); c.redundantConditionError(nullptr, "If x > 11 the condition x > 10 is always true.", false);
c.moduloAlwaysTrueFalseError(nullptr, "1"); c.moduloAlwaysTrueFalseError(nullptr, "1");

13
lib/checkother.cpp
View File

@ -2005,10 +2005,10 @@ void CheckOther::checkDuplicateExpression()
} }
} }
} else if (styleEnabled && } else if (styleEnabled &&
isOppositeExpression(mTokenizer->isCPP(), tok->astOperand1(), tok->astOperand2(), mSettings->library, false) && isOppositeExpression(mTokenizer->isCPP(), tok->astOperand1(), tok->astOperand2(), mSettings->library, false, &errorPath) &&
!Token::Match(tok, "=|-|-=|/|/=") && !Token::Match(tok, "=|-|-=|/|/=") &&
isWithoutSideEffects(mTokenizer->isCPP(), tok->astOperand1())) { isWithoutSideEffects(mTokenizer->isCPP(), tok->astOperand1())) {
oppositeExpressionError(tok, tok, tok->str()); oppositeExpressionError(tok, tok, tok->str(), errorPath);
} else if (!Token::Match(tok, "[-/%]")) { // These operators are not associative } else if (!Token::Match(tok, "[-/%]")) { // These operators are not associative
if (styleEnabled && tok->astOperand2() && tok->str() == tok->astOperand1()->str() && isSameExpression(mTokenizer->isCPP(), true, tok->astOperand2(), tok->astOperand1()->astOperand2(), mSettings->library, true, &errorPath) && isWithoutSideEffects(mTokenizer->isCPP(), tok->astOperand2())) if (styleEnabled && tok->astOperand2() && tok->str() == tok->astOperand1()->str() && isSameExpression(mTokenizer->isCPP(), true, tok->astOperand2(), tok->astOperand1()->astOperand2(), mSettings->library, true, &errorPath) && isWithoutSideEffects(mTokenizer->isCPP(), tok->astOperand2()))
duplicateExpressionError(tok->astOperand2(), tok->astOperand1()->astOperand2(), tok, errorPath); duplicateExpressionError(tok->astOperand2(), tok->astOperand1()->astOperand2(), tok, errorPath);
@ -2037,11 +2037,14 @@ void CheckOther::checkDuplicateExpression()
} }
} }
void CheckOther::oppositeExpressionError(const Token *tok1, const Token *tok2, const std::string &op) void CheckOther::oppositeExpressionError(const Token *tok1, const Token *tok2, const std::string &op, ErrorPath errors)
{ {
const std::list<const Token *> toks = { tok2, tok1 }; if(tok1)
errors.emplace_back(tok1, "");
if(tok2)
errors.emplace_back(tok2, "");
reportError(toks, Severity::style, "oppositeExpression", "Opposite expression on both sides of \'" + op + "\'.\n" reportError(errors, Severity::style, "oppositeExpression", "Opposite expression on both sides of \'" + op + "\'.\n"
"Finding the opposite expression on both sides of an operator is suspicious and might " "Finding the opposite expression on both sides of an operator is suspicious and might "
"indicate a cut and paste or logic error. Please examine this code carefully to " "indicate a cut and paste or logic error. Please examine this code carefully to "
"determine if it is correct.", CWE398, false); "determine if it is correct.", CWE398, false);

4
lib/checkother.h
View File

@ -239,7 +239,7 @@ private:
void misusedScopeObjectError(const Token *tok, const std::string &varname); void misusedScopeObjectError(const Token *tok, const std::string &varname);
void duplicateBranchError(const Token *tok1, const Token *tok2); void duplicateBranchError(const Token *tok1, const Token *tok2);
void duplicateAssignExpressionError(const Token *tok1, const Token *tok2); void duplicateAssignExpressionError(const Token *tok1, const Token *tok2);
void oppositeExpressionError(const Token *tok1, const Token *tok2, const std::string &op); void oppositeExpressionError(const Token *tok1, const Token *tok2, const std::string &op, ErrorPath errors);
void duplicateExpressionError(const Token *tok1, const Token *tok2, const Token *opTok, ErrorPath errors); void duplicateExpressionError(const Token *tok1, const Token *tok2, const Token *opTok, ErrorPath errors);
void duplicateValueTernaryError(const Token *tok); void duplicateValueTernaryError(const Token *tok);
void duplicateExpressionTernaryError(const Token *tok); void duplicateExpressionTernaryError(const Token *tok);
@ -302,7 +302,7 @@ private:
c.clarifyCalculationError(nullptr, "+"); c.clarifyCalculationError(nullptr, "+");
c.clarifyStatementError(nullptr); c.clarifyStatementError(nullptr);
c.duplicateBranchError(nullptr, nullptr); c.duplicateBranchError(nullptr, nullptr);
c.oppositeExpressionError(nullptr, nullptr, "&&"); c.oppositeExpressionError(nullptr, nullptr, "&&", errorPath);
c.duplicateExpressionError(nullptr, nullptr, nullptr, errorPath); c.duplicateExpressionError(nullptr, nullptr, nullptr, errorPath);
c.duplicateValueTernaryError(nullptr); c.duplicateValueTernaryError(nullptr);
c.duplicateExpressionTernaryError(nullptr); c.duplicateExpressionTernaryError(nullptr);