[analyzer] Remove impossible BugType from CStringChecker #152163
Conversation
CStringChecker had an AdditionOverflow bug type which was intended for a situation where the analyzer concludes that the addition of two size/length values overflows `size_t`. I strongly suspect that the analyzer could emit bugs of this type in certain complex corner cases (e.g. due to inaccurate cast modeling), but these reports would be all false positives because in the real world the sum of two size/length values is always far below SIZE_MAX. (Although note that there was no test where the analyzer emitted a bug with this type.) To simplify the code (and perhaps eliminate false positives), I eliminated this bug type and replaced code that emits it by a simple `addSink()` call (because we still want to get rid of the execution paths where the analyzer has invalid assumptions).
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Donát Nagy (NagyDonat) ChangesCStringChecker had an AdditionOverflow bug type which was intended for a situation where the analyzer concludes that the addition of two size/length values overflows I strongly suspect that the analyzer could emit bugs of this type in certain complex corner cases (e.g. due to inaccurate cast modeling), but these reports would be all false positives because in the real world the sum of two size/length values is always far below SIZE_MAX. (Although note that there was no test where the analyzer emitted a bug with this type.) To simplify the code (and perhaps eliminate false positives), I eliminated this bug type and replaced code that emits it by a simple Full diff: https://github.com/llvm/llvm-project/pull/152163.diff 1 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
index fd0a3982bf461..0e5fc0a074938 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -97,10 +97,6 @@ class CStringChecker
CheckerFrontendWithBugType UninitializedRead{
"Accessing unitialized/garbage values"};
- // FIXME: This bug type should be removed because it is only emitted in a
- // situation that is practically impossible.
- const BugType AdditionOverflow{&OutOfBounds, "API"};
-
StringRef getDebugTag() const override { return "MallocChecker"; }
static void *getTag() { static int tag; return &tag; }
@@ -330,7 +326,6 @@ class CStringChecker
const Stmt *S, StringRef WarningMsg) const;
void emitNotCStringBug(CheckerContext &C, ProgramStateRef State,
const Stmt *S, StringRef WarningMsg) const;
- void emitAdditionOverflowBug(CheckerContext &C, ProgramStateRef State) const;
void emitUninitializedReadBug(CheckerContext &C, ProgramStateRef State,
const Expr *E, const MemRegion *R,
StringRef Msg) const;
@@ -843,22 +838,6 @@ void CStringChecker::emitNotCStringBug(CheckerContext &C, ProgramStateRef State,
}
}
-void CStringChecker::emitAdditionOverflowBug(CheckerContext &C,
- ProgramStateRef State) const {
- if (ExplodedNode *N = C.generateErrorNode(State)) {
- // This isn't a great error message, but this should never occur in real
- // code anyway -- you'd have to create a buffer longer than a size_t can
- // represent, which is sort of a contradiction.
- const char *WarningMsg =
- "This expression will create a string whose length is too big to "
- "be represented as a size_t";
-
- auto Report = std::make_unique<PathSensitiveBugReport>(AdditionOverflow,
- WarningMsg, N);
- C.emitReport(std::move(Report));
- }
-}
-
ProgramStateRef CStringChecker::checkAdditionOverflow(CheckerContext &C,
ProgramStateRef state,
NonLoc left,
@@ -896,19 +875,22 @@ ProgramStateRef CStringChecker::checkAdditionOverflow(CheckerContext &C,
SVal willOverflow = svalBuilder.evalBinOpNN(state, BO_GT, left,
*maxMinusRightNL, cmpTy);
- ProgramStateRef stateOverflow, stateOkay;
- std::tie(stateOverflow, stateOkay) =
- state->assume(willOverflow.castAs<DefinedOrUnknownSVal>());
+ auto [StateOverflow, StateOkay] =
+ state->assume(willOverflow.castAs<DefinedOrUnknownSVal>());
- if (stateOverflow && !stateOkay) {
- // We have an overflow. Emit a bug report.
- emitAdditionOverflowBug(C, stateOverflow);
+ if (StateOverflow && !StateOkay) {
+ // On this path the analyzer is convinced that the addition of these two
+ // values would overflow `size_t` which must be caused by the inaccuracy
+ // of our modeling because this method is called in situations where the
+ // summands are size/length values which are much less than SIZE_MAX. To
+ // avoid false positives let's just sink this invalid path.
+ C.addSink(StateOverflow);
return nullptr;
}
// From now on, assume an overflow didn't occur.
- assert(stateOkay);
- state = stateOkay;
+ assert(StateOkay);
+ state = StateOkay;
}
return state;
|
|
@llvm/pr-subscribers-clang Author: Donát Nagy (NagyDonat) ChangesCStringChecker had an AdditionOverflow bug type which was intended for a situation where the analyzer concludes that the addition of two size/length values overflows I strongly suspect that the analyzer could emit bugs of this type in certain complex corner cases (e.g. due to inaccurate cast modeling), but these reports would be all false positives because in the real world the sum of two size/length values is always far below SIZE_MAX. (Although note that there was no test where the analyzer emitted a bug with this type.) To simplify the code (and perhaps eliminate false positives), I eliminated this bug type and replaced code that emits it by a simple Full diff: https://github.com/llvm/llvm-project/pull/152163.diff 1 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
index fd0a3982bf461..0e5fc0a074938 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -97,10 +97,6 @@ class CStringChecker
CheckerFrontendWithBugType UninitializedRead{
"Accessing unitialized/garbage values"};
- // FIXME: This bug type should be removed because it is only emitted in a
- // situation that is practically impossible.
- const BugType AdditionOverflow{&OutOfBounds, "API"};
-
StringRef getDebugTag() const override { return "MallocChecker"; }
static void *getTag() { static int tag; return &tag; }
@@ -330,7 +326,6 @@ class CStringChecker
const Stmt *S, StringRef WarningMsg) const;
void emitNotCStringBug(CheckerContext &C, ProgramStateRef State,
const Stmt *S, StringRef WarningMsg) const;
- void emitAdditionOverflowBug(CheckerContext &C, ProgramStateRef State) const;
void emitUninitializedReadBug(CheckerContext &C, ProgramStateRef State,
const Expr *E, const MemRegion *R,
StringRef Msg) const;
@@ -843,22 +838,6 @@ void CStringChecker::emitNotCStringBug(CheckerContext &C, ProgramStateRef State,
}
}
-void CStringChecker::emitAdditionOverflowBug(CheckerContext &C,
- ProgramStateRef State) const {
- if (ExplodedNode *N = C.generateErrorNode(State)) {
- // This isn't a great error message, but this should never occur in real
- // code anyway -- you'd have to create a buffer longer than a size_t can
- // represent, which is sort of a contradiction.
- const char *WarningMsg =
- "This expression will create a string whose length is too big to "
- "be represented as a size_t";
-
- auto Report = std::make_unique<PathSensitiveBugReport>(AdditionOverflow,
- WarningMsg, N);
- C.emitReport(std::move(Report));
- }
-}
-
ProgramStateRef CStringChecker::checkAdditionOverflow(CheckerContext &C,
ProgramStateRef state,
NonLoc left,
@@ -896,19 +875,22 @@ ProgramStateRef CStringChecker::checkAdditionOverflow(CheckerContext &C,
SVal willOverflow = svalBuilder.evalBinOpNN(state, BO_GT, left,
*maxMinusRightNL, cmpTy);
- ProgramStateRef stateOverflow, stateOkay;
- std::tie(stateOverflow, stateOkay) =
- state->assume(willOverflow.castAs<DefinedOrUnknownSVal>());
+ auto [StateOverflow, StateOkay] =
+ state->assume(willOverflow.castAs<DefinedOrUnknownSVal>());
- if (stateOverflow && !stateOkay) {
- // We have an overflow. Emit a bug report.
- emitAdditionOverflowBug(C, stateOverflow);
+ if (StateOverflow && !StateOkay) {
+ // On this path the analyzer is convinced that the addition of these two
+ // values would overflow `size_t` which must be caused by the inaccuracy
+ // of our modeling because this method is called in situations where the
+ // summands are size/length values which are much less than SIZE_MAX. To
+ // avoid false positives let's just sink this invalid path.
+ C.addSink(StateOverflow);
return nullptr;
}
// From now on, assume an overflow didn't occur.
- assert(stateOkay);
- state = stateOkay;
+ assert(StateOkay);
+ state = StateOkay;
}
return state;
|
|
The failure of the CI jobs is unrelated to this commit; it seems that the tests were broken on main at the moment when the CI jobs tested this commit. As main was stabilized since that time, re-running the CI jobs should be successful. |
|
Rerunning the CI checks didn't change the fact that the Github automation runs the tests on a merge between my commit (which would pass the CI) with 74af2ce (where the CI fails) instead of a more recent main revision (where the CI would pass). I did a merge from main to force the use of a non-broken revision. |
CStringChecker had an AdditionOverflow bug type which was intended for a situation where the analyzer concludes that the addition of two size/length values overflows
size_t.I strongly suspect that the analyzer could emit bugs of this type in certain complex corner cases (e.g. due to inaccurate cast modeling), but these reports would be all false positives because in the real world the sum of two size/length values is always far below SIZE_MAX. (Although note that there was no test where the analyzer emitted a bug with this type.)
To simplify the code (and perhaps eliminate false positives), I eliminated this bug type and replaced code that emits it by a simple
addSink()call (because we still want to get rid of the execution paths where the analyzer has invalid assumptions).