Skip to content

Fix #15000 (CheckCondition: detect compareValueOutOfTypeRange for bounded cast/arithmetic expressions) #8819

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jfdeverge wants to merge 1 commit into
base: main
Choose a base branch
from
+232 −0
Open

Fix #15000 (CheckCondition: detect compareValueOutOfTypeRange for bounded cast/arithmetic expressions) #8819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
186 changes: 186 additions & 0 deletions lib/checkcondition.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1972,6 +1972,172 @@ void CheckConditionImpl::assignmentInCondition(const Token *eq)
Certainty::normal);
}

static bool getIntegerTypeRange(const Token* tok,

@danmar danmar Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the feeling we have a utility function to determine min/max values of an integer type.

const Settings& settings,
MathLib::bigint& lower,
MathLib::bigint& upper)
{
if (!tok || !tok->valueType() || tok->valueType()->pointer)
return false;

const ValueType* const valueType = tok->valueType();
if (!valueType->isIntegral())
return false;

// bool can only hold 0 or 1, regardless of signedness
if (valueType->type == ValueType::Type::BOOL) {
lower = 0;
upper = 1;
return true;
}

const std::size_t bits = settings.platform.char_bit *
valueType->getSizeOf(settings, ValueType::Accuracy::ExactOrZero, ValueType::SizeOf::Pointee);
if (bits == 0 || bits > 64)
return false;

if (bits == 64 && valueType->sign == ValueType::Sign::UNSIGNED)
return false;
const MathLib::bigint max = bits == 64 ? std::numeric_limits<MathLib::bigint>::max() : (MathLib::bigint(1) << bits) - 1;
if (valueType->sign == ValueType::Sign::SIGNED) {
if (bits == 64) {
lower = std::numeric_limits<MathLib::bigint>::min();
upper = std::numeric_limits<MathLib::bigint>::max();
} else {
lower = -(MathLib::bigint(1) << (bits - 1));
upper = max / 2;
}
} else if (valueType->sign == ValueType::Sign::UNSIGNED) {
lower = 0;
upper = max;
} else {
return false; // unknown sign: do not guess the range
}
return true;
}

static bool getIntegerExpressionRange(const Token* tok,

@danmar danmar Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you checked if there is some similar utility function somewhere?

const Settings& settings,

@danmar danmar Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion but passing settings is overkill we could just pass a platform.

MathLib::bigint& lower,
MathLib::bigint& upper)
{
if (!tok)
return false;
if (tok->hasKnownIntValue()) {

@danmar danmar Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there is not a known value.. I think it would be good to consider the "impossible" values.
Example:

void foo(uint32_t x) {
    if (x > 100) return;
    a = (uint64_t)x;
}

In the cast the values for token x are (this is the --debug output):

  x {!<=-1,!>=101,<=100}

The !>=101 means x cannot have values 101 and more.

lower = upper = tok->getKnownIntValue();
return true;
}

if (tok->isCast() && tok->astOperand1()) {
MathLib::bigint typeLower;
MathLib::bigint typeUpper;
const bool hasSourceRange = getIntegerExpressionRange(tok->astOperand1(), settings, lower, upper);

@danmar danmar Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c++ casts have two operands.

const bool hasTypeRange = getIntegerTypeRange(tok, settings, typeLower, typeUpper);
if (!hasSourceRange && !hasTypeRange)
return false;
if (!hasSourceRange) {
lower = typeLower;
upper = typeUpper;
return true;
}
if (!hasTypeRange)
return true;
lower = std::max(lower, typeLower);
upper = std::min(upper, typeUpper);
return lower <= upper;
}

if (Token::simpleMatch(tok, "(") && tok->astOperand1())

@danmar danmar Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what exact code you target here.

return getIntegerExpressionRange(tok->astOperand1(), settings, lower, upper);

if (Token::Match(tok, "+|-")) {
MathLib::bigint operandLower;
MathLib::bigint operandUpper;
MathLib::bigint constantLower;
MathLib::bigint constantUpper;
if (!getIntegerExpressionRange(tok->astOperand1(), settings, operandLower, operandUpper) ||
!getIntegerExpressionRange(tok->astOperand2(), settings, constantLower, constantUpper) ||

@danmar danmar Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we will always return false for a unary +|- ?

constantLower != constantUpper)
return false;
const MathLib::bigint minimum = std::numeric_limits<MathLib::bigint>::min();
const MathLib::bigint maximum = std::numeric_limits<MathLib::bigint>::max();
if (tok->str() == "+" &&
((constantLower > 0 && operandUpper > maximum - constantLower) ||
(constantLower < 0 && operandLower < minimum - constantLower)))
return false;
if (tok->str() == "-" &&
((constantUpper > 0 && operandLower < minimum + constantUpper) ||
(constantUpper < 0 && operandUpper > maximum + constantUpper)))
return false;
if (tok->str() == "+") {
lower = operandLower + constantLower;
upper = operandUpper + constantUpper;
} else {
lower = operandLower - constantUpper;
upper = operandUpper - constantLower;
}
return true;
}

return getIntegerTypeRange(tok, settings, lower, upper);
}

static bool compareIntegerRange(const Token* comparison,
MathLib::bigint lower,
MathLib::bigint upper,
MathLib::bigint value,
bool& result)
{
if (!comparison || !comparison->isComparisonOp())
return false;
if (comparison->str() == "<") {
if (upper < value)
result = true;
else if (lower >= value)
result = false;
else
return false;
} else if (comparison->str() == "<=") {
if (upper <= value)
result = true;
else if (lower > value)
result = false;
else
return false;
} else if (comparison->str() == ">") {
if (lower > value)
result = true;
else if (upper <= value)
result = false;
else
return false;
} else if (comparison->str() == ">=") {
if (lower >= value)
result = true;
else if (upper < value)
result = false;
else
return false;
} else if (comparison->str() == "==") {
if (lower == upper)
result = (lower == value);
else if (value < lower || value > upper)
result = false;
else
return false;
} else if (comparison->str() == "!=") {
if (lower == upper)
result = (lower != value);
else if (value < lower || value > upper)
result = true;
else
return false;
} else {
return false;
}
return true;
}

void CheckConditionImpl::checkCompareValueOutOfTypeRange()
{
if (!mSettings.severity.isEnabled(Severity::style) && !mSettings.isPremiumEnabled("compareValueOutOfTypeRangeError"))
Expand All @@ -1988,6 +2154,26 @@ void CheckConditionImpl::checkCompareValueOutOfTypeRange()
if (!tok->isComparisonOp() || !tok->isBinaryOp())
continue;

for (int i = 0; i < 2; ++i) {
const Token* const expressionTok = (i == 0) ? tok->astOperand2() : tok->astOperand1();
const Token* const valueTok = (i == 0) ? tok->astOperand1() : tok->astOperand2();
if (!expressionTok || !valueTok || !valueTok->hasKnownIntValue() || expressionTok->hasKnownIntValue())
continue;
if (expressionTok->str() != "(" && !expressionTok->isCast() && !expressionTok->isArithmeticalOp())
continue;
MathLib::bigint lower;
MathLib::bigint upper;
if (!getIntegerExpressionRange(expressionTok, mSettings, lower, upper))
continue;
bool result;
if (!compareIntegerRange(tok, lower, upper, valueTok->getKnownIntValue(), result) || diag(tok))
continue;
compareValueOutOfTypeRangeError(valueTok,
expressionTok->valueType() ? expressionTok->valueType()->str() : "",
valueTok->getKnownIntValue(),
result);
}

for (int i = 0; i < 2; ++i) {
const Token * const valueTok = (i == 0) ? tok->astOperand1() : tok->astOperand2();
const Token * const typeTok = valueTok->astSibling();
Expand Down
46 changes: 46 additions & 0 deletions test/testcondition.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6546,6 +6546,52 @@ class TestCondition : public TestFixture {
"[test.cpp:4:13]: (style) Comparing expression of type 'const unsigned int &' against value 4294967295. Condition is always false. [compareValueOutOfTypeRangeError]\n",
errout_str());

check("typedef unsigned int uint32;\n"
"typedef unsigned long long uint64;\n"
"typedef long long sint64;\n"
"void f(uint32 x) {\n"
" uint64 tmp = ((uint64)x) + 1ULL;\n"
" if (tmp > 4294967295ULL)\n"
" tmp = 4294967295ULL;\n"
" if ((((sint64)((uint32)tmp)) - 1LL) < 0LL) {}\n"
" if ((((sint64)((uint32)tmp)) - 1LL) > 4294967295LL) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("[test.cpp:9:43]: (style) Comparing expression of type 'signed long long' against value 4294967295. Condition is always false. [compareValueOutOfTypeRangeError]\n",

@chrchr-github chrchr-github Sep 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text/ID of the warning seems incorrect. 4294967295 is within range for signed long long.

@jfdeverge jfdeverge Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello! The condition is essentially (sint64), the diagnostic log is correct. But the check is only possible because there is a (uint32) inside.

@danmar danmar Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm it will at least be confusing. At first glance it sounds like 'signed long long' expressions shouldn't be compared against 4294967295.

It might be less confusing if the type 'signed long long' would be removed. The range of 'signed long long' expressions is much greater, the issue here is that the expression range corresponds to a 'uint32'. How problematic would that be to remove the type if the expression range does match the type range?

errout_str());

// cast directly around variable: both the range-based and the declared-type
// analysis can prove the condition invariant; diag() must prevent duplicates
check("void f(unsigned char c) {\n"
" if ((unsigned char)c > 255) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("[test.cpp:2:28]: (style) Comparing expression of type 'unsigned char' against value 255. Condition is always false. [compareValueOutOfTypeRangeError]\n",
errout_str());

check("void f(unsigned char c) {\n"
" if ((unsigned char)c == 256) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("[test.cpp:2:29]: (style) Comparing expression of type 'unsigned char' against value 256. Condition is always false. [compareValueOutOfTypeRangeError]\n",
errout_str());

check("void f(unsigned int u) {\n"
" if ((unsigned int)u > 4294967295ULL) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("[test.cpp:2:27]: (style) Comparing expression of type 'unsigned int' against value 4294967295. Condition is always false. [compareValueOutOfTypeRangeError]\n",
errout_str());

check("void f(unsigned short s) {\n"
" if ((unsigned int)s > 4294967295ULL) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("[test.cpp:2:27]: (style) Comparing expression of type 'unsigned int' against value 4294967295. Condition is always false. [compareValueOutOfTypeRangeError]\n",
errout_str());

// wchar_t range is derived through ValueType::getSizeOf()
check("void f(wchar_t c) {\n"
" if ((wchar_t)c > 0x7fffffff) {}\n"
"}\n", settingsUnix64);
ASSERT_EQUALS("[test.cpp:2:22]: (style) Comparing expression of type 'signed wchar_t' against value 2147483647. Condition is always false. [compareValueOutOfTypeRangeError]\n",
errout_str());

check("void f() {\n"
" long long ll = 1024 * 1024 * 1024;\n"
" if (ll * 8 < INT_MAX) {}\n"
Expand Down
Loading