balazske created this revision.
Herald added subscribers: cfe-commits, ASDenysPetrov, martong, Charusso,
gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet,
baloghadamsoftware, xazax.hun.
Herald added a reviewer: Szelethus.
Herald added a project: clang.
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`.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D80018
Files:
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
clang/test/Analysis/stream-error.c
Index: clang/test/Analysis/stream-error.c
===================================================================
--- clang/test/Analysis/stream-error.c
+++ clang/test/Analysis/stream-error.c
@@ -76,7 +76,7 @@
}
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 @@
} 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,36 @@
}
fclose(F);
}
+
+void error_fseek_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_clearerr_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)) {
+ clearerr(F);
+ fwrite(Buf, 1, 10, F); // expected-warning {{might be 'indeterminate'}}
+ }
+ }
+ fclose(F);
+}
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -97,6 +97,16 @@
/// Ignored in non-opened stream state but must be NoError.
StreamErrorState 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 FilePositionIndeterminate = false;
+
bool isOpened() const { return State == Opened; }
bool isClosed() const { return State == Closed; }
bool isOpenFailed() const { return State == OpenFailed; }
@@ -105,12 +115,14 @@
// 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 FPI = false) {
+ return StreamState{L, Opened, ES, FPI};
}
static StreamState getClosed(const FnDescription *L) {
return StreamState{L, Closed};
@@ -123,6 +135,7 @@
ID.AddPointer(LastOperation);
ID.AddInteger(State);
ID.AddInteger(ErrorState);
+ ID.AddBoolean(FilePositionIndeterminate);
}
};
@@ -176,7 +189,8 @@
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;
@@ -282,6 +296,16 @@
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.
@@ -450,6 +474,9 @@
if (!State)
return;
State = ensureStreamOpened(StreamVal, C, State);
+ if (!State)
+ return;
+ State = ensureNoFilePositionIndeterminate(StreamVal, C, State);
if (!State)
return;
@@ -471,6 +498,9 @@
if (!State)
return;
State = ensureStreamOpened(StreamVal, C, State);
+ if (!State)
+ return;
+ State = ensureNoFilePositionIndeterminate(StreamVal, C, State);
if (!State)
return;
@@ -552,7 +582,11 @@
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.
+ // The indeterminate flag is set here for any error but is ignored for EOF by
+ // other code.
+ StreamState NewState = StreamState::getOpened(Desc, NewES, true);
StateFailed = StateFailed->set<StreamMap>(StreamSym, NewState);
C.addTransition(StateFailed);
}
@@ -605,9 +639,11 @@
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);
@@ -627,7 +663,10 @@
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);
}
@@ -655,7 +694,8 @@
// 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)));
}
if (StreamErrorState NewES = SS->ErrorState & (~ErrorKind)) {
// Execution path(s) with ErrorKind not set.
@@ -663,7 +703,8 @@
// 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)));
}
}
@@ -771,6 +812,65 @@
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."));
+
+ // Clear FilePositionIndeterminate from state.
+ StreamState NewState =
+ StreamState::getOpened(SS->LastOperation, SS->ErrorState, false);
+
+ // If only FEof is possible, report no warning (indeterminate position in
+ // FEof is ignored).
+ if (SS->ErrorState.isFEof())
+ return State->set<StreamMap>(Sym, NewState);
+
+ // If FEof is not possible, warn for indeterminate position and stop
+ // analysis.
+ if (!(SS->ErrorState & ErrorFEof)) {
+ ExplodedNode *N = C.generateErrorNode(State);
+ if (N) {
+ C.emitReport(std::make_unique<PathSensitiveBugReport>(
+ *BT_IndeterminatePosition,
+ BT_IndeterminatePosition->getDescription(), N));
+ return nullptr;
+ }
+ return State; // FIXME: nullptr?
+ }
+
+ // FEof and other states are possible.
+ // The path with FEof is the one that can continue.
+ // For this reason a non-fatal error is generated to continue the analysis
+ // with only FEof state set.
+ ExplodedNode *N = C.generateNonFatalErrorNode(State);
+ if (N) {
+ C.emitReport(std::make_unique<PathSensitiveBugReport>(
+ *BT_IndeterminatePosition, BT_IndeterminatePosition->getDescription(),
+ N));
+ }
+ NewState.ErrorState = ErrorFEof;
+ return State->set<StreamMap>(Sym, NewState);
+ }
+
+ return State;
+}
+
ProgramStateRef
StreamChecker::ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C,
ProgramStateRef State) const {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
