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,7 +232,20 @@ /// If true, evaluate special testing stream functions. bool TestMode = false; - const BugType *getBT_StreamEof() const { return &BT_StreamEof; } + using BugMessageMap = llvm::DenseMap; + /// At any stream operation that can cause (multiple type of) bugs, we can + /// determine the failure reason text by only knowing the bug type. The same + /// text is applicable at every operation that may cause that bug. This map + /// is used to lookup the message text in a note tag that is added at the + /// failing operation. + const BugMessageMap BugMessages = { + {&BT_FileNull, "Assuming opening the stream fails here"}, + {&BT_UseAfterClose, "Stream closed here"}, + {&BT_UseAfterOpenFailed, "Assuming opening the stream fails here"}, + {&BT_IndeterminatePosition, "Assuming this stream operation fails and " + "leaves the file position indeterminate"}, + {&BT_StreamEof, "Assuming stream reaches end-of-file here"}, + {&BT_ResourceLeak, "Stream opened here"}}; private: CallDescriptionMap FnDescriptions = { @@ -256,11 +270,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 +293,15 @@ 0}}, }; + const char *FEofNoteMessages[2] = { + "The end-of-file flag is set on the stream", + "The end-of-file flag is not set on the stream", + }; + const char *FErrorNoteMessages[2] = { + "The error flag is set on the stream", + "The error flag is not set on the stream", + }; + void evalFopen(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const; @@ -309,8 +334,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 +401,41 @@ 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; + /// 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 last operation in the path that has added this kind of + /// NoteTag is the one that caused the bug. It is enough to know the bug type + /// to determine the note tag text. + const NoteTag *constructFailureNoteTag(CheckerContext &C, + SymbolRef StreamSym) const { - std::string operator()(PathSensitiveBugReport &BR) const { - if (BR.isInteresting(StreamSym) && &BR.getBugType() == BT_ResourceLeak) - return Message; + return C.getNoteTag([this, StreamSym](PathSensitiveBugReport &BR) { + if (!BR.isInteresting(StreamSym)) + return ""; - return ""; - } - }; + // This is done to make the report only at the last location with the same + // note tag. + BR.markNotInteresting(StreamSym); - const NoteTag *constructNoteTag(CheckerContext &C, SymbolRef StreamSym, - const std::string &Message) const { - return C.getNoteTag(NoteFn{&BT_ResourceLeak, StreamSym, Message}); + return this->BugMessages.lookup(&BR.getBugType()); + }); } - 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 (that can be reason of a bug) 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; }); } @@ -499,9 +528,8 @@ StateNull = StateNull->set(RetSym, StreamState::getOpenFailed(Desc)); - C.addTransition(StateNotNull, - constructNoteTag(C, RetSym, "Stream opened here")); - C.addTransition(StateNull); + C.addTransition(StateNotNull, constructFailureNoteTag(C, RetSym)); + C.addTransition(StateNull, constructFailureNoteTag(C, RetSym)); } void StreamChecker::preFreopen(const FnDescription *Desc, const CallEvent &Call, @@ -556,9 +584,8 @@ StateRetNull = StateRetNull->set(StreamSym, StreamState::getOpenFailed(Desc)); - C.addTransition(StateRetNotNull, - constructNoteTag(C, StreamSym, "Stream reopened here")); - C.addTransition(StateRetNull); + C.addTransition(StateRetNotNull, constructFailureNoteTag(C, StreamSym)); + C.addTransition(StateRetNull, constructFailureNoteTag(C, StreamSym)); } void StreamChecker::evalFclose(const FnDescription *Desc, const CallEvent &Call, @@ -579,7 +606,7 @@ // and can not be used any more. State = State->set(Sym, StreamState::getClosed(Desc)); - C.addTransition(State); + C.addTransition(State, constructFailureNoteTag(C, Sym)); } void StreamChecker::preFread(const FnDescription *Desc, const CallEvent &Call, @@ -704,10 +731,7 @@ // indicator for the stream is indeterminate. StreamState NewSS = StreamState::getOpened(Desc, NewES, !NewES.isFEof()); StateFailed = StateFailed->set(StreamSym, NewSS); - if (IsFread && OldSS->ErrorState != ErrorFEof) - C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym)); - else - C.addTransition(StateFailed); + C.addTransition(StateFailed, constructFailureNoteTag(C, StreamSym)); } void StreamChecker::preFseek(const FnDescription *Desc, const CallEvent &Call, @@ -766,7 +790,7 @@ StreamState::getOpened(Desc, ErrorNone | ErrorFEof | ErrorFError, true)); C.addTransition(StateNotFailed); - C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym)); + C.addTransition(StateFailed, constructFailureNoteTag(C, StreamSym)); } void StreamChecker::evalClearerr(const FnDescription *Desc, @@ -792,7 +816,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 +838,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 +930,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 +948,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 +990,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 +1001,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,32 @@ // 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 {{Assuming opening the stream 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 {{Assuming opening the stream 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 {{Assuming opening the stream fails here}} + if (!F) // expected-note {{'F' is non-null}} expected-note {{Taking false branch}} + return; + // FIXME: Note about failing open belongs here. + 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 +106,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 {{Assuming opening the stream fails here}} if (F != NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is equal to NULL}} fclose(F); return; @@ -100,7 +126,7 @@ 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}} + if (feof(F)) { // expected-note {{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}} } @@ -124,7 +150,7 @@ return; } fread(Buf, 1, 1, F); // expected-note {{Assuming stream reaches end-of-file here}} - if (feof(F)) { // expected-note {{Taking true branch}} + if (feof(F)) { // expected-note {{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}} } @@ -138,10 +164,99 @@ 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}} + if (ferror(F)) { // expected-note {{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 {{Assuming this stream operation fails and leaves the file position indeterminate}} + if (ferror(F)) { // expected-note {{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 {{Assuming this stream operation fails and leaves the file position indeterminate}} + if (!feof(F)) // expected-note{{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 {{Assuming this stream operation fails and leaves the file position indeterminate}} + if (ferror(F)) // expected-note {{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 {{Assuming this stream operation fails and leaves the file position indeterminate}} + if (!ferror(F) && !feof(F)) // expected-note {{The error flag is not set on the stream}} expected-note {{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); +} + +void check_feof_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 {{Assuming stream reaches end-of-file here}} + if (feof(F)) // expected-note{{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}} + fclose(F); +} + +void check_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}} + // expected-note@-1 {{Taking false branch}} expected-note@-1 {{'F' is non-null}} + return; + fseek(F, 1, SEEK_SET); // expected-note {{Assuming this stream operation fails and leaves the file position indeterminate}} + // expected-note@-1 {{Assuming stream reaches end-of-file here}} + 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}} + // expected-warning@-2 {{Read function called when stream is in EOF state. Function has no effect}} + // expected-note@-3 {{Read function called when stream is in EOF state. Function has no effect}} + fclose(F); +}