From faf80470509bd5281caf527e777f94bd620e9dea Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Wed, 2 Aug 2023 12:27:29 +0200 Subject: [PATCH] Fix FP truncLongCastReturn on Windows (#5262) --- lib/checktype.cpp | 73 +++++++++++++++++++++++++++++++++-------------- lib/checktype.h | 4 +-- lib/tokenize.cpp | 2 ++ lib/valueflow.cpp | 18 ++---------- test/testtype.cpp | 40 ++++++++++++++++++++++---- 5 files changed, 91 insertions(+), 46 deletions(-) diff --git a/lib/checktype.cpp b/lib/checktype.cpp index 7cc2d4d7643..6f793273392 100644 --- a/lib/checktype.cpp +++ b/lib/checktype.cpp @@ -289,6 +289,26 @@ void CheckType::signConversionError(const Token *tok, const ValueFlow::Value *ne //--------------------------------------------------------------------------- // Checking for long cast of int result const long x = var1 * var2; //--------------------------------------------------------------------------- +static bool checkTypeCombination(const ValueType& src, const ValueType& tgt, const Settings* settings) +{ + static const std::pair typeCombinations[] = { + { ValueType::Type::INT, ValueType::Type::LONG }, + { ValueType::Type::INT, ValueType::Type::LONGLONG }, + { ValueType::Type::LONG, ValueType::Type::LONGLONG }, + { ValueType::Type::FLOAT, ValueType::Type::DOUBLE }, + { ValueType::Type::FLOAT, ValueType::Type::LONGDOUBLE }, + { ValueType::Type::DOUBLE, ValueType::Type::LONGDOUBLE }, + }; + + const std::size_t sizeSrc = ValueFlow::getSizeOf(src, settings); + const std::size_t sizeTgt = ValueFlow::getSizeOf(tgt, settings); + if (!(sizeSrc > 0 && sizeTgt > 0 && sizeSrc < sizeTgt)) + return false; + + return std::any_of(std::begin(typeCombinations), std::end(typeCombinations), [&](const std::pair& p) { + return src.type == p.first && tgt.type == p.second; + }); +} void CheckType::checkLongCast() { @@ -311,16 +331,15 @@ void CheckType::checkLongCast() if (!lhstype || !rhstype) continue; + if (!checkTypeCombination(*rhstype, *lhstype, mSettings)) + continue; // assign int result to long/longlong const nonpointer? - if (rhstype->type == ValueType::Type::INT && - rhstype->pointer == 0U && + if (rhstype->pointer == 0U && rhstype->originalTypeName.empty() && - (lhstype->type == ValueType::Type::LONG || lhstype->type == ValueType::Type::LONGLONG) && lhstype->pointer == 0U && - lhstype->constness == 1U && lhstype->originalTypeName.empty()) - longCastAssignError(tok); + longCastAssignError(tok, rhstype, lhstype); } // Return.. @@ -329,16 +348,9 @@ void CheckType::checkLongCast() // function must return long data const Token * def = scope->classDef; - bool islong = false; - while (Token::Match(def, "%type%|::")) { - if (def->str() == "long" && def->originalName().empty()) { - islong = true; - break; - } - def = def->previous(); - } - if (!islong) + if (!Token::Match(def, "%name% (") || !def->next()->valueType()) continue; + const ValueType* retVt = def->next()->valueType(); // return statements const Token *ret = nullptr; @@ -346,7 +358,9 @@ void CheckType::checkLongCast() if (tok->str() == "return") { if (Token::Match(tok->astOperand1(), "<<|*")) { const ValueType *type = tok->astOperand1()->valueType(); - if (type && type->type == ValueType::Type::INT && type->pointer == 0U && type->originalTypeName.empty()) + if (type && checkTypeCombination(*type, *retVt, mSettings) && + type->pointer == 0U && + type->originalTypeName.empty()) ret = tok; } // All return statements must have problem otherwise no warning @@ -358,26 +372,41 @@ void CheckType::checkLongCast() } if (ret) - longCastReturnError(ret); + longCastReturnError(ret, ret->astOperand1()->valueType(), retVt); } } -void CheckType::longCastAssignError(const Token *tok) +static void makeBaseTypeString(std::string& typeStr) +{ + const auto pos = typeStr.find("signed"); + if (pos != std::string::npos) + typeStr.erase(typeStr.begin(), typeStr.begin() + pos + 6 + 1); +} + +void CheckType::longCastAssignError(const Token *tok, const ValueType* src, const ValueType* tgt) { + std::string srcStr = src ? src->str() : "int"; + makeBaseTypeString(srcStr); + std::string tgtStr = tgt ? tgt->str() : "long"; + makeBaseTypeString(tgtStr); reportError(tok, Severity::style, "truncLongCastAssignment", - "int result is assigned to long variable. If the variable is long to avoid loss of information, then you have loss of information.\n" - "int result is assigned to long variable. If the variable is long to avoid loss of information, then there is loss of information. To avoid loss of information you must cast a calculation operand to long, for example 'l = a * b;' => 'l = (long)a * b;'.", CWE197, Certainty::normal); + srcStr + " result is assigned to " + tgtStr + " variable. If the variable is " + tgtStr + " to avoid loss of information, then you have loss of information.\n" + + srcStr + " result is assigned to " + tgtStr + " variable. If the variable is " + tgtStr + " to avoid loss of information, then there is loss of information. To avoid loss of information you must cast a calculation operand to " + tgtStr + ", for example 'l = a * b;' => 'l = (" + tgtStr + ")a * b;'.", CWE197, Certainty::normal); } -void CheckType::longCastReturnError(const Token *tok) +void CheckType::longCastReturnError(const Token *tok, const ValueType* src, const ValueType* tgt) { + std::string srcStr = src ? src->str() : "int"; + makeBaseTypeString(srcStr); + std::string tgtStr = tgt ? tgt->str() : "long"; + makeBaseTypeString(tgtStr); reportError(tok, Severity::style, "truncLongCastReturn", - "int result is returned as long value. If the return value is long to avoid loss of information, then you have loss of information.\n" - "int result is returned as long value. If the return value is long to avoid loss of information, then there is loss of information. To avoid loss of information you must cast a calculation operand to long, for example 'return a*b;' => 'return (long)a*b'.", CWE197, Certainty::normal); + srcStr +" result is returned as " + tgtStr + " value. If the return value is " + tgtStr + " to avoid loss of information, then you have loss of information.\n" + + srcStr +" result is returned as " + tgtStr + " value. If the return value is " + tgtStr + " to avoid loss of information, then there is loss of information. To avoid loss of information you must cast a calculation operand to long, for example 'return a*b;' => 'return (long)a*b'.", CWE197, Certainty::normal); } //--------------------------------------------------------------------------- diff --git a/lib/checktype.h b/lib/checktype.h index 34417b371f6..e403bf16dd0 100644 --- a/lib/checktype.h +++ b/lib/checktype.h @@ -84,8 +84,8 @@ class CPPCHECKLIB CheckType : public Check { void tooBigSignedBitwiseShiftError(const Token *tok, int lhsbits, const ValueFlow::Value &rhsbits); void integerOverflowError(const Token *tok, const ValueFlow::Value &value); void signConversionError(const Token *tok, const ValueFlow::Value *negativeValue, const bool constvalue); - void longCastAssignError(const Token *tok); - void longCastReturnError(const Token *tok); + void longCastAssignError(const Token *tok, const ValueType* src = nullptr, const ValueType* tgt = nullptr); + void longCastReturnError(const Token *tok, const ValueType* src = nullptr, const ValueType* tgt = nullptr); void floatToIntegerOverflowError(const Token *tok, const ValueFlow::Value &value); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const override { diff --git a/lib/tokenize.cpp b/lib/tokenize.cpp index 1197430fc0f..9363dec6caf 100644 --- a/lib/tokenize.cpp +++ b/lib/tokenize.cpp @@ -3428,8 +3428,10 @@ void Tokenizer::fillTypeSizes() mTypeSize["short"] = mSettings->platform.sizeof_short; mTypeSize["int"] = mSettings->platform.sizeof_int; mTypeSize["long"] = mSettings->platform.sizeof_long; + mTypeSize["long long"] = mSettings->platform.sizeof_long_long; mTypeSize["float"] = mSettings->platform.sizeof_float; mTypeSize["double"] = mSettings->platform.sizeof_double; + mTypeSize["long double"] = mSettings->platform.sizeof_long_double; mTypeSize["wchar_t"] = mSettings->platform.sizeof_wchar_t; mTypeSize["size_t"] = mSettings->platform.sizeof_size_t; mTypeSize["*"] = mSettings->platform.sizeof_pointer; diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 128caef0e2c..cf3cdb56187 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -1091,29 +1091,15 @@ static void setTokenValueCast(Token *parent, const ValueType &valueType, const V static nonneg int getSizeOfType(const Token *typeTok, const Settings *settings) { const ValueType &valueType = ValueType::parseDecl(typeTok, *settings, true); // TODO: set isCpp - if (valueType.pointer > 0) - return settings->platform.sizeof_pointer; - if (valueType.type == ValueType::Type::BOOL || valueType.type == ValueType::Type::CHAR) - return 1; - if (valueType.type == ValueType::Type::SHORT) - return settings->platform.sizeof_short; - if (valueType.type == ValueType::Type::INT) - return settings->platform.sizeof_int; - if (valueType.type == ValueType::Type::LONG) - return settings->platform.sizeof_long; - if (valueType.type == ValueType::Type::LONGLONG) - return settings->platform.sizeof_long_long; - if (valueType.type == ValueType::Type::WCHAR_T) - return settings->platform.sizeof_wchar_t; - return 0; + return ValueFlow::getSizeOf(valueType, settings); } size_t ValueFlow::getSizeOf(const ValueType &vt, const Settings *settings) { if (vt.pointer) return settings->platform.sizeof_pointer; - if (vt.type == ValueType::Type::CHAR) + if (vt.type == ValueType::Type::BOOL || vt.type == ValueType::Type::CHAR) return 1; if (vt.type == ValueType::Type::SHORT) return settings->platform.sizeof_short; diff --git a/test/testtype.cpp b/test/testtype.cpp index b6ce658672d..c49ef7aae31 100644 --- a/test/testtype.cpp +++ b/test/testtype.cpp @@ -325,9 +325,19 @@ class TestType : public TestFixture { void longCastAssign() { const Settings settings = settingsBuilder().severity(Severity::style).platform(cppcheck::Platform::Type::Unix64).build(); + const Settings settingsWin = settingsBuilder().severity(Severity::style).platform(cppcheck::Platform::Type::Win64).build(); + + const char code[] = "long f(int x, int y) {\n" + " const long ret = x * y;\n" + " return ret;\n" + "}\n"; + check(code, settings); + ASSERT_EQUALS("[test.cpp:2]: (style) int result is assigned to long variable. If the variable is long to avoid loss of information, then you have loss of information.\n", errout.str()); + check(code, settingsWin); + ASSERT_EQUALS("", errout.str()); check("long f(int x, int y) {\n" - " const long ret = x * y;\n" + " long ret = x * y;\n" " return ret;\n" "}\n", settings); ASSERT_EQUALS("[test.cpp:2]: (style) int result is assigned to long variable. If the variable is long to avoid loss of information, then you have loss of information.\n", errout.str()); @@ -351,21 +361,39 @@ class TestType : public TestFixture { " return ret;\n" "}\n", settings); ASSERT_EQUALS("", errout.str()); + + check("double g(float f) {\n" + " return f * f;\n" + "}\n", settings); + ASSERT_EQUALS("[test.cpp:2]: (style) float result is returned as double value. If the return value is double to avoid loss of information, then you have loss of information.\n", + errout.str()); } void longCastReturn() { - const Settings settings = settingsBuilder().severity(Severity::style).build(); + const Settings settings = settingsBuilder().severity(Severity::style).platform(cppcheck::Platform::Type::Unix64).build(); + const Settings settingsWin = settingsBuilder().severity(Severity::style).platform(cppcheck::Platform::Type::Win64).build(); - check("long f(int x, int y) {\n" - " return x * y;\n" - "}\n", settings); + const char code[] = "long f(int x, int y) {\n" + " return x * y;\n" + "}\n"; + check(code, settings); ASSERT_EQUALS("[test.cpp:2]: (style) int result is returned as long value. If the return value is long to avoid loss of information, then you have loss of information.\n", errout.str()); + check(code, settingsWin); + ASSERT_EQUALS("", errout.str()); + + const char code2[] = "long long f(int x, int y) {\n" + " return x * y;\n" + "}\n"; + check(code2, settings); + ASSERT_EQUALS("[test.cpp:2]: (style) int result is returned as long long value. If the return value is long long to avoid loss of information, then you have loss of information.\n", errout.str()); + check(code2, settingsWin); + ASSERT_EQUALS("[test.cpp:2]: (style) int result is returned as long long value. If the return value is long long to avoid loss of information, then you have loss of information.\n", errout.str()); // typedef check("size_t f(int x, int y) {\n" " return x * y;\n" "}\n", settings); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) int result is returned as long value. If the return value is long to avoid loss of information, then you have loss of information.\n", errout.str()); } // This function ensure that test works with different compilers. Floats can