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 @@ -19,6 +19,7 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h" +#include "llvm/ADT/DenseMap.h" #include using namespace clang; @@ -231,8 +232,6 @@ /// If true, evaluate special testing stream functions. bool TestMode = false; - const BugType *getBT_StreamEof() const { return &BT_StreamEof; } - private: CallDescriptionMap FnDescriptions = { {{"fopen"}, {nullptr, &StreamChecker::evalFopen, ArgNone}}, @@ -256,11 +255,13 @@ {&StreamChecker::preDefault, &StreamChecker::evalClearerr, 0}}, {{"feof", 1}, {&StreamChecker::preDefault, - std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFEof), + std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFEof, + FEofNoteMessages), 0}}, {{"ferror", 1}, {&StreamChecker::preDefault, - std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFError), + std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFError, + FErrorNoteMessages), 0}}, {{"fileno", 1}, {&StreamChecker::preDefault, nullptr, 0}}, }; @@ -277,6 +278,15 @@ 0}}, }; + const char *FEofNoteMessages[2] = { + "Assuming the end-of-file flag is set on the stream", + "Assuming the end-of-file flag is not set on the stream", + }; + const char *FErrorNoteMessages[2] = { + "Assuming the error flag is set on the stream", + "Assuming the error flag is not set on the stream", + }; + void evalFopen(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const; @@ -309,8 +319,8 @@ CheckerContext &C) const; void evalFeofFerror(const FnDescription *Desc, const CallEvent &Call, - CheckerContext &C, - const StreamErrorState &ErrorKind) const; + CheckerContext &C, const StreamErrorState &ErrorKind, + const char *NoteMessages[2]) const; void evalSetFeofFerror(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C, @@ -376,37 +386,40 @@ return FnDescriptions.lookup(Call); } - /// Generate a message for BugReporterVisitor if the stored symbol is - /// marked as interesting by the actual bug report. - // FIXME: Use lambda instead. - struct NoteFn { - const BugType *BT_ResourceLeak; - SymbolRef StreamSym; - std::string Message; - - std::string operator()(PathSensitiveBugReport &BR) const { - if (BR.isInteresting(StreamSym) && &BR.getBugType() == BT_ResourceLeak) - return Message; + /// Create a NoteTag to display a note if a later bug report is generated. + /// A NoteTag is added at every stream operation that fails in some way or + /// causes a later failure (bug). Successful opening a stream is a "failure" + /// in this sense if a resource leak is detected later. + /// At a bug report the "failed operation" is always the last in the path + /// (where this note is displayed) and previous such notes are not displayed. + const NoteTag *constructFailureNoteTag(CheckerContext &C, SymbolRef StreamSym, + const char *Message) const { + + return C.getNoteTag([StreamSym, Message](PathSensitiveBugReport &BR) { + if (!BR.isInteresting(StreamSym)) + return ""; - return ""; - } - }; + // A failing operation is always the last failable backwards. + // Stop reporting the previous (failable) operations. + BR.markNotInteresting(StreamSym); - const NoteTag *constructNoteTag(CheckerContext &C, SymbolRef StreamSym, - const std::string &Message) const { - return C.getNoteTag(NoteFn{&BT_ResourceLeak, StreamSym, Message}); + return Message; + }); } - 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 ""; + /// Construct a NoteTag to display a message if any bug is detected later on + /// the path (if no other failing operation follows). + /// This note is inserted into places where something important about + /// the last failing operation is discovered. + const NoteTag *constructNonFailureNoteTag(CheckerContext &C, + SymbolRef StreamSym, + const char *Message) const { - BR.markNotInteresting(StreamSym); + return C.getNoteTag([StreamSym, Message](PathSensitiveBugReport &BR) { + if (!BR.isInteresting(StreamSym)) + return ""; - return "Assuming stream reaches end-of-file here"; + return Message; }); } @@ -500,8 +513,9 @@ StateNull->set(RetSym, StreamState::getOpenFailed(Desc)); C.addTransition(StateNotNull, - constructNoteTag(C, RetSym, "Stream opened here")); - C.addTransition(StateNull); + constructFailureNoteTag(C, RetSym, "Stream opened here")); + C.addTransition(StateNull, + constructFailureNoteTag(C, RetSym, "Stream open fails here")); } void StreamChecker::preFreopen(const FnDescription *Desc, const CallEvent &Call, @@ -557,8 +571,9 @@ StateRetNull->set(StreamSym, StreamState::getOpenFailed(Desc)); C.addTransition(StateRetNotNull, - constructNoteTag(C, StreamSym, "Stream reopened here")); - C.addTransition(StateRetNull); + constructFailureNoteTag(C, StreamSym, "Stream opened here")); + C.addTransition(StateRetNull, constructFailureNoteTag( + C, StreamSym, "Stream reopen fails here")); } void StreamChecker::evalFclose(const FnDescription *Desc, const CallEvent &Call, @@ -579,7 +594,7 @@ // and can not be used any more. State = State->set(Sym, StreamState::getClosed(Desc)); - C.addTransition(State); + C.addTransition(State, constructFailureNoteTag(C, Sym, "Stream closed here")); } void StreamChecker::preFread(const FnDescription *Desc, const CallEvent &Call, @@ -705,9 +720,13 @@ StreamState NewSS = StreamState::getOpened(Desc, NewES, !NewES.isFEof()); StateFailed = StateFailed->set(StreamSym, NewSS); if (IsFread && OldSS->ErrorState != ErrorFEof) - C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym)); + C.addTransition(StateFailed, + constructFailureNoteTag( + C, StreamSym, + "Stream reaches end-of-file or operation fails here")); else - C.addTransition(StateFailed); + C.addTransition(StateFailed, constructFailureNoteTag( + C, StreamSym, "Operation fails here")); } void StreamChecker::preFseek(const FnDescription *Desc, const CallEvent &Call, @@ -766,7 +785,10 @@ StreamState::getOpened(Desc, ErrorNone | ErrorFEof | ErrorFError, true)); C.addTransition(StateNotFailed); - C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym)); + C.addTransition( + StateFailed, + constructFailureNoteTag( + C, StreamSym, "Operation fails or stream reaches end-of-file here")); } void StreamChecker::evalClearerr(const FnDescription *Desc, @@ -792,7 +814,8 @@ void StreamChecker::evalFeofFerror(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C, - const StreamErrorState &ErrorKind) const { + const StreamErrorState &ErrorKind, + const char *NoteMessages[2]) const { ProgramStateRef State = C.getState(); SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol(); if (!StreamSym) @@ -813,20 +836,24 @@ // Function returns true. // From now on it is the only one error state. ProgramStateRef TrueState = bindAndAssumeTrue(State, C, CE); - C.addTransition(TrueState->set( + TrueState = TrueState->set( StreamSym, StreamState::getOpened(Desc, ErrorKind, SS->FilePositionIndeterminate && - !ErrorKind.isFEof()))); + !ErrorKind.isFEof())); + C.addTransition(TrueState, + constructNonFailureNoteTag(C, StreamSym, NoteMessages[0])); } if (StreamErrorState NewES = SS->ErrorState & (~ErrorKind)) { // Execution path(s) with ErrorKind not set. // Function returns false. // New error state is everything before minus ErrorKind. ProgramStateRef FalseState = bindInt(0, State, C, CE); - C.addTransition(FalseState->set( + FalseState = FalseState->set( StreamSym, StreamState::getOpened( - Desc, NewES, SS->FilePositionIndeterminate && !NewES.isFEof()))); + Desc, NewES, SS->FilePositionIndeterminate && !NewES.isFEof())); + C.addTransition(FalseState, + constructNonFailureNoteTag(C, StreamSym, NoteMessages[1])); } } @@ -901,9 +928,11 @@ // according to cppreference.com . ExplodedNode *N = C.generateErrorNode(); if (N) { - C.emitReport(std::make_unique( + auto R = std::make_unique( BT_UseAfterClose, - "Stream might be already closed. Causes undefined behaviour.", N)); + "Stream might be already closed. Causes undefined behaviour.", N); + R->markInteresting(Sym); + C.emitReport(std::move(R)); return nullptr; } @@ -917,12 +946,14 @@ // failed to open. ExplodedNode *N = C.generateErrorNode(); if (N) { - C.emitReport(std::make_unique( + auto R = std::make_unique( BT_UseAfterOpenFailed, "Stream might be invalid after " "(re-)opening it has failed. " "Can cause undefined behaviour.", - N)); + N); + R->markInteresting(Sym); + C.emitReport(std::move(R)); return nullptr; } return State; @@ -957,8 +988,10 @@ if (!N) return nullptr; - C.emitReport(std::make_unique( - BT_IndeterminatePosition, BugMessage, N)); + auto R = std::make_unique( + BT_IndeterminatePosition, BugMessage, N); + R->markInteresting(Sym); + C.emitReport(std::move(R)); return State->set( Sym, StreamState::getOpened(SS->LastOperation, ErrorFEof, false)); } @@ -966,9 +999,12 @@ // 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, BugMessage, N)); + if (N) { + auto R = std::make_unique( + 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 --- a/clang/test/Analysis/stream-note.c +++ b/clang/test/Analysis/stream-note.c @@ -33,12 +33,12 @@ // expected-note@-2 {{Opened stream never closed. Potential resource leak}} void check_note_freopen() { - FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}} + FILE *F = fopen("file", "r"); if (!F) // expected-note@-1 {{'F' is non-null}} // expected-note@-2 {{Taking false branch}} return; - F = freopen(0, "w", F); // expected-note {{Stream reopened here}} + F = freopen(0, "w", F); // expected-note {{Stream opened here}} if (!F) // expected-note@-1 {{'F' is non-null}} // expected-note@-2 {{Taking false branch}} @@ -47,6 +47,33 @@ // expected-warning@-1 {{Opened stream never closed. Potential resource leak}} // expected-note@-2 {{Opened stream never closed. Potential resource leak}} +void check_note_fopen_fail() { + FILE *F = fopen("file", "r"); // expected-note {{Stream open fails here}} expected-note {{Assuming pointer value is null}} expected-note {{'F' initialized here}} + fclose(F); // expected-warning {{Stream pointer might be NULL}} + // expected-note@-1 {{Stream pointer might be NULL}} +} + +void check_note_freopen_fail() { + FILE *F = fopen("file", "r"); + if (!F) // expected-note {{'F' is non-null}} expected-note {{Taking false branch}} + return; + freopen(0, "w", F); // expected-note {{Stream reopen fails here}} + fclose(F); // expected-warning {{Stream might be invalid after (re-)opening it has failed. Can cause undefined behaviour}} + // expected-note@-1 {{Stream might be invalid after (re-)opening it has failed. Can cause undefined behaviour}} +} + +void check_note_freopen_fail_null() { + // FIXME: The following note should not be here. + FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}} + if (!F) // expected-note {{'F' is non-null}} expected-note {{Taking false branch}} + return; + // FIXME: Note about failing 'freopen' belongs here. + // Why is a note at 'fopen'? + F = freopen(0, "w", F); // expected-note {{Null pointer value stored to 'F'}} + fclose(F); // expected-warning {{Stream pointer might be NULL}} + // expected-note@-1 {{Stream pointer might be NULL}} +} + void check_note_leak_2(int c) { FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}} if (!F1) @@ -80,7 +107,7 @@ void check_track_null() { FILE *F; - F = fopen("foo1.c", "r"); // expected-note {{Value assigned to 'F'}} expected-note {{Assuming pointer value is null}} + F = fopen("foo1.c", "r"); // expected-note {{Value assigned to 'F'}} expected-note {{Assuming pointer value is null}} expected-note {{Stream open fails here}} if (F != NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is equal to NULL}} fclose(F); return; @@ -99,8 +126,8 @@ fread(Buf, 1, 1, F); if (feof(F)) { // expected-note {{Taking true branch}} clearerr(F); - fread(Buf, 1, 1, F); // expected-note {{Assuming stream reaches end-of-file here}} - if (feof(F)) { // expected-note {{Taking true branch}} + fread(Buf, 1, 1, F); // expected-note {{Stream reaches end-of-file or operation fails here}} + if (feof(F)) { // expected-note {{Assuming the end-of-file flag is set on the stream}} expected-note {{Taking true branch}} fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}} // expected-note@-1 {{Read function called when stream is in EOF state. Function has no effect}} } @@ -123,8 +150,8 @@ fclose(F); return; } - fread(Buf, 1, 1, F); // expected-note {{Assuming stream reaches end-of-file here}} - if (feof(F)) { // expected-note {{Taking true branch}} + fread(Buf, 1, 1, F); // expected-note {{Stream reaches end-of-file or operation fails here}} + if (feof(F)) { // expected-note {{Assuming the end-of-file flag is set on the stream}} expected-note {{Taking true branch}} fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}} // expected-note@-1 {{Read function called when stream is in EOF state. Function has no effect}} } @@ -137,11 +164,71 @@ F = fopen("foo1.c", "r"); if (F == NULL) // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}} return; - int RRet = fread(Buf, 1, 1, F); // expected-note {{Assuming stream reaches end-of-file here}} - if (ferror(F)) { // expected-note {{Taking false branch}} + int RRet = fread(Buf, 1, 1, F); // expected-note {{Stream reaches end-of-file or operation fails here}} + if (ferror(F)) { // expected-note {{Assuming the error flag is not set on the stream}} expected-note {{Taking false branch}} } else { fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}} // expected-note@-1 {{Read function called when stream is in EOF state. Function has no effect}} } fclose(F); } + +void check_indeterminate_notes_only_at_last_failure() { + FILE *F; + char Buf[10]; + F = fopen("foo1.c", "r"); + if (!F) // expected-note {{Taking false branch}} expected-note {{'F' is non-null}} + return; + fread(Buf, 1, 1, F); + if (ferror(F)) { // expected-note {{Taking true branch}} + F = freopen(0, "w", F); + if (!F) // expected-note {{Taking false branch}} expected-note {{'F' is non-null}} + return; + fread(Buf, 1, 1, F); // expected-note {{Stream reaches end-of-file or operation fails here}} + if (ferror(F)) { // expected-note {{Assuming the error flag is set on the stream}} expected-note{{Taking true branch}} + fread(Buf, 1, 1, F); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}} + // expected-note@-1 {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}} + } + } + fclose(F); +} + +void check_indeterminate_notes_fseek() { + FILE *F; + char Buf[10]; + F = fopen("foo1.c", "r"); + if (!F) // expected-note {{Taking false branch}} expected-note {{'F' is non-null}} + return; + fseek(F, 1, SEEK_SET); // expected-note {{Operation fails or stream reaches end-of-file here}} + if (!feof(F)) // expected-note{{Assuming the end-of-file flag is not set on the stream}} expected-note {{Taking true branch}} + fread(Buf, 1, 1, F); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}} + // expected-note@-1 {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}} + fclose(F); +} + +void check_indeterminate_notes_fwrite() { + FILE *F; + char Buf[10]; + F = fopen("foo1.c", "r"); + if (!F) // expected-note {{Taking false branch}} expected-note {{'F' is non-null}} + return; + fwrite(Buf, 1, 1, F); // expected-note {{Operation fails here}} + if (ferror(F)) // expected-note {{Assuming the error flag is set on the stream}} expected-note {{Taking true branch}} + fread(Buf, 1, 1, F); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}} + // expected-note@-1 {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}} + fclose(F); +} + +void check_indeterminate_notes_fseek_no_feof_no_ferror() { + FILE *F; + char Buf[10]; + F = fopen("foo1.c", "r"); + if (!F) // expected-note {{Taking false branch}} expected-note {{'F' is non-null}} + return; + fseek(F, 1, SEEK_SET); // expected-note {{Operation fails or stream reaches end-of-file here}} + if (!ferror(F) && !feof(F)) // expected-note {{Assuming the error flag is not set on the stream}} expected-note {{Assuming the end-of-file flag is not set on the stream}} + // expected-note@-1 {{Taking true branch}} expected-note@-1 {{Left side of '&&' is true}} + fread(Buf, 1, 1, F); // expected-warning{{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}} + // expected-note@-1{{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}} + fclose(F); +}