Repository navigation
Partial fix for #15048: Inconsistent error severity for nullpointer - #8961
chrchr-github wants to merge 2 commits into
Conversation
|
Test results for commit 3aa29a1 (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8961/ 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
There was a problem hiding this comment.
AI review
The PR is still inconsistent: whether nullPointer is an error or a warning depends on how the branch is written, not on how sure cppcheck is. value->conditional is set only when the forward analyzer goes through an if/else (or past an early return in one). The same definite dereference written with ?:, &&, for or while still gets error. For example, if (b) { return *p; } is now a warning, but b ? *p : 0 and while (b) { return *p; } stay errors. That is the same kind of inconsistency that #15048 is about.
Also, the downgraded findings keep the text "Null pointer dereference" rather than "Possible null pointer dereference". The same id and the same message now come at two severities. In the cases that change, p is null on every path that reaches the dereference, so the dereference is certain whenever it runs.
The unit tests pass, and I found no crash or hang.
Testing
Builds and tests
- I built
cppcheckandtestrunnerinpr/, andcppcheckinbase/. ./testrunner TestNullPointer: 142 tests, 0 failed. The full./testrunner: 5351 tests, 0 failed.pytest other_test.py sarif_test.py whole-program_test.pyintest/cli: 247 passed, 10 failed. The failures come from the environment, not the PR: cppcheck was built without rules support (--rule-file), and thetest_redundant_file_readstests fail withFileNotFoundError.
Comparing severities by control-flow shape (scratch/b.cpp):
int g1(bool b) {
int* p = nullptr;
if (b) { return 1; } else { return *p; }
}
int g2(bool b) {
int* p = nullptr;
return b ? *p : 0;
}
int g3(bool b) {
int* p = nullptr;
return b && *p;
}
int g4(int n) {
int* p = nullptr;
for (int i = 0; i < n; i++) { return *p; }
return 0;
}
int g5(bool b) {
int* p = nullptr;
do { if (b) return *p; } while (0);
return 0;
}
int g6(bool b) {
int* p = nullptr;
if (b) {}
return *p;
}
int g7(bool b) {
int* p = nullptr;
if (b) { int x = 0; (void)x; }
else { int y = 1; (void)y; }
return *p;
}
void g8(bool b) {
char* p = nullptr;
if (b) return;
p[0] = 0;
}Command: cppcheck -q --template='{line}:{severity}:{id}:{message}' b.cpp
== base
3:error:nullPointer:Null pointer dereference: p
7:error:nullPointer:Null pointer dereference: p
11:error:nullPointer:Null pointer dereference: p
15:error:nullPointer:Null pointer dereference: p
20:error:nullPointer:Null pointer dereference: p
26:error:nullPointer:Null pointer dereference: p
32:error:nullPointer:Null pointer dereference: p
37:error:nullPointer:Null pointer dereference: p
== pr
3:warning:nullPointer:Null pointer dereference: p
7:error:nullPointer:Null pointer dereference: p
11:error:nullPointer:Null pointer dereference: p
15:error:nullPointer:Null pointer dereference: p
20:warning:nullPointer:Null pointer dereference: p
26:error:nullPointer:Null pointer dereference: p
32:error:nullPointer:Null pointer dereference: p
37:warning:nullPointer:Null pointer dereference: p
What this shows:
- An
if/elsebranch and an early return in anifnow give a warning (lines 3, 20 and 37). ?:,&&andforstill give an error (lines 7, 11 and 15).- In
scratch/a.cpp,if (b) { return *p; }is now a warning, whilewhile (b) { return *p; }is still an error.
In every case the message still says "Null pointer dereference", not "Possible …".
Automated review of 3aa29a1 by Claude. The pull request was built and tested locally.
|
|
||
| reportError(std::move(errorPath), | ||
| value->isKnown() ? Severity::error : Severity::warning, | ||
| value->isKnown() && !value->conditional ? Severity::error : Severity::warning, |
There was a problem hiding this comment.
AI review
conditional is only set by makeConditional() when the forward analyzer assumes an if condition. So the severity now depends on how the code is written, not on how certain the value is:
int f(bool b) { int* p = nullptr; if (b) { return *p; } return 0; } // warning
int g(bool b) { int* p = nullptr; while (b) { return *p; } return 0; } // error
int h(bool b) { int* p = nullptr; return b ? *p : 0; } // error
int k(bool b) { int* p = nullptr; return b && *p; } // errorThe message on line 457 is still chosen from isKnown() alone, so these warnings say "Null pointer dereference" instead of "Possible null pointer dereference". The same id and the same text now come at two severities.
In all of these cases p is null on every path that reaches the dereference. If the goal is consistent severity, it should probably not depend on the conditional flag.
No description provided.