[clang] [clang][analyzer] Add StreamChecker note tags for "indeterminate stream position". (PR #83288)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 28 08:41:45 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Balázs Kéri (balazske)
<details>
<summary>Changes</summary>
If a stream operation fails the position can become "indeterminate". This may cause warning from the checker at a later operation. The new note tag shows the place where the position becomes "indeterminate", this is where a failure occurred.
---
Full diff: https://github.com/llvm/llvm-project/pull/83288.diff
2 Files Affected:
- (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+168-125)
- (modified) clang/test/Analysis/stream-note.c (+67)
``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 65bdc4cac30940..f3c3b0ce6d1a6f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -218,87 +218,6 @@ inline void assertStreamStateOpened(const StreamState *SS) {
assert(SS->isOpened() && "Stream is expected to be opened");
}
-struct StreamOperationEvaluator {
- SValBuilder &SVB;
- const ASTContext &ACtx;
-
- SymbolRef StreamSym;
- const StreamState *SS = nullptr;
- const CallExpr *CE = nullptr;
-
- StreamOperationEvaluator(CheckerContext &C)
- : SVB(C.getSValBuilder()), ACtx(C.getASTContext()) {
- ;
- }
-
- bool Init(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C,
- ProgramStateRef State) {
- StreamSym = getStreamArg(Desc, Call).getAsSymbol();
- if (!StreamSym)
- return false;
- SS = State->get<StreamMap>(StreamSym);
- if (!SS)
- return false;
- CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
- if (!CE)
- return false;
-
- assertStreamStateOpened(SS);
-
- return true;
- }
-
- bool isStreamEof() const { return SS->ErrorState == ErrorFEof; }
-
- NonLoc getZeroVal(const CallEvent &Call) {
- return *SVB.makeZeroVal(Call.getResultType()).getAs<NonLoc>();
- }
-
- ProgramStateRef setStreamState(ProgramStateRef State,
- const StreamState &NewSS) {
- return State->set<StreamMap>(StreamSym, NewSS);
- }
-
- ProgramStateRef makeAndBindRetVal(ProgramStateRef State, CheckerContext &C) {
- NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
- return State->BindExpr(CE, C.getLocationContext(), RetVal);
- }
-
- ProgramStateRef bindReturnValue(ProgramStateRef State, CheckerContext &C,
- uint64_t Val) {
- return State->BindExpr(CE, C.getLocationContext(),
- SVB.makeIntVal(Val, CE->getCallReturnType(ACtx)));
- }
-
- ProgramStateRef bindReturnValue(ProgramStateRef State, CheckerContext &C,
- SVal Val) {
- return State->BindExpr(CE, C.getLocationContext(), Val);
- }
-
- ProgramStateRef bindNullReturnValue(ProgramStateRef State,
- CheckerContext &C) {
- return State->BindExpr(CE, C.getLocationContext(),
- C.getSValBuilder().makeNullWithType(CE->getType()));
- }
-
- ProgramStateRef assumeBinOpNN(ProgramStateRef State,
- BinaryOperator::Opcode Op, NonLoc LHS,
- NonLoc RHS) {
- auto Cond = SVB.evalBinOpNN(State, Op, LHS, RHS, SVB.getConditionType())
- .getAs<DefinedOrUnknownSVal>();
- if (!Cond)
- return nullptr;
- return State->assume(*Cond, true);
- }
-
- ConstraintManager::ProgramStatePair
- makeRetValAndAssumeDual(ProgramStateRef State, CheckerContext &C) {
- DefinedSVal RetVal = makeRetVal(C, CE);
- State = State->BindExpr(CE, C.getLocationContext(), RetVal);
- return C.getConstraintManager().assumeDual(State, RetVal);
- }
-};
-
class StreamChecker : public Checker<check::PreCall, eval::Call,
check::DeadSymbols, check::PointerEscape> {
BugType BT_FileNull{this, "NULL stream pointer", "Stream handling error"};
@@ -313,6 +232,9 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
BugType BT_ResourceLeak{this, "Resource leak", "Stream handling error",
/*SuppressOnSink =*/true};
+ const char *FeofNote = "Assuming stream reaches end-of-file here";
+ const char *FerrorNote = "Assuming this stream operation fails";
+
public:
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
@@ -322,11 +244,57 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
const CallEvent *Call,
PointerEscapeKind Kind) const;
+ const BugType *getBT_StreamEof() const { return &BT_StreamEof; }
+ const BugType *getBT_IndeterminatePosition() const { return &BT_IndeterminatePosition; }
+
+ 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 FeofNote;
+ });
+ }
+
+ const NoteTag *constructSetErrorNoteTag(CheckerContext &C,
+ SymbolRef StreamSym) const {
+ return C.getNoteTag([this, StreamSym](PathSensitiveBugReport &BR) {
+ if (!BR.isInteresting(StreamSym) ||
+ &BR.getBugType() != this->getBT_IndeterminatePosition())
+ return "";
+
+ BR.markNotInteresting(StreamSym);
+
+ return FerrorNote;
+ });
+ }
+
+ const NoteTag *constructSetEofOrErrorNoteTag(CheckerContext &C,
+ SymbolRef StreamSym) const {
+ return C.getNoteTag([this, StreamSym](PathSensitiveBugReport &BR) {
+ if (!BR.isInteresting(StreamSym))
+ return "";
+
+ if (&BR.getBugType() == this->getBT_StreamEof()) {
+ BR.markNotInteresting(StreamSym);
+ return FeofNote;
+ }
+ if (&BR.getBugType() == this->getBT_IndeterminatePosition()) {
+ BR.markNotInteresting(StreamSym);
+ return FerrorNote;
+ }
+
+ return "";
+ });
+ }
+
/// If true, evaluate special testing stream functions.
bool TestMode = false;
- const BugType *getBT_StreamEof() const { return &BT_StreamEof; }
-
private:
CallDescriptionMap<FnDescription> FnDescriptions = {
{{{"fopen"}, 2}, {nullptr, &StreamChecker::evalFopen, ArgNone}},
@@ -557,7 +525,7 @@ 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.
- const NoteTag *constructNoteTag(CheckerContext &C, SymbolRef StreamSym,
+ const NoteTag *constructLeakNoteTag(CheckerContext &C, SymbolRef StreamSym,
const std::string &Message) const {
return C.getNoteTag([this, StreamSym,
Message](PathSensitiveBugReport &BR) -> std::string {
@@ -567,19 +535,6 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
});
}
- 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";
- });
- }
-
void initMacroValues(CheckerContext &C) const {
if (EofVal)
return;
@@ -607,6 +562,102 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
CheckerContext &C);
};
+struct StreamOperationEvaluator {
+ SValBuilder &SVB;
+ const ASTContext &ACtx;
+
+ SymbolRef StreamSym;
+ const StreamState *SS = nullptr;
+ const CallExpr *CE = nullptr;
+ StreamErrorState NewES;
+
+ StreamOperationEvaluator(CheckerContext &C)
+ : SVB(C.getSValBuilder()), ACtx(C.getASTContext()) {
+ ;
+ }
+
+ bool Init(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C,
+ ProgramStateRef State) {
+ StreamSym = getStreamArg(Desc, Call).getAsSymbol();
+ if (!StreamSym)
+ return false;
+ SS = State->get<StreamMap>(StreamSym);
+ if (!SS)
+ return false;
+ NewES = SS->ErrorState;
+ CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+ if (!CE)
+ return false;
+
+ assertStreamStateOpened(SS);
+
+ return true;
+ }
+
+ bool isStreamEof() const { return SS->ErrorState == ErrorFEof; }
+
+ NonLoc getZeroVal(const CallEvent &Call) {
+ return *SVB.makeZeroVal(Call.getResultType()).getAs<NonLoc>();
+ }
+
+ ProgramStateRef setStreamState(ProgramStateRef State,
+ const StreamState &NewSS) {
+ NewES = NewSS.ErrorState;
+ return State->set<StreamMap>(StreamSym, NewSS);
+ }
+
+ ProgramStateRef makeAndBindRetVal(ProgramStateRef State, CheckerContext &C) {
+ NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
+ return State->BindExpr(CE, C.getLocationContext(), RetVal);
+ }
+
+ ProgramStateRef bindReturnValue(ProgramStateRef State, CheckerContext &C,
+ uint64_t Val) {
+ return State->BindExpr(CE, C.getLocationContext(),
+ SVB.makeIntVal(Val, CE->getCallReturnType(ACtx)));
+ }
+
+ ProgramStateRef bindReturnValue(ProgramStateRef State, CheckerContext &C,
+ SVal Val) {
+ return State->BindExpr(CE, C.getLocationContext(), Val);
+ }
+
+ ProgramStateRef bindNullReturnValue(ProgramStateRef State,
+ CheckerContext &C) {
+ return State->BindExpr(CE, C.getLocationContext(),
+ C.getSValBuilder().makeNullWithType(CE->getType()));
+ }
+
+ ProgramStateRef assumeBinOpNN(ProgramStateRef State,
+ BinaryOperator::Opcode Op, NonLoc LHS,
+ NonLoc RHS) {
+ auto Cond = SVB.evalBinOpNN(State, Op, LHS, RHS, SVB.getConditionType())
+ .getAs<DefinedOrUnknownSVal>();
+ if (!Cond)
+ return nullptr;
+ return State->assume(*Cond, true);
+ }
+
+ ConstraintManager::ProgramStatePair
+ makeRetValAndAssumeDual(ProgramStateRef State, CheckerContext &C) {
+ DefinedSVal RetVal = makeRetVal(C, CE);
+ State = State->BindExpr(CE, C.getLocationContext(), RetVal);
+ return C.getConstraintManager().assumeDual(State, RetVal);
+ }
+
+ const NoteTag *getStreamErrorNoteTag(const StreamChecker *Ch, CheckerContext &C) {
+ bool SetFeof = NewES.FEof && !SS->ErrorState.FEof;
+ bool SetFerror = NewES.FError && !SS->ErrorState.FError;
+ if (SetFeof && !SetFerror)
+ return Ch->constructSetEofNoteTag(C, StreamSym);
+ if (!SetFeof && SetFerror)
+ return Ch->constructSetErrorNoteTag(C, StreamSym);
+ if (SetFeof && SetFerror)
+ return Ch->constructSetEofOrErrorNoteTag(C, StreamSym);
+ return nullptr;
+ }
+};
+
} // end anonymous namespace
const ExplodedNode *StreamChecker::getAcquisitionSite(const ExplodedNode *N,
@@ -697,7 +748,7 @@ void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc));
C.addTransition(StateNotNull,
- constructNoteTag(C, RetSym, "Stream opened here"));
+ constructLeakNoteTag(C, RetSym, "Stream opened here"));
C.addTransition(StateNull);
}
@@ -755,7 +806,7 @@ void StreamChecker::evalFreopen(const FnDescription *Desc,
StateRetNull->set<StreamMap>(StreamSym, StreamState::getOpenFailed(Desc));
C.addTransition(StateRetNotNull,
- constructNoteTag(C, StreamSym, "Stream reopened here"));
+ constructLeakNoteTag(C, StreamSym, "Stream reopened here"));
C.addTransition(StateRetNull);
}
@@ -867,10 +918,7 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
// indicator for the stream is indeterminate.
StateFailed = E.setStreamState(
StateFailed, StreamState::getOpened(Desc, NewES, !NewES.isFEof()));
- if (IsFread && !E.isStreamEof())
- C.addTransition(StateFailed, constructSetEofNoteTag(C, E.StreamSym));
- else
- C.addTransition(StateFailed);
+ C.addTransition(StateFailed, E.getStreamErrorNoteTag(this, C));
}
void StreamChecker::evalFgetx(const FnDescription *Desc, const CallEvent &Call,
@@ -929,10 +977,7 @@ void StreamChecker::evalFgetx(const FnDescription *Desc, const CallEvent &Call,
E.isStreamEof() ? ErrorFEof : ErrorFEof | ErrorFError;
StateFailed = E.setStreamState(
StateFailed, StreamState::getOpened(Desc, NewES, !NewES.isFEof()));
- if (!E.isStreamEof())
- C.addTransition(StateFailed, constructSetEofNoteTag(C, E.StreamSym));
- else
- C.addTransition(StateFailed);
+ C.addTransition(StateFailed, E.getStreamErrorNoteTag(this, C));
}
void StreamChecker::evalFputx(const FnDescription *Desc, const CallEvent &Call,
@@ -974,7 +1019,7 @@ void StreamChecker::evalFputx(const FnDescription *Desc, const CallEvent &Call,
ProgramStateRef StateFailed = E.bindReturnValue(State, C, *EofVal);
StateFailed = E.setStreamState(
StateFailed, StreamState::getOpened(Desc, ErrorFError, true));
- C.addTransition(StateFailed);
+ C.addTransition(StateFailed, E.getStreamErrorNoteTag(this, C));
}
void StreamChecker::evalFprintf(const FnDescription *Desc,
@@ -1008,7 +1053,7 @@ void StreamChecker::evalFprintf(const FnDescription *Desc,
// position indicator for the stream is indeterminate.
StateFailed = E.setStreamState(
StateFailed, StreamState::getOpened(Desc, ErrorFError, true));
- C.addTransition(StateFailed);
+ C.addTransition(StateFailed, E.getStreamErrorNoteTag(this, C));
}
void StreamChecker::evalFscanf(const FnDescription *Desc, const CallEvent &Call,
@@ -1058,10 +1103,7 @@ void StreamChecker::evalFscanf(const FnDescription *Desc, const CallEvent &Call,
E.isStreamEof() ? ErrorFEof : ErrorNone | ErrorFEof | ErrorFError;
StateFailed = E.setStreamState(
StateFailed, StreamState::getOpened(Desc, NewES, !NewES.isFEof()));
- if (!E.isStreamEof())
- C.addTransition(StateFailed, constructSetEofNoteTag(C, E.StreamSym));
- else
- C.addTransition(StateFailed);
+ C.addTransition(StateFailed, E.getStreamErrorNoteTag(this, C));
}
void StreamChecker::evalUngetc(const FnDescription *Desc, const CallEvent &Call,
@@ -1129,10 +1171,7 @@ void StreamChecker::evalGetdelim(const FnDescription *Desc,
E.isStreamEof() ? ErrorFEof : ErrorFEof | ErrorFError;
StateFailed = E.setStreamState(
StateFailed, StreamState::getOpened(Desc, NewES, !NewES.isFEof()));
- if (E.isStreamEof())
- C.addTransition(StateFailed, constructSetEofNoteTag(C, E.StreamSym));
- else
- C.addTransition(StateFailed);
+ C.addTransition(StateFailed, E.getStreamErrorNoteTag(this, C));
}
void StreamChecker::preFseek(const FnDescription *Desc, const CallEvent &Call,
@@ -1184,7 +1223,7 @@ void StreamChecker::evalFseek(const FnDescription *Desc, const CallEvent &Call,
NewErrS = NewErrS | ErrorFEof;
StateFailed = E.setStreamState(StateFailed,
StreamState::getOpened(Desc, NewErrS, true));
- C.addTransition(StateFailed, constructSetEofNoteTag(C, E.StreamSym));
+ C.addTransition(StateFailed, E.getStreamErrorNoteTag(this, C));
}
void StreamChecker::evalFgetpos(const FnDescription *Desc,
@@ -1228,7 +1267,7 @@ void StreamChecker::evalFsetpos(const FnDescription *Desc,
StateFailed, StreamState::getOpened(Desc, ErrorNone | ErrorFError, true));
C.addTransition(StateNotFailed);
- C.addTransition(StateFailed);
+ C.addTransition(StateFailed, E.getStreamErrorNoteTag(this, C));
}
void StreamChecker::evalFtell(const FnDescription *Desc, const CallEvent &Call,
@@ -1541,18 +1580,22 @@ ProgramStateRef StreamChecker::ensureNoFilePositionIndeterminate(
if (!N)
return nullptr;
- C.emitReport(std::make_unique<PathSensitiveBugReport>(
- BT_IndeterminatePosition, BugMessage, N));
+ auto R = std::make_unique<PathSensitiveBugReport>(
+ BT_IndeterminatePosition, BugMessage, N);
+ R->markInteresting(Sym);
+ C.emitReport(std::move(R));
return State->set<StreamMap>(
Sym, StreamState::getOpened(SS->LastOperation, ErrorFEof, false));
}
// Known or unknown error state without FEOF possible.
// Stop analysis, report error.
- ExplodedNode *N = C.generateErrorNode(State);
- if (N)
- C.emitReport(std::make_unique<PathSensitiveBugReport>(
- BT_IndeterminatePosition, BugMessage, N));
+ if (ExplodedNode *N = C.generateErrorNode(State)) {
+ auto R = std::make_unique<PathSensitiveBugReport>(
+ BT_IndeterminatePosition, BugMessage, N);
+ R->markInteresting(Sym);
+ C.emitReport(std::move(R));
+ }
return nullptr;
}
diff --git a/clang/test/Analysis/stream-note.c b/clang/test/Analysis/stream-note.c
index abb4784c078aa8..f77cd4aa62841d 100644
--- a/clang/test/Analysis/stream-note.c
+++ b/clang/test/Analysis/stream-note.c
@@ -166,3 +166,70 @@ void check_eof_notes_feof_or_no_error(void) {
}
fclose(F);
}
+
+void check_indeterminate_notes(void) {
+ FILE *F;
+ F = fopen("foo1.c", "r");
+ if (F == NULL) // expected-note {{Taking false branch}} \
+ // expected-note {{'F' is not equal to NULL}}
+ return;
+ int R = fgetc(F); // no note
+ if (R >= 0) { // expected-note {{Taking true branch}} \
+ // expected-note {{'R' is >= 0}}
+ fgetc(F); // expected-note {{Assuming this stream operation fails}}
+ if (ferror(F)) // expected-note {{Taking true branch}}
+ fgetc(F); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}} \
+ // expected-note {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+ }
+ fclose(F);
+}
+
+void check_indeterminate_after_clearerr(void) {
+ 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); // expected-note {{Assuming this stream operation fails}}
+ if (ferror(F)) { // expected-note {{Taking true branch}}
+ clearerr(F);
+ fread(Buf, 1, 1, F); // expected-warning {{might be 'indeterminate' after a failed operation}} \
+ // expected-note {{might be 'indeterminate' after a failed operation}}
+ }
+ fclose(F);
+}
+
+void check_indeterminate_eof(void) {
+ FILE *F;
+ char Buf[2];
+ F = fopen("foo1.c", "r");
+ if (F == NULL) // expected-note {{Taking false branch}} \
+ // expected-note {{'F' is not equal to NULL}} \
+ // expected-note {{Taking false branch}} \
+ // expected-note {{'F' is not equal to NULL}}
+ return;
+ fgets(Buf, sizeof(Buf), F); // expected-note {{Assuming this stream operation fails}} \
+ // expected-note {{Assuming stream reaches end-of-file here}}
+
+ fgets(Buf, sizeof(Buf), F); // expected-warning {{might be 'indeterminate'}} \
+ // expected-note {{might be 'indeterminate'}} \
+ // expected-warning {{stream is in EOF state}} \
+ // expected-note {{stream is in EOF state}}
+ fclose(F);
+}
+
+void check_indeterminate_fseek(void) {
+ FILE *F = fopen("file", "r");
+ if (!F) // expected-note {{Taking false branch}} \
+ // expected-note {{'F' is non-null}}
+ return;
+ int Ret = fseek(F, 1, SEEK_SET); // expected-note {{Assuming this stream operation fails}}
+ if (Ret) { // expected-note {{Taking true branch}} \
+ // expected-note {{'Ret' is not equal to 0}}
+ char Buf[2];
+ fwrite(Buf, 1, 2, F); // expected-warning {{might be 'indeterminate'}} \
+ // expected-note {{might be 'indeterminate'}}
+ }
+ fclose(F);
+}
``````````
</details>
https://github.com/llvm/llvm-project/pull/83288
More information about the cfe-commits
mailing list