From c85f889bf2042fc53c7fa78ff407f0b9783ea305 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Berder?= <18538310+francois-berder@users.noreply.github.com> Date: Wed, 26 Jun 2024 18:01:41 +0200 Subject: [PATCH] Truncate value of increment operator (#6342) This PR fixes a similar bug than [#11591](https://trac.cppcheck.net/ticket/11591) fixed in PR https://github.com/danmar/cppcheck/pull/6331. --------- Signed-off-by: Francois Berder --- lib/valueflow.cpp | 14 ++------------ lib/vf_common.cpp | 14 ++++++++++++++ lib/vf_common.h | 2 ++ lib/vf_settokenvalue.cpp | 36 ++++++++++++++++++++++++++++++++---- test/testcondition.cpp | 15 +++++++++++++++ test/testvalueflow.cpp | 22 ++++++++++++++++++++-- 6 files changed, 85 insertions(+), 18 deletions(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index fe6b4d045d4..0bd0b70d16f 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -411,16 +411,6 @@ void ValueFlow::combineValueProperties(const ValueFlow::Value &value1, const Val result.path = value1.path; } -static long long truncateIntValue(long long value, size_t value_size, const ValueType::Sign dst_sign) -{ - const MathLib::biguint unsignedMaxValue = (1ULL << (value_size * 8)) - 1ULL; - const MathLib::biguint signBit = 1ULL << (value_size * 8 - 1); - value &= unsignedMaxValue; - if (dst_sign == ValueType::Sign::SIGNED && (value & signBit)) - value |= ~unsignedMaxValue; - - return value; -} template static size_t accumulateStructMembers(const Scope* scope, F f) @@ -1879,7 +1869,7 @@ struct ValueFlowAnalyzer : Analyzer { if (dst) { const size_t sz = ValueFlow::getSizeOf(*dst, settings); if (sz > 0 && sz < sizeof(MathLib::biguint)) { - long long newvalue = truncateIntValue(value->intvalue, sz, dst->sign); + long long newvalue = ValueFlow::truncateIntValue(value->intvalue, sz, dst->sign); /* Handle overflow/underflow for value bounds */ if (value->bound != ValueFlow::Value::Bound::Point) { @@ -5105,7 +5095,7 @@ static std::list truncateValues(std::list va } if (value.isIntValue() && sz > 0 && sz < sizeof(MathLib::biguint)) - value.intvalue = truncateIntValue(value.intvalue, sz, dst->sign); + value.intvalue = ValueFlow::truncateIntValue(value.intvalue, sz, dst->sign); } return values; } diff --git a/lib/vf_common.cpp b/lib/vf_common.cpp index 7505e89c36d..ae1f4a43d8f 100644 --- a/lib/vf_common.cpp +++ b/lib/vf_common.cpp @@ -93,6 +93,20 @@ namespace ValueFlow return true; } + long long truncateIntValue(long long value, size_t value_size, const ValueType::Sign dst_sign) + { + if (value_size == 0) + return value; + + const MathLib::biguint unsignedMaxValue = std::numeric_limits::max() >> ((sizeof(unsignedMaxValue) - value_size) * 8); + const MathLib::biguint signBit = 1ULL << (value_size * 8 - 1); + value &= unsignedMaxValue; + if (dst_sign == ValueType::Sign::SIGNED && (value & signBit)) + value |= ~unsignedMaxValue; + + return value; + } + static nonneg int getSizeOfType(const Token *typeTok, const Settings &settings) { const ValueType &valueType = ValueType::parseDecl(typeTok, settings); diff --git a/lib/vf_common.h b/lib/vf_common.h index 050b9960969..d67dfc6e722 100644 --- a/lib/vf_common.h +++ b/lib/vf_common.h @@ -35,6 +35,8 @@ namespace ValueFlow { bool getMinMaxValues(const ValueType* vt, const Platform& platform, MathLib::bigint& minValue, MathLib::bigint& maxValue); + long long truncateIntValue(long long value, size_t value_size, const ValueType::Sign dst_sign); + Token * valueFlowSetConstantValue(Token *tok, const Settings &settings); Value castValue(Value value, const ValueType::Sign sign, nonneg int bit); diff --git a/lib/vf_settokenvalue.cpp b/lib/vf_settokenvalue.cpp index b869c5427fa..4bd7bfd8f37 100644 --- a/lib/vf_settokenvalue.cpp +++ b/lib/vf_settokenvalue.cpp @@ -640,8 +640,22 @@ namespace ValueFlow continue; Value v(val); if (parent == tok->previous()) { - if (v.isIntValue() || v.isSymbolicValue()) - v.intvalue = v.intvalue + 1; + if (v.isIntValue() || v.isSymbolicValue()) { + const ValueType *dst = tok->valueType(); + if (dst) { + const size_t sz = ValueFlow::getSizeOf(*dst, settings); + long long newvalue = ValueFlow::truncateIntValue(v.intvalue + 1, sz, dst->sign); + if (v.bound != ValueFlow::Value::Bound::Point) { + if (newvalue < v.intvalue) { + v.invertBound(); + newvalue -= 2; + } + } + v.intvalue = newvalue; + } else { + v.intvalue = v.intvalue + 1; + } + } else v.floatValue = v.floatValue + 1.0; } @@ -656,8 +670,22 @@ namespace ValueFlow continue; Value v(val); if (parent == tok->previous()) { - if (v.isIntValue() || v.isSymbolicValue()) - v.intvalue = v.intvalue - 1; + if (v.isIntValue() || v.isSymbolicValue()) { + const ValueType *dst = tok->valueType(); + if (dst) { + const size_t sz = ValueFlow::getSizeOf(*dst, settings); + long long newvalue = ValueFlow::truncateIntValue(v.intvalue - 1, sz, dst->sign); + if (v.bound != ValueFlow::Value::Bound::Point) { + if (newvalue > v.intvalue) { + v.invertBound(); + newvalue += 2; + } + } + v.intvalue = newvalue; + } else { + v.intvalue = v.intvalue - 1; + } + } else v.floatValue = v.floatValue - 1.0; } diff --git a/test/testcondition.cpp b/test/testcondition.cpp index e7709c7fa5e..8808699bff2 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -123,6 +123,7 @@ class TestCondition : public TestFixture { TEST_CASE(knownConditionCast); // #9976 TEST_CASE(knownConditionIncrementLoop); // #9808 TEST_CASE(knownConditionAfterBailout); // #12526 + TEST_CASE(knownConditionIncDecOperator); } #define check(...) check_(__FILE__, __LINE__, __VA_ARGS__) @@ -6077,6 +6078,20 @@ class TestCondition : public TestFixture { ); ASSERT_EQUALS("[test.cpp:13] -> [test.cpp:18]: (style) Condition 'mS.b' is always false\n", errout_str()); } + + void knownConditionIncDecOperator() { + check( + "void f() {\n" + " unsigned int d = 0;\n" + " for (int i = 0; i < 4; ++i) {\n" + " if (i < 3)\n" + " ++d;\n" + " else if (--d == 0)\n" + " ;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + } }; REGISTER_TEST(TestCondition) diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index f417a643b4a..b39822365e3 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -150,7 +150,7 @@ class TestValueFlow : public TestFixture { TEST_CASE(valueFlowIdempotent); TEST_CASE(valueFlowUnsigned); TEST_CASE(valueFlowMod); - TEST_CASE(valueFlowInc); + TEST_CASE(valueFlowIncDec); TEST_CASE(valueFlowNotNull); TEST_CASE(valueFlowSymbolic); TEST_CASE(valueFlowSymbolicIdentity); @@ -7925,7 +7925,7 @@ class TestValueFlow : public TestFixture { ASSERT_EQUALS(false, testValueOfXImpossible(code, 3U, 1)); } - void valueFlowInc() { + void valueFlowIncDec() { const char *code; std::list values; @@ -7939,6 +7939,24 @@ class TestValueFlow : public TestFixture { values = tokenValues(code, "i ]"); ASSERT_EQUALS(1U, values.size()); ASSERT_EQUALS(0LLU, values.back().intvalue); + + code = "int f() {\n" + " const int a[1] = {};\n" + " unsigned char i = 255;\n" + " return a[++i];\n" + "}\n"; + values = tokenValues(code, "++"); + ASSERT_EQUALS(1U, values.size()); + ASSERT_EQUALS(0LLU, values.back().intvalue); + + code = "int f() {\n" + " const int a[128] = {};\n" + " char b = -128;\n" + " return a[--b];\n" + "}\n"; + values = tokenValues(code, "--"); + ASSERT_EQUALS(1U, values.size()); + ASSERT_EQUALS(127LLU, values.back().intvalue); } void valueFlowNotNull()