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 @@ -92,7 +92,27 @@ /// 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 @@ // 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 @@ 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; @@ -279,6 +303,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. @@ -449,6 +483,9 @@ State = ensureStreamOpened(StreamVal, C, State); if (!State) return; + State = ensureNoFilePositionIndeterminate(StreamVal, C, State); + if (!State) + return; SymbolRef Sym = StreamVal.getAsSymbol(); if (Sym && State->get(Sym)) { @@ -470,6 +507,9 @@ State = ensureStreamOpened(StreamVal, C, State); if (!State) return; + State = ensureNoFilePositionIndeterminate(StreamVal, C, State); + if (!State) + return; C.addTransition(State); } @@ -548,7 +588,9 @@ 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(StreamSym, NewState); C.addTransition(StateFailed); } @@ -601,9 +643,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); @@ -623,7 +667,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); } @@ -651,7 +698,9 @@ // 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 && + !ErrorKind.isFEof()))); } if (StreamErrorState NewES = SS->ErrorState & (~ErrorKind)) { // Execution path(s) with ErrorKind not set. @@ -659,7 +708,9 @@ // 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 && !NewES.isFEof()))); } } @@ -767,6 +818,55 @@ 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.")); + + 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( + *BT_IndeterminatePosition, BT_IndeterminatePosition->getDescription(), + N)); + return State->set( + 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( + *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 --- 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,70 @@ } 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); +}