MTC created this revision.
MTC added reviewers: NoQ, george.karpenkov, xazax.hun.
Herald added subscribers: cfe-commits, a.sidorin, rnkovacs, szepet.
When improving the modeling `evalMemset()`, some scenes need to emit report of
`NotNullTerm`. In this case,
there are three places in `CStringChecker.cpp` that have the same code about
`NotNullTerm` report. In
addition, `CStringChecker.cpp` already has `emitOverlapBug()`, so I think it
needs code refactoring.
Thanks in advance for the code review!
Repository:
rC Clang
https://reviews.llvm.org/D44557
Files:
lib/StaticAnalyzer/Checkers/CStringChecker.cpp
Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -194,6 +194,14 @@
const Stmt *First,
const Stmt *Second) const;
+ void emitNullArgBug(CheckerContext &C, ProgramStateRef State, const Stmt *S,
+ StringRef WarningMsg) const;
+ void emitOutOfBoundsBug(CheckerContext &C, ProgramStateRef State,
+ 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;
+
ProgramStateRef checkAdditionOverflow(CheckerContext &C,
ProgramStateRef state,
NonLoc left,
@@ -239,30 +247,14 @@
std::tie(stateNull, stateNonNull) = assumeZero(C, state, l, S->getType());
if (stateNull && !stateNonNull) {
- if (!Filter.CheckCStringNullArg)
- return nullptr;
-
- ExplodedNode *N = C.generateErrorNode(stateNull);
- if (!N)
- return nullptr;
-
- if (!BT_Null)
- BT_Null.reset(new BuiltinBug(
- Filter.CheckNameCStringNullArg, categories::UnixAPI,
- "Null pointer argument in call to byte string function"));
-
- SmallString<80> buf;
- llvm::raw_svector_ostream os(buf);
- assert(CurrentFunctionDescription);
- os << "Null pointer argument in call to " << CurrentFunctionDescription;
-
- // Generate a report for this bug.
- BuiltinBug *BT = static_cast<BuiltinBug*>(BT_Null.get());
- auto report = llvm::make_unique<BugReport>(*BT, os.str(), N);
+ if (Filter.CheckCStringNullArg) {
+ SmallString<80> buf;
+ llvm::raw_svector_ostream os(buf);
+ assert(CurrentFunctionDescription);
+ os << "Null pointer argument in call to " << CurrentFunctionDescription;
- report->addRange(S->getSourceRange());
- bugreporter::trackNullOrUndefValue(N, S, *report);
- C.emitReport(std::move(report));
+ emitNullArgBug(C, stateNull, S, os.str());
+ }
return nullptr;
}
@@ -305,31 +297,14 @@
ProgramStateRef StInBound = state->assumeInBound(Idx, Size, true);
ProgramStateRef StOutBound = state->assumeInBound(Idx, Size, false);
if (StOutBound && !StInBound) {
- ExplodedNode *N = C.generateErrorNode(StOutBound);
- if (!N)
- return nullptr;
-
- CheckName Name;
// These checks are either enabled by the CString out-of-bounds checker
// explicitly or the "basic" CStringNullArg checker support that Malloc
// checker enables.
assert(Filter.CheckCStringOutOfBounds || Filter.CheckCStringNullArg);
- if (Filter.CheckCStringOutOfBounds)
- Name = Filter.CheckNameCStringOutOfBounds;
- else
- Name = Filter.CheckNameCStringNullArg;
- if (!BT_Bounds) {
- BT_Bounds.reset(new BuiltinBug(
- Name, "Out-of-bound array access",
- "Byte string function accesses out-of-bound array element"));
- }
- BuiltinBug *BT = static_cast<BuiltinBug*>(BT_Bounds.get());
-
- // Generate a report for this bug.
- std::unique_ptr<BugReport> report;
+ // Emit a bug report.
if (warningMsg) {
- report = llvm::make_unique<BugReport>(*BT, warningMsg, N);
+ emitOutOfBoundsBug(C, StOutBound, S, warningMsg);
} else {
assert(CurrentFunctionDescription);
assert(CurrentFunctionDescription[0] != '\0');
@@ -339,15 +314,8 @@
os << toUppercase(CurrentFunctionDescription[0])
<< &CurrentFunctionDescription[1]
<< " accesses out-of-bound array element";
- report = llvm::make_unique<BugReport>(*BT, os.str(), N);
+ emitOutOfBoundsBug(C, StOutBound, S, os.str());
}
-
- // FIXME: It would be nice to eventually make this diagnostic more clear,
- // e.g., by referencing the original declaration or by saying *why* this
- // reference is outside the range.
-
- report->addRange(S->getSourceRange());
- C.emitReport(std::move(report));
return nullptr;
}
@@ -565,6 +533,79 @@
C.emitReport(std::move(report));
}
+void CStringChecker::emitNullArgBug(CheckerContext &C, ProgramStateRef State,
+ const Stmt *S, StringRef WarningMsg) const {
+ if (ExplodedNode *N = C.generateErrorNode(State)) {
+ if (!BT_Null)
+ BT_Null.reset(new BuiltinBug(
+ Filter.CheckNameCStringNullArg, categories::UnixAPI,
+ "Null pointer argument in call to byte string function"));
+
+ BuiltinBug *BT = static_cast<BuiltinBug *>(BT_Null.get());
+ auto Report = llvm::make_unique<BugReport>(*BT, WarningMsg, N);
+ bugreporter::trackNullOrUndefValue(N, S, *Report);
+ C.emitReport(std::move(Report));
+ }
+}
+
+void CStringChecker::emitOutOfBoundsBug(CheckerContext &C,
+ ProgramStateRef State, const Stmt *S,
+ StringRef WarningMsg) const {
+ if (ExplodedNode *N = C.generateErrorNode(State)) {
+ if (!BT_Bounds)
+ BT_Bounds.reset(new BuiltinBug(
+ Filter.CheckCStringOutOfBounds ? Filter.CheckNameCStringOutOfBounds
+ : Filter.CheckNameCStringNullArg,
+ "Out-of-bound array access",
+ "Byte string function accesses out-of-bound array element"));
+
+ BuiltinBug *BT = static_cast<BuiltinBug *>(BT_Bounds.get());
+
+ // FIXME: It would be nice to eventually make this diagnostic more clear,
+ // e.g., by referencing the original declaration or by saying *why* this
+ // reference is outside the range.
+ auto Report = llvm::make_unique<BugReport>(*BT, WarningMsg, N);
+ Report->addRange(S->getSourceRange());
+ C.emitReport(std::move(Report));
+ }
+}
+
+void CStringChecker::emitNotCStringBug(CheckerContext &C, ProgramStateRef State,
+ const Stmt *S,
+ StringRef WarningMsg) const {
+ if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
+ if (!BT_NotCString)
+ BT_NotCString.reset(new BuiltinBug(
+ Filter.CheckNameCStringNotNullTerm, categories::UnixAPI,
+ "Argument is not a null-terminated string."));
+
+ auto Report = llvm::make_unique<BugReport>(*BT_NotCString, WarningMsg, N);
+
+ Report->addRange(S->getSourceRange());
+ C.emitReport(std::move(Report));
+ }
+}
+
+void CStringChecker::emitAdditionOverflowBug(CheckerContext &C,
+ ProgramStateRef State) const {
+ if (ExplodedNode *N = C.generateErrorNode(State)) {
+ if (!BT_NotCString)
+ BT_NotCString.reset(
+ new BuiltinBug(Filter.CheckNameCStringOutOfBounds, "API",
+ "Sum of expressions causes overflow."));
+
+ // 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 = llvm::make_unique<BugReport>(*BT_NotCString, WarningMsg, N);
+ C.emitReport(std::move(Report));
+ }
+}
+
ProgramStateRef CStringChecker::checkAdditionOverflow(CheckerContext &C,
ProgramStateRef state,
NonLoc left,
@@ -608,26 +649,7 @@
if (stateOverflow && !stateOkay) {
// We have an overflow. Emit a bug report.
- ExplodedNode *N = C.generateErrorNode(stateOverflow);
- if (!N)
- return nullptr;
-
- if (!BT_AdditionOverflow)
- BT_AdditionOverflow.reset(
- new BuiltinBug(Filter.CheckNameCStringOutOfBounds, "API",
- "Sum of expressions causes overflow"));
-
- // 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 *warning =
- "This expression will create a string whose length is too big to "
- "be represented as a size_t";
-
- // Generate a report for this bug.
- C.emitReport(
- llvm::make_unique<BugReport>(*BT_AdditionOverflow, warning, N));
-
+ emitAdditionOverflowBug(C, stateOverflow);
return nullptr;
}
@@ -727,30 +749,17 @@
// C string. In the context of locations, the only time we can issue such
// a warning is for labels.
if (Optional<loc::GotoLabel> Label = Buf.getAs<loc::GotoLabel>()) {
- if (!Filter.CheckCStringNotNullTerm)
- return UndefinedVal();
-
- if (ExplodedNode *N = C.generateNonFatalErrorNode(state)) {
- if (!BT_NotCString)
- BT_NotCString.reset(new BuiltinBug(
- Filter.CheckNameCStringNotNullTerm, categories::UnixAPI,
- "Argument is not a null-terminated string."));
-
+ if (Filter.CheckCStringNotNullTerm) {
SmallString<120> buf;
llvm::raw_svector_ostream os(buf);
assert(CurrentFunctionDescription);
os << "Argument to " << CurrentFunctionDescription
<< " is the address of the label '" << Label->getLabel()->getName()
<< "', which is not a null-terminated string";
- // Generate a report for this bug.
- auto report = llvm::make_unique<BugReport>(*BT_NotCString, os.str(), N);
-
- report->addRange(Ex->getSourceRange());
- C.emitReport(std::move(report));
+ emitNotCStringBug(C, state, Ex, os.str());
}
return UndefinedVal();
-
}
// If it's not a region and not a label, give up.
@@ -787,15 +796,7 @@
// Other regions (mostly non-data) can't have a reliable C string length.
// In this case, an error is emitted and UndefinedVal is returned.
// The caller should always be prepared to handle this case.
- if (!Filter.CheckCStringNotNullTerm)
- return UndefinedVal();
-
- if (ExplodedNode *N = C.generateNonFatalErrorNode(state)) {
- if (!BT_NotCString)
- BT_NotCString.reset(new BuiltinBug(
- Filter.CheckNameCStringNotNullTerm, categories::UnixAPI,
- "Argument is not a null-terminated string."));
-
+ if (Filter.CheckCStringNotNullTerm) {
SmallString<120> buf;
llvm::raw_svector_ostream os(buf);
@@ -807,13 +808,8 @@
else
os << "not a null-terminated string";
- // Generate a report for this bug.
- auto report = llvm::make_unique<BugReport>(*BT_NotCString, os.str(), N);
-
- report->addRange(Ex->getSourceRange());
- C.emitReport(std::move(report));
+ emitNotCStringBug(C, state, Ex, os.str());
}
-
return UndefinedVal();
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
