[clang] 90cb529 - [clang][analyzer] Improve report of file read at EOF condition (alpha.unix.Stream checker).
Balázs Kéri via cfe-commits
cfe-commits at lists.llvm.org
Tue Jul 20 23:42:05 PDT 2021
Author: Balázs Kéri
Date: 2021-07-21T08:54:11+02:00
New Revision: 90cb5297adf086073f14d455050b5cde9a03503d
URL: https://github.com/llvm/llvm-project/commit/90cb5297adf086073f14d455050b5cde9a03503d
DIFF: https://github.com/llvm/llvm-project/commit/90cb5297adf086073f14d455050b5cde9a03503d.diff
LOG: [clang][analyzer] Improve report of file read at EOF condition (alpha.unix.Stream checker).
The checker warns if a stream is read that is already in end-of-file
(EOF) state.
The commit adds indication of the last location where the EOF flag is set
on the stream.
Reviewed By: Szelethus
Differential Revision: https://reviews.llvm.org/D104925
Added:
Modified:
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
clang/test/Analysis/stream-note.c
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 75db1e195a432..dd65f8c035aa1 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -25,6 +25,10 @@ using namespace clang;
using namespace ento;
using namespace std::placeholders;
+//===----------------------------------------------------------------------===//
+// Definition of state data structures.
+//===----------------------------------------------------------------------===//
+
namespace {
struct FnDescription;
@@ -146,6 +150,14 @@ struct StreamState {
}
};
+} // namespace
+
+//===----------------------------------------------------------------------===//
+// StreamChecker class and utility functions.
+//===----------------------------------------------------------------------===//
+
+namespace {
+
class StreamChecker;
using FnCheck = std::function<void(const StreamChecker *, const FnDescription *,
const CallEvent &, CheckerContext &)>;
@@ -219,6 +231,8 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
/// If true, evaluate special testing stream functions.
bool TestMode = false;
+ const BugType *getBT_StreamEof() const { return &BT_StreamEof; }
+
private:
CallDescriptionMap<FnDescription> FnDescriptions = {
{{"fopen"}, {nullptr, &StreamChecker::evalFopen, ArgNone}},
@@ -337,7 +351,8 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
/// There will be always a state transition into the passed State,
/// by the new non-fatal error node or (if failed) a normal transition,
/// to ensure uniform handling.
- void reportFEofWarning(CheckerContext &C, ProgramStateRef State) const;
+ void reportFEofWarning(SymbolRef StreamSym, CheckerContext &C,
+ ProgramStateRef State) const;
/// Emit resource leak warnings for the given symbols.
/// Createn a non-fatal error node for these, and returns it (if any warnings
@@ -363,14 +378,14 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
/// Generate a message for BugReporterVisitor if the stored symbol is
/// marked as interesting by the actual bug report.
+ // FIXME: Use lambda instead.
struct NoteFn {
- const CheckerNameRef CheckerName;
+ const BugType *BT_ResourceLeak;
SymbolRef StreamSym;
std::string Message;
std::string operator()(PathSensitiveBugReport &BR) const {
- if (BR.isInteresting(StreamSym) &&
- CheckerName == BR.getBugType().getCheckerName())
+ if (BR.isInteresting(StreamSym) && &BR.getBugType() == BT_ResourceLeak)
return Message;
return "";
@@ -379,7 +394,20 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
const NoteTag *constructNoteTag(CheckerContext &C, SymbolRef StreamSym,
const std::string &Message) const {
- return C.getNoteTag(NoteFn{getCheckerName(), StreamSym, Message});
+ return C.getNoteTag(NoteFn{&BT_ResourceLeak, StreamSym, Message});
+ }
+
+ const NoteTag *constructSetEofNoteTag(CheckerContext &C,
+ SymbolRef StreamSym) const {
+ return C.getNoteTag([this, StreamSym](PathSensitiveBugReport &BR) {
+ if (!BR.isInteresting(StreamSym) ||
+ &BR.getBugType() != this->getBT_StreamEof())
+ return "";
+
+ BR.markNotInteresting(StreamSym);
+
+ return "Assuming stream reaches end-of-file here";
+ });
}
/// Searches for the ExplodedNode where the file descriptor was acquired for
@@ -391,6 +419,9 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
} // end anonymous namespace
+// This map holds the state of a stream.
+// The stream is identified with a SymbolRef that is created when a stream
+// opening function is modeled by the checker.
REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
inline void assertStreamStateOpened(const StreamState *SS) {
@@ -419,6 +450,10 @@ const ExplodedNode *StreamChecker::getAcquisitionSite(const ExplodedNode *N,
return nullptr;
}
+//===----------------------------------------------------------------------===//
+// Methods of StreamChecker.
+//===----------------------------------------------------------------------===//
+
void StreamChecker::checkPreCall(const CallEvent &Call,
CheckerContext &C) const {
const FnDescription *Desc = lookupFn(Call);
@@ -566,7 +601,7 @@ void StreamChecker::preFread(const FnDescription *Desc, const CallEvent &Call,
if (Sym && State->get<StreamMap>(Sym)) {
const StreamState *SS = State->get<StreamMap>(Sym);
if (SS->ErrorState & ErrorFEof)
- reportFEofWarning(C, State);
+ reportFEofWarning(Sym, C, State);
} else {
C.addTransition(State);
}
@@ -609,11 +644,11 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
if (!NMembVal)
return;
- const StreamState *SS = State->get<StreamMap>(StreamSym);
- if (!SS)
+ const StreamState *OldSS = State->get<StreamMap>(StreamSym);
+ if (!OldSS)
return;
- assertStreamStateOpened(SS);
+ assertStreamStateOpened(OldSS);
// C'99 standard, §7.19.8.1.3, the return value of fread:
// The fread function returns the number of elements successfully read, which
@@ -632,7 +667,7 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
// Generate a transition for the success state.
// If we know the state to be FEOF at fread, do not add a success state.
- if (!IsFread || (SS->ErrorState != ErrorFEof)) {
+ if (!IsFread || (OldSS->ErrorState != ErrorFEof)) {
ProgramStateRef StateNotFailed =
State->BindExpr(CE, C.getLocationContext(), *NMembVal);
if (StateNotFailed) {
@@ -661,14 +696,18 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
StreamErrorState NewES;
if (IsFread)
- NewES = (SS->ErrorState == ErrorFEof) ? ErrorFEof : ErrorFEof | ErrorFError;
+ NewES =
+ (OldSS->ErrorState == ErrorFEof) ? ErrorFEof : ErrorFEof | ErrorFError;
else
NewES = ErrorFError;
// If a (non-EOF) error occurs, the resulting value of the file position
// indicator for the stream is indeterminate.
- StreamState NewState = StreamState::getOpened(Desc, NewES, !NewES.isFEof());
- StateFailed = StateFailed->set<StreamMap>(StreamSym, NewState);
- C.addTransition(StateFailed);
+ StreamState NewSS = StreamState::getOpened(Desc, NewES, !NewES.isFEof());
+ StateFailed = StateFailed->set<StreamMap>(StreamSym, NewSS);
+ if (IsFread && OldSS->ErrorState != ErrorFEof)
+ C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
+ else
+ C.addTransition(StateFailed);
}
void StreamChecker::preFseek(const FnDescription *Desc, const CallEvent &Call,
@@ -727,7 +766,7 @@ void StreamChecker::evalFseek(const FnDescription *Desc, const CallEvent &Call,
StreamState::getOpened(Desc, ErrorNone | ErrorFEof | ErrorFError, true));
C.addTransition(StateNotFailed);
- C.addTransition(StateFailed);
+ C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
}
void StreamChecker::evalClearerr(const FnDescription *Desc,
@@ -960,14 +999,16 @@ StreamChecker::ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C,
return State;
}
-void StreamChecker::reportFEofWarning(CheckerContext &C,
+void StreamChecker::reportFEofWarning(SymbolRef StreamSym, CheckerContext &C,
ProgramStateRef State) const {
if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
- C.emitReport(std::make_unique<PathSensitiveBugReport>(
+ auto R = std::make_unique<PathSensitiveBugReport>(
BT_StreamEof,
"Read function called when stream is in EOF state. "
"Function has no effect.",
- N));
+ N);
+ R->markInteresting(StreamSym);
+ C.emitReport(std::move(R));
return;
}
C.addTransition(State);
@@ -1058,6 +1099,10 @@ ProgramStateRef StreamChecker::checkPointerEscape(
return State;
}
+//===----------------------------------------------------------------------===//
+// Checker registration.
+//===----------------------------------------------------------------------===//
+
void ento::registerStreamChecker(CheckerManager &Mgr) {
Mgr.registerChecker<StreamChecker>();
}
@@ -1073,4 +1118,4 @@ void ento::registerStreamTesterChecker(CheckerManager &Mgr) {
bool ento::shouldRegisterStreamTesterChecker(const CheckerManager &Mgr) {
return true;
-}
+}
\ No newline at end of file
diff --git a/clang/test/Analysis/stream-note.c b/clang/test/Analysis/stream-note.c
index a509bb1b58315..2a2928486e5bc 100644
--- a/clang/test/Analysis/stream-note.c
+++ b/clang/test/Analysis/stream-note.c
@@ -88,3 +88,60 @@ void check_track_null() {
fclose(F); // expected-warning {{Stream pointer might be NULL}}
// expected-note at -1 {{Stream pointer might be NULL}}
}
+
+void check_eof_notes_feof_after_feof() {
+ FILE *F;
+ char Buf[10];
+ F = fopen("foo1.c", "r");
+ if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
+ return;
+ }
+ fread(Buf, 1, 1, F);
+ if (feof(F)) { // expected-note {{Taking true branch}}
+ clearerr(F);
+ fread(Buf, 1, 1, F); // expected-note {{Assuming stream reaches end-of-file here}}
+ if (feof(F)) { // expected-note {{Taking true branch}}
+ fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}}
+ // expected-note at -1 {{Read function called when stream is in EOF state. Function has no effect}}
+ }
+ }
+ fclose(F);
+}
+
+void check_eof_notes_feof_after_no_feof() {
+ FILE *F;
+ char Buf[10];
+ F = fopen("foo1.c", "r");
+ if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
+ return;
+ }
+ fread(Buf, 1, 1, F);
+ if (feof(F)) { // expected-note {{Taking false branch}}
+ fclose(F);
+ return;
+ } else if (ferror(F)) { // expected-note {{Taking false branch}}
+ fclose(F);
+ return;
+ }
+ fread(Buf, 1, 1, F); // expected-note {{Assuming stream reaches end-of-file here}}
+ if (feof(F)) { // expected-note {{Taking true branch}}
+ fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}}
+ // expected-note at -1 {{Read function called when stream is in EOF state. Function has no effect}}
+ }
+ fclose(F);
+}
+
+void check_eof_notes_feof_or_no_error() {
+ FILE *F;
+ char Buf[10];
+ F = fopen("foo1.c", "r");
+ if (F == NULL) // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
+ return;
+ int RRet = fread(Buf, 1, 1, F); // expected-note {{Assuming stream reaches end-of-file here}}
+ if (ferror(F)) { // expected-note {{Taking false branch}}
+ } else {
+ fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}}
+ // expected-note at -1 {{Read function called when stream is in EOF state. Function has no effect}}
+ }
+ fclose(F);
+}
More information about the cfe-commits
mailing list