Repository navigation
Conversation
a1c4a25 to
8dc01be
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
8dc01be to
d37f6b1
Compare
| " 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", |
There was a problem hiding this comment.
The text/ID of the warning seems incorrect. 4294967295 is within range for signed long long.
There was a problem hiding this comment.
Hello! The condition is essentially (sint64), the diagnostic log is correct. But the check is only possible because there is a (uint32) inside.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Issue fixed in latest version, please re-review.
|
I have created this ticket: https://trac.cppcheck.net/ticket/15000 |
|
I will update the commit message with Fix #15000 ... |
d37f6b1 to
3e3fa63
Compare
| return false; | ||
|
|
||
| std::uint8_t bits = 0; | ||
| switch (tok->valueType()->type) { |
There was a problem hiding this comment.
maybe this switch could be simplified to something like
if (tok->valueType()->isIntegral())
bits = settings.platform.char_bit *
tok->valueType()->getSizeOf(settings, ValueType::Accuracy::ExactOrZero, ValueType::SizeOf::Pointee);
I think it would be shorter, duplicate less logic, and it would also support wchar_t
the check for isIntegral() could even be moved up to line 1980 to be very clear that this function only works for integral types
Cheers!
--Aaron
There was a problem hiding this comment.
Thanks for pointing to getSizeOf, it simplifies a lot.
| lower = -(MathLib::bigint(1) << (bits - 1)); | ||
| upper = max / 2; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
enum Sign : std::uint8_t { UNKNOWN_SIGN, SIGNED, UNSIGNED } sign = UNKNOWN_SIGN;
I think if the sign is unknown you should probably return false, not treat it the same as an UNSIGNED
3e3fa63 to
2d5f3a7
Compare
|
The PR title should be updated. The commit message will not be shown in the git log but the PR title will. |
jfdeverge
changed the title
Done. PR title updated to "Fix #15000 CheckCondition: detect compareValueOutOfTypeRange for bounded cast/arithmetic expressions" ; let me know if better convention would apply here. |
| if (tok->isCast() && tok->astOperand1()) { | ||
| MathLib::bigint typeLower; | ||
| MathLib::bigint typeUpper; | ||
| const bool hasSourceRange = getIntegerExpressionRange(tok->astOperand1(), settings, lower, upper); |
There was a problem hiding this comment.
c++ casts have two operands.
There was a problem hiding this comment.
Fixed in latest commit, please re-review.
| return lower <= upper; | ||
| } | ||
|
|
||
| if (Token::simpleMatch(tok, "(") && tok->astOperand1()) |
There was a problem hiding this comment.
I am not sure what exact code you target here.
There was a problem hiding this comment.
Removed. This is dead code.
| MathLib::bigint constantLower; | ||
| MathLib::bigint constantUpper; | ||
| if (!getIntegerExpressionRange(tok->astOperand1(), settings, operandLower, operandUpper) || | ||
| !getIntegerExpressionRange(tok->astOperand2(), settings, constantLower, constantUpper) || |
There was a problem hiding this comment.
we will always return false for a unary +|- ?
There was a problem hiding this comment.
Buggy and completely removed in latest version, please re-review.
| " 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", |
There was a problem hiding this comment.
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?
danmar
changed the title
Thanks! I don't have a strict pattern that must be followed but I typically put parentheses around the ticket title. Most important is that you indicated which ticket it solves.. |
| Certainty::normal); | ||
| } | ||
|
|
||
| static bool getIntegerTypeRange(const Token* tok, |
There was a problem hiding this comment.
I have the feeling we have a utility function to determine min/max values of an integer type.
There was a problem hiding this comment.
Replaced by stuffs from vf_common.cpp.
| return true; | ||
| } | ||
|
|
||
| static bool getIntegerExpressionRange(const Token* tok, |
There was a problem hiding this comment.
have you checked if there is some similar utility function somewhere?
There was a problem hiding this comment.
Replaced by stuffs from vf_common.cpp.
| } | ||
|
|
||
| static bool getIntegerExpressionRange(const Token* tok, | ||
| const Settings& settings, |
There was a problem hiding this comment.
I don't have a strong opinion but passing settings is overkill we could just pass a platform.
There was a problem hiding this comment.
Code is removed now.
| { | ||
| if (!tok) | ||
| return false; | ||
| if (tok->hasKnownIntValue()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think it is better now with the ValueFlow approach, please re-review.
|
I have a feeling that some of the logic should be performed in ValueFlow and not in the check. Currently, ValueFlow only seems to handle |
I agree that checkers should have some nice way to determine min/max values for an expression. If that is put in |
I meant that ValueFlow should determine that the condition is always true/false, instead of having complex parsing logic (in this case, for casts and arithmetic) in the checker. |
2d5f3a7 to
a8d48d3
Compare
jfdeverge
changed the title
| } else { | ||
| } else if (vt->sign == ValueType::Sign::SIGNED) { | ||
| minValue = LLONG_MIN; | ||
| maxValue = LLONG_MAX; |
There was a problem hiding this comment.
I don't understand how signed 64 bit and unsigned 64 bit have the same max value. Maybe for unsigned you meant ULLONG_MAX?
There was a problem hiding this comment.
Agreed, ULLONG_MAX is the true maximum for unsigned 64-bit values. getMinMaxValues currently returns bigint, which is signed and cannot represent ULLONG_MAX. I will update the legacy comment "todo max unsigned value" to "bigint cannot represent ULLONG_MAX; conservative max for unsigned 64-bit"
A biguint-based API can be introduced separately if the exact unsigned 64-bit maximum is needed ? A quick grep shows there will be some updates needed in some call-sites.
1f38959 to
1ebcbe5
Compare
1ebcbe5 to
1de33f6
Compare
|
I rebased against main (no conflicts). |
1de33f6 to
8502e53
Compare
|
I am working on a server that provides test reports: please take a look at the regressions. |
89e7250 to
cc643ff
Compare
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Summary of review:
🤖 Generated with Claude Code |
|
Test results for commit cc643ff (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8819/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
d2d6820 to
8104e70
Compare
jfdeverge
changed the title
f2ff7f7 to
fdaaa16
Compare
|
Test results for commit fdaaa16 (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8819/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found three new problems. Two are reproduced regressions against the merge base and one is a coverage gap. Earlier reviews did not raise any of them.
-
New
containerOutOfBoundsfalse positive on a common resize guard. Forif (v.size() < entries + 2) v.resize(entries + 2); v[0] = 1;the PR now warns thatvis empty. The PR does not fix this; it changes the expected output intest/teststl.cpp:529from""to the warning. The warning depends only onentries + 2wrapping to 0 or 1. The same happens withunsignedandn + 1u. -
The
negativeIndexbypass for induction variables (isNonnegativeInductionVariable) can hide real negative indexes. It only checks whether the loop body changes the variable. It does not look at the loop condition, sofor (int i = 0; i-- > -3; i++) a[i] = 0;is no longer reported. The base version reports it. -
The no-wrap check misses easy cases, which loses existing
knownConditionTrueFalseresults. WhenisUnsignedArithmeticNoWrapcannot prove that unsigned+/-/*does not wrap, the impossible bounds are dropped. This loses two correct warnings that the base version gives:unsigned s = c; if (s + 1u == 0u)withcanunsigned char;if (i + 1 == 0)insidefor (unsigned long i = 0; i < n; i++).
The tests check that the wrap cases are now silent, but none checks that diagnostics like these are kept.
The #15000 reproducer works. The PR also correctly removes the old false positive x - y > 100u "always false" when x < 10 && y < 20.
Testing
I built cppcheck and testrunner in pr/, and cppcheck in base/. TestCondition, TestBufferOverrun, TestStl, TestType, TestOther and TestValueFlow all pass (0 failures).
I compared PR and base on scratch files with cppcheck -q --enable=style,warning --platform=unix64 [--library=std] file.
containerOutOfBounds FP (new in the PR)
#include <vector>
#include <cstdint>
#include <cstddef>
void f(size_t entries) {
std::vector<uint8_t> v;
if (v.size() < entries + 2)
v.resize(entries + 2);
v[0] = 1;
}
void g(unsigned n) {
std::vector<int> v;
if (v.size() < n + 1u)
v.resize(n + 1u);
v[0] = 1;
}PR:
b.cpp:8:6: warning: Out of bounds access in expression 'v[0]' because 'v' is empty. [containerOutOfBounds]
b.cpp:6:18: note: Assuming condition is false
b.cpp:14:6: warning: Out of bounds access in expression 'v[0]' because 'v' is empty. [containerOutOfBounds]
Base gives no containerOutOfBounds warning here.
negativeIndex FN (new in the PR)
int a[10];
void f1(void) {
for (int i = 0; i-- > -3; i++) { a[i] = 0; }
}Base: error: Array 'a[10]' accessed at index -1, which is out of bounds. [negativeIndex]. PR: no warning.
knownConditionTrueFalse FNs (new in the PR)
void use(int);
void f4(unsigned long n) { for (unsigned long i = 0; i < n; i++) { if (i + 1 == 0) use(3); } }
void f6(unsigned char c) { unsigned s = c; if (s + 1u == 0u) use(4); }Base:
c.c:6:78: style: Condition 'i+1==0' is always false [knownConditionTrueFalse]
c.c:8:55: style: Condition 's+1u==0u' is always false [knownConditionTrueFalse]
PR: neither warning.
Other checks
- With the PR,
if (u < 10) { if (u + 1 > 20) ...}andif (u >= 5) { if (u - 1 == 0) ...}are still reported. - The #15000 pattern is reported.
- A ternary-guarded shift
x ? (1 << s) : 0withs = x ? 40 : 3still gives shiftTooManyBits, the same as base.
Automated review of fdaaa16 by Claude. The pull request was built and tested locally.
| const auto hasRangeBound = [](const Value& value) { | ||
| return (value.isIntValue() || value.isSymbolicValue()) && value.bound != Value::Bound::Point; | ||
| }; | ||
| if (skipUnsignedRangeBounds && (hasRangeBound(value1) || hasRangeBound(value2))) |
There was a problem hiding this comment.
AI review
When isUnsignedArithmeticNoWrap cannot prove that the operation does not wrap, all range bounds are dropped. That check misses simple cases, so correct diagnostics from base are lost:
void f6(unsigned char c) { unsigned s = c; if (s + 1u == 0u) use(4); }
void f4(unsigned long n) { for (unsigned long i = 0; i < n; i++) { if (i + 1 == 0) use(3); } }Base reports Condition 's+1u==0u' is always false and Condition 'i+1==0' is always false; the PR reports neither. Please add tests that check that such existing diagnostics are kept.
60124db to
ecf42f4
Compare
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
New uninitvar false positive: with the PR, an ordinary float or long double parameter is reported as uninitialized after a >=/<= comparison with a literal. The cause is the new float branch in SimpleConditionHandler::parse (lib/valueflow.cpp:5120). When the type is not exactly double, it uses ValueFlow::Value::unknown() as the false-branch value. That value has valueType == UNINIT. It is then marked possible, given the condition, and propagated into the else path, where CheckUninitVar reports it. Base gives no warning. float is common, so this would cause many FPs.
The earlier reviews did not raise this. Apart from it, I found no new problems in the time I had. The earlier review covered the containerOutOfBounds, negativeIndex and no-wrap issues. The current version adds a check of the loop condition in isNonnegativeInductionVariable.
Testing
I built cppcheck and testrunner in pr/, and cppcheck in base/. These suites all pass (0 failures): TestCondition, TestBufferOverrun, TestStl, TestType, TestOther, TestValueFlow, TestFunctions, TestUninitVar and TestNullPointer.
uninitvar FP (new in the PR)
fl.c:
float f1(float x) { if (x >= 1.0f) return 1; return x; }
float f5(float x) { if (x >= 1.0f) return 1; if (x < 1.0f) return 2; return x; }
long double g1(long double x) { if (x <= 2.0L) return 1; return x; }
float g2(float x) { if (1.0f <= x) return 1; return x * 2; }Command: cppcheck -q --enable=style,warning --platform=unix64 fl.c
Output with the PR:
fl.c:1:53: warning: Uninitialized variable: x [uninitvar]
fl.c:1:27: note: Assuming that condition 'x>=1.0f' is not redundant
...
fl.c:3:65: warning: Uninitialized variable: x [uninitvar]
fl.c:3:39: note: Assuming that condition 'x<=2.0L' is not redundant
fl.c:4:53: warning: Uninitialized variable: x [uninitvar]
fl.c:4:30: note: Assuming that condition '1.0f<=x' is not redundant
Base gives no warning for any of these. The same code with double gives no warning, because there the nextafter branch is taken.
Other checks
- Casts compared with out-of-range values (
(int32_t)x - 1 == -2147483649LL,(int8_t)x < -128,(_Bool)x > 1) give the same results as base. (uint8_t)x + 1 == 256is not reported, which is correct.
Automated review of ecf42f4 by Claude. The pull request was built and tested locally.
eb7b5bf to
4c9cc81
Compare
4c9cc81 to
023fb3b
Compare
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
New false positive: floatConversionOverflow. The new float >=/<= condition handler in SimpleConditionHandler::parse (lib/valueflow.cpp:5102) is the cause. It gives double variables possible values with an upper or lower bound, such as <= nextafter(1e10). A later >/< check on the same variable does not narrow these values, because floats are not handled there. checkFloatToIntegerOverflow reads every float value as a concrete value and ignores bound. So in a common clamp-before-cast pattern like if (d >= 1e10) return 0; if (d > 100.0) return 0; return (int)d; the PR reports float (1e+10) to integer conversion overflow, which cannot happen there. Base gives no warning. Before this PR float values never had bounds, so the checkers that consume float values were never written for them. Possible fixes: make float range values impossible values instead of possible ones, or audit the float consumers (floatConversionOverflow, invalidFunctionArg, …) before adding these values.
Apart from that I found no new problems. The integer parts I tested behave correctly. Some of them fix wrong results that base gives, for example unsigned y = 0; if ((y - 1u) == 0xFFFFFFFFu) and long long l = u - 5; if (l == 4294967294LL).
Testing
I built cppcheck and testrunner in pr/, and cppcheck in base/.
Test suites: these all pass with 0 failures: TestCondition, TestBufferOverrun, TestStl, TestType, TestOther, TestValueFlow, TestFunctions, TestUninitVar and TestNullPointer. In test/cli/other_test.py, -k floating gives 2 passed.
floatConversionOverflow false positive (new in the PR)
e.c:
int e1(double d) { if (d >= 1e10) return 0; if (d > 100.0) return 0; return (int)d; }
int e2(double d) { if (d >= 1e10) return 0; if (d < 0.0 || d > 100.0) return 0; return (int)d; }
int e4(double d) { if (d >= 1e10 || d > 100.0) return 0; return (int)d; }Command: cppcheck -q --enable=style,warning --platform=unix64 e.c
Output with the PR:
e.c:1:77: warning: Undefined behaviour: float (1e+10) to integer conversion overflow. [floatConversionOverflow]
e.c:1:26: note: Assuming that condition 'd>=1e10' is not redundant
e.c:2:88: warning: Undefined behaviour: float (1e+10) to integer conversion overflow. [floatConversionOverflow]
e.c:3:65: warning: Undefined behaviour: float (1e+10) to integer conversion overflow. [floatConversionOverflow]
Base gives no floatConversionOverflow for any of these. In each function d is at most 100 when the cast runs. A realistic saturation function shows the same false positive: if (d >= 1e10) return INT_MAX; if (d <= -1e10) return INT_MIN; if (d > 2147483647.0) return INT_MAX; if (d < -2147483648.0) return INT_MIN; return (int)d;.
Other checks, with results the same as base or improved
- Double
>=/<=conditions followed by<,>,!=,==,1.0/x,sqrtandacosgive no new warnings. if (x <= 1.0) use(x); return log(x - 1.0);gives a new invalidFunctionArg warning of the form "Either the condition is redundant or…". It is plausible.- Clamp patterns whose final bounds are set by
>=/<=(e.g.if (d >= 2147483648.0) … if (d <= -2147483649.0) … (int)d) are not reported, which is correct. - Unsigned point arithmetic: base reports
(y-1u)==0xFFFFFFFFuwithy = 0as always false, which is wrong. The PR correctly reports it as always true. Forlong long l = u - 5; l == 4294967294LL, base wrongly says always false and the PR correctly says always true.int i = u - 5; i < 0is still always true, which is correct.
Automated review of 023fb3b by Claude. The pull request was built and tested locally.
| return conds; | ||
|
|
||
| if (tok->isComparisonOp() && tok->astOperand1() && tok->astOperand2() && | ||
| (tok->str() == ">=" || tok->str() == "<=")) { |
There was a problem hiding this comment.
AI review
This gives double variables possible values with an upper or lower bound. Before this PR, float values never had bounds. checkFloatToIntegerOverflow (lib/checktype.cpp) ignores bound and reads the bounded value as a concrete value. Because </> on floats are not parsed here, a later stricter check does not remove the value. Result with the PR (base gives no warning):
int e1(double d) { if (d >= 1e10) return 0; if (d > 100.0) return 0; return (int)d; }warning: Undefined behaviour: float (1e+10) to integer conversion overflow. [floatConversionOverflow]
note: Assuming that condition 'd>=1e10' is not redundant
if (d >= 1e10 || d > 100.0) return 0; return (int)d; gives the same false positive. Either make these values impossible values, or make the float consumers bound-aware before adding them. Please also add a test for this.
6 participants
There was missing detection of 'MISRA C:2012 Rule 14.3" on some specific code structure generated by Matlab Simulink coder.
We identified these issues by comparing its results with reports previously generated by our company’s internal tool on the same software.
The bug report would have looked like below.
Problem
Cppcheck did not report a MISRA C:2012 Rule 14.3 violation when a comparison
used an integer expression whose range was provably bounded by casts and
constant arithmetic. The existing check handled a variable compared with a
value outside the variable's declared type range, but did not evaluate this
equivalent expression form.
Minimal Reproducer
After the clamp and casts, the left-hand expression cannot exceed
UINT32_MAX - 1. Therefore the final condition is always false and shouldproduce the
compareValueOutOfTypeRangeErrordiagnostic, which the MISRApostprocessor maps to Rule 14.3.
Observed Behavior
Before the fix, the final comparison produced no diagnostic. This allowed the
generated-code pattern to pass through the cppcheck-based MISRA pipeline
without the expected Rule 14.3 finding.