[clang] 9081fa2 - [Analyzer][StreamChecker] Added check for "indeterminate file position".
Balázs Kéri via cfe-commits
cfe-commits at lists.llvm.org
Wed May 27 23:21:01 PDT 2020
Author: Balázs Kéri
Date: 2020-05-28T08:21:57+02:00
New Revision: 9081fa20991d101728434b354a96283b26495b71
URL: https://github.com/llvm/llvm-project/commit/9081fa20991d101728434b354a96283b26495b71
DIFF: https://github.com/llvm/llvm-project/commit/9081fa20991d101728434b354a96283b26495b71.diff
LOG: [Analyzer][StreamChecker] Added check for "indeterminate file position".
Summary:
According to the standard, after a `wread` or `fwrite` call the file position
becomes "indeterminate". It is assumable that a next read or write causes
undefined behavior, so a (fatal error) warning is added for this case.
The indeterminate position can be cleared by some operations, for example
`fseek` or `freopen`, not with `clearerr`.
Reviewers: Szelethus, baloghadamsoftware, martong, NoQ, xazax.hun, dcoughlin
Reviewed By: Szelethus
Subscribers: rnkovacs, NoQ, xazax.hun, baloghadamsoftware, szepet, a.sidorin, mikhail.ramalho, Szelethus, donat.nagy, dkrupp, gamesh411, Charusso, martong, ASDenysPetrov, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D80018
Added:
Modified:
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
clang/test/Analysis/stream-error.c
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 2e90be4350a0..63ebfaf90dc8 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -92,7 +92,27 @@ struct StreamState {
/// State of the error flags.
/// Ignored in non-opened stream state but must be NoError.
- StreamErrorState ErrorState;
+ StreamErrorState const ErrorState;
+
+ /// Indicate if the file has an "indeterminate file position indicator".
+ /// This can be set at a failing read or write or seek operation.
+ /// If it is set no more read or write is allowed.
+ /// This value is not dependent on the stream error flags:
+ /// The error flag may be cleared with `clearerr` but the file position
+ /// remains still indeterminate.
+ /// This value applies to all error states in ErrorState except FEOF.
+ /// An EOF+indeterminate state is the same as EOF state.
+ bool const FilePositionIndeterminate = false;
+
+ StreamState(const FnDescription *L, KindTy S, const StreamErrorState &ES,
+ bool IsFilePositionIndeterminate)
+ : LastOperation(L), State(S), ErrorState(ES),
+ FilePositionIndeterminate(IsFilePositionIndeterminate) {
+ assert((!ES.isFEof() || !IsFilePositionIndeterminate) &&
+ "FilePositionIndeterminate should be false in FEof case.");
+ assert((State == Opened || ErrorState.isNoError()) &&
+ "ErrorState should be None in non-opened stream state.");
+ }
bool isOpened() const { return State == Opened; }
bool isClosed() const { return State == Closed; }
@@ -102,24 +122,27 @@ struct StreamState {
// In not opened state error state should always NoError, so comparison
// here is no problem.
return LastOperation == X.LastOperation && State == X.State &&
- ErrorState == X.ErrorState;
+ ErrorState == X.ErrorState &&
+ FilePositionIndeterminate == X.FilePositionIndeterminate;
}
static StreamState getOpened(const FnDescription *L,
- const StreamErrorState &ES = {}) {
- return StreamState{L, Opened, ES};
+ const StreamErrorState &ES = ErrorNone,
+ bool IsFilePositionIndeterminate = false) {
+ return StreamState{L, Opened, ES, IsFilePositionIndeterminate};
}
static StreamState getClosed(const FnDescription *L) {
- return StreamState{L, Closed, {}};
+ return StreamState{L, Closed, {}, false};
}
static StreamState getOpenFailed(const FnDescription *L) {
- return StreamState{L, OpenFailed, {}};
+ return StreamState{L, OpenFailed, {}, false};
}
void Profile(llvm::FoldingSetNodeID &ID) const {
ID.AddPointer(LastOperation);
ID.AddInteger(State);
ID.AddInteger(ErrorState);
+ ID.AddBoolean(FilePositionIndeterminate);
}
};
@@ -173,7 +196,8 @@ ProgramStateRef bindInt(uint64_t Value, ProgramStateRef State,
class StreamChecker
: public Checker<check::PreCall, eval::Call, check::DeadSymbols> {
mutable std::unique_ptr<BuiltinBug> BT_nullfp, BT_illegalwhence,
- BT_UseAfterClose, BT_UseAfterOpenFailed, BT_ResourceLeak, BT_StreamEof;
+ BT_UseAfterClose, BT_UseAfterOpenFailed, BT_ResourceLeak, BT_StreamEof,
+ BT_IndeterminatePosition;
public:
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
@@ -279,6 +303,16 @@ class StreamChecker
ProgramStateRef ensureStreamOpened(SVal StreamVal, CheckerContext &C,
ProgramStateRef State) const;
+ /// Check that the stream has not an invalid ("indeterminate") file position,
+ /// generate warning for it.
+ /// (EOF is not an invalid position.)
+ /// The returned state can be nullptr if a fatal error was generated.
+ /// It can return non-null state if the stream has not an invalid position or
+ /// there is execution path with non-invalid position.
+ ProgramStateRef
+ ensureNoFilePositionIndeterminate(SVal StreamVal, CheckerContext &C,
+ ProgramStateRef State) const;
+
/// Check the legality of the 'whence' argument of 'fseek'.
/// Generate error and return nullptr if it is found to be illegal.
/// Otherwise returns the state.
@@ -447,6 +481,9 @@ void StreamChecker::preFread(const FnDescription *Desc, const CallEvent &Call,
if (!State)
return;
State = ensureStreamOpened(StreamVal, C, State);
+ if (!State)
+ return;
+ State = ensureNoFilePositionIndeterminate(StreamVal, C, State);
if (!State)
return;
@@ -468,6 +505,9 @@ void StreamChecker::preFwrite(const FnDescription *Desc, const CallEvent &Call,
if (!State)
return;
State = ensureStreamOpened(StreamVal, C, State);
+ if (!State)
+ return;
+ State = ensureNoFilePositionIndeterminate(StreamVal, C, State);
if (!State)
return;
@@ -548,7 +588,9 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
NewES = (SS->ErrorState == ErrorFEof) ? ErrorFEof : ErrorFEof | ErrorFError;
else
NewES = ErrorFError;
- StreamState NewState = StreamState::getOpened(Desc, NewES);
+ // 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);
}
@@ -601,9 +643,11 @@ void StreamChecker::evalFseek(const FnDescription *Desc, const CallEvent &Call,
StateNotFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
// We get error.
// It is possible that fseek fails but sets none of the error flags.
+ // If fseek failed, assume that the file position becomes indeterminate in any
+ // case.
StateFailed = StateFailed->set<StreamMap>(
StreamSym,
- StreamState::getOpened(Desc, ErrorNone | ErrorFEof | ErrorFError));
+ StreamState::getOpened(Desc, ErrorNone | ErrorFEof | ErrorFError, true));
C.addTransition(StateNotFailed);
C.addTransition(StateFailed);
@@ -623,7 +667,10 @@ void StreamChecker::evalClearerr(const FnDescription *Desc,
assertStreamStateOpened(SS);
- State = State->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
+ // FilePositionIndeterminate is not cleared.
+ State = State->set<StreamMap>(
+ StreamSym,
+ StreamState::getOpened(Desc, ErrorNone, SS->FilePositionIndeterminate));
C.addTransition(State);
}
@@ -651,7 +698,9 @@ void StreamChecker::evalFeofFerror(const FnDescription *Desc,
// From now on it is the only one error state.
ProgramStateRef TrueState = bindAndAssumeTrue(State, C, CE);
C.addTransition(TrueState->set<StreamMap>(
- StreamSym, StreamState::getOpened(Desc, ErrorKind)));
+ StreamSym, StreamState::getOpened(Desc, ErrorKind,
+ SS->FilePositionIndeterminate &&
+ !ErrorKind.isFEof())));
}
if (StreamErrorState NewES = SS->ErrorState & (~ErrorKind)) {
// Execution path(s) with ErrorKind not set.
@@ -659,7 +708,9 @@ void StreamChecker::evalFeofFerror(const FnDescription *Desc,
// New error state is everything before minus ErrorKind.
ProgramStateRef FalseState = bindInt(0, State, C, CE);
C.addTransition(FalseState->set<StreamMap>(
- StreamSym, StreamState::getOpened(Desc, NewES)));
+ StreamSym,
+ StreamState::getOpened(
+ Desc, NewES, SS->FilePositionIndeterminate && !NewES.isFEof())));
}
}
@@ -767,6 +818,55 @@ ProgramStateRef StreamChecker::ensureStreamOpened(SVal StreamVal,
return State;
}
+ProgramStateRef StreamChecker::ensureNoFilePositionIndeterminate(
+ SVal StreamVal, CheckerContext &C, ProgramStateRef State) const {
+ SymbolRef Sym = StreamVal.getAsSymbol();
+ if (!Sym)
+ return State;
+
+ const StreamState *SS = State->get<StreamMap>(Sym);
+ if (!SS)
+ return State;
+
+ assert(SS->isOpened() && "First ensure that stream is opened.");
+
+ if (SS->FilePositionIndeterminate) {
+ if (!BT_IndeterminatePosition)
+ BT_IndeterminatePosition.reset(
+ new BuiltinBug(this, "Invalid stream state",
+ "File position of the stream might be 'indeterminate' "
+ "after a failed operation. "
+ "Can cause undefined behavior."));
+
+ if (SS->ErrorState & ErrorFEof) {
+ // The error is unknown but may be FEOF.
+ // Continue analysis with the FEOF error state.
+ // Report warning because the other possible error states.
+ ExplodedNode *N = C.generateNonFatalErrorNode(State);
+ if (!N)
+ return nullptr;
+
+ C.emitReport(std::make_unique<PathSensitiveBugReport>(
+ *BT_IndeterminatePosition, BT_IndeterminatePosition->getDescription(),
+ N));
+ 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, BT_IndeterminatePosition->getDescription(),
+ N));
+
+ return nullptr;
+ }
+
+ return State;
+}
+
ProgramStateRef
StreamChecker::ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C,
ProgramStateRef State) const {
diff --git a/clang/test/Analysis/stream-error.c b/clang/test/Analysis/stream-error.c
index cc0147deafdf..e91ab2c6c28c 100644
--- a/clang/test/Analysis/stream-error.c
+++ b/clang/test/Analysis/stream-error.c
@@ -76,7 +76,7 @@ void error_fread() {
}
if (ferror(F)) {
clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
- fread(Buf, 1, 10, F); // no warning
+ fread(Buf, 1, 10, F); // expected-warning {{might be 'indeterminate'}}
}
}
fclose(F);
@@ -94,7 +94,7 @@ void error_fwrite() {
} else {
clang_analyzer_eval(feof(F)); // expected-warning {{FALSE}}
clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
- fwrite(0, 1, 10, F); // no warning
+ fwrite(0, 1, 10, F); // expected-warning {{might be 'indeterminate'}}
}
fclose(F);
Ret = fwrite(0, 1, 10, F); // expected-warning {{Stream might be already closed}}
@@ -166,3 +166,70 @@ void error_fseek() {
}
fclose(F);
}
+
+void error_indeterminate() {
+ FILE *F = fopen("file", "r+");
+ if (!F)
+ return;
+ const char *Buf = "123456789";
+ int rc = fseek(F, 0, SEEK_SET);
+ if (rc) {
+ if (feof(F)) {
+ fwrite(Buf, 1, 10, F); // no warning
+ } else if (ferror(F)) {
+ fwrite(Buf, 1, 10, F); // expected-warning {{might be 'indeterminate'}}
+ } else {
+ fwrite(Buf, 1, 10, F); // expected-warning {{might be 'indeterminate'}}
+ }
+ }
+ fclose(F);
+}
+
+void error_indeterminate_clearerr() {
+ FILE *F = fopen("file", "r+");
+ if (!F)
+ return;
+ const char *Buf = "123456789";
+ int rc = fseek(F, 0, SEEK_SET);
+ if (rc) {
+ if (feof(F)) {
+ clearerr(F);
+ fwrite(Buf, 1, 10, F); // no warning
+ } else if (ferror(F)) {
+ clearerr(F);
+ fwrite(Buf, 1, 10, F); // expected-warning {{might be 'indeterminate'}}
+ } else {
+ clearerr(F);
+ fwrite(Buf, 1, 10, F); // expected-warning {{might be 'indeterminate'}}
+ }
+ }
+ fclose(F);
+}
+
+void error_indeterminate_feof1() {
+ FILE *F = fopen("file", "r+");
+ if (!F)
+ return;
+ char Buf[10];
+ if (fread(Buf, 1, 10, F) < 10) {
+ if (feof(F)) {
+ // error is feof, should be non-indeterminate
+ fwrite("1", 1, 1, F); // no warning
+ }
+ }
+ fclose(F);
+}
+
+void error_indeterminate_feof2() {
+ FILE *F = fopen("file", "r+");
+ if (!F)
+ return;
+ char Buf[10];
+ if (fread(Buf, 1, 10, F) < 10) {
+ if (ferror(F) == 0) {
+ // error is feof, should be non-indeterminate
+ fwrite("1", 1, 1, F); // no warning
+ }
+ }
+ fclose(F);
+}
More information about the cfe-commits
mailing list