diff --git a/lib/checkother.cpp b/lib/checkother.cpp index b9ff731b0ce..355881a92b1 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3513,9 +3513,11 @@ void CheckOtherImpl::redundantCopyError(const Token *tok,const std::string& varn // Checking for shift by negative values //--------------------------------------------------------------------------- -static bool isNegative(const Token *tok, const Settings &settings) +static const ValueFlow::Value* isNegative(const Token *tok, const Settings &settings) { - return tok->valueType() && tok->valueType()->sign == ValueType::SIGNED && tok->getValueLE(-1LL, settings); + if (!tok->valueType() || tok->valueType()->sign != ValueType::SIGNED) + return nullptr; + return tok->getValueLE(-1LL, settings); } void CheckOtherImpl::checkNegativeBitwiseShift() @@ -3551,24 +3553,28 @@ void CheckOtherImpl::checkNegativeBitwiseShift() if (ternary) continue; - // Get negative rhs value. preferably a value which doesn't have 'condition'. - if (portability && isNegative(tok->astOperand1(), mSettings)) - negativeBitwiseShiftError(tok, 1); - else if (isNegative(tok->astOperand2(), mSettings)) - negativeBitwiseShiftError(tok, 2); + const ValueFlow::Value* value = isNegative(tok->astOperand1(), mSettings); // lhs + if (portability && value) + negativeBitwiseShiftError(tok, true, value); + else { + value = isNegative(tok->astOperand2(), mSettings); // rhs + if (value) + negativeBitwiseShiftError(tok, false, value); + } } } -void CheckOtherImpl::negativeBitwiseShiftError(const Token *tok, int op) +void CheckOtherImpl::negativeBitwiseShiftError(const Token *tok, bool isLHS, const ValueFlow::Value* v) { - if (op == 1) - // LHS - this is used by intention in various software, if it - // is used often in a project and works as expected then this is - // a portability issue - reportError(tok, Severity::portability, "shiftNegativeLHS", "Shifting a negative value is technically undefined behaviour", CWE758, Certainty::normal); - else // RHS - reportError(tok, Severity::error, "shiftNegative", "Shifting by a negative value is undefined behaviour", CWE758, Certainty::normal); + // LHS - this is used by intention in various software, if it + // is used often in a project and works as expected then this is + // a portability issue + const char* id = isLHS ? "shiftNegativeLHS" : "shiftNegative"; + const std::string msg = isLHS ? "Shifting a negative value is technically undefined behaviour" : "Shifting by a negative value is undefined behaviour"; + const Severity severity = isLHS ? Severity::portability : (v && v->errorSeverity() && !v->conditional ? Severity::error : Severity::warning); + const ErrorPath errorPath = getErrorPath(tok, v, msg); + reportError(errorPath, severity, id, msg, CWE758, Certainty::normal); } //--------------------------------------------------------------------------- @@ -4928,8 +4934,8 @@ void CheckOther::getErrorMessages(ErrorLogger& errorLogger, const Settings &sett c.zerodivError(nullptr, nullptr); c.misusedScopeObjectError(nullptr, "varname"); c.invalidPointerCastError(nullptr, "float *", "double *", false, false); - c.negativeBitwiseShiftError(nullptr, 1); - c.negativeBitwiseShiftError(nullptr, 2); + c.negativeBitwiseShiftError(nullptr, true); + c.negativeBitwiseShiftError(nullptr, false); c.raceAfterInterlockedDecrementError(nullptr); c.invalidFreeError(nullptr, "malloc", false); c.overlappingWriteUnion(nullptr); diff --git a/lib/checkother.h b/lib/checkother.h index 823a5783fb3..61a865e9ab6 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -310,7 +310,7 @@ class CPPCHECKLIB CheckOtherImpl : public CheckImpl { 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 negativeBitwiseShiftError(const Token *tok, bool isLHS, const ValueFlow::Value *v = nullptr); void redundantCopyError(const Token *tok, const std::string &varname); void incompleteArrayFillError(const Token* tok, const std::string& buffer, const std::string& function, bool boolean); void varFuncNullUBError(const Token *tok); diff --git a/test/testother.cpp b/test/testother.cpp index d115acc2163..5af80d3117b 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -230,6 +230,7 @@ class TestOther : public TestFixture { TEST_CASE(checkRedundantCopy); TEST_CASE(checkNegativeShift); + TEST_CASE(checkNegativeShiftErrorPath); TEST_CASE(incompleteArrayFill); @@ -10490,6 +10491,24 @@ class TestOther : public TestFixture { ASSERT_EQUALS("", errout_str()); } + void checkNegativeShiftErrorPath() { + setMultiline(); + Settings s = settings1; + s.templateLocation = "{file}:{line}:note:{info}\n"; + + check("int f(int i, bool b) {\n" + " int s = -1;\n" + " if (b)\n" + " return i;\n" + " return i << s;\n" + "}\n", dinit(CheckOptions, $.settings = &s)); + ASSERT_EQUALS("[test.cpp:5:14]: warning: Shifting by a negative value is undefined behaviour [shiftNegative]\n" + "[test.cpp:2:14]: note: Assignment 's=-1', assigned value is -1\n" + "[test.cpp:3:9]: note: Assuming condition is false\n" + "[test.cpp:5:14]: note: Shifting by a negative value is undefined behaviour\n", + errout_str()); + } + void incompleteArrayFill() { check("void f() {\n" " int a[5];\n"