diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/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 { mutable std::unique_ptr 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(StreamSym, NewState); C.addTransition(StateFailed); } @@ -605,9 +639,11 @@ StateNotFailed->set(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( 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(StreamSym, StreamState::getOpened(Desc)); + // FilePositionIndeterminate is not cleared. + State = State->set( + 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( - 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( - 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(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(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( + *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( + *BT_IndeterminatePosition, BT_IndeterminatePosition->getDescription(), + N)); + } + NewState.ErrorState = ErrorFEof; + return State->set(Sym, NewState); + } + + 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 --- a/clang/test/Analysis/stream-error.c +++ b/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); +}