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,14 @@ /// If true, evaluate special testing stream functions. bool TestMode = false; - const BugType *getBT_StreamEof() const { return &BT_StreamEof; } + using BugMessageMap = llvm::DenseMap; + 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"}, + {&BT_StreamEof, "Assuming stream reaches end-of-file here"}, + {&BT_ResourceLeak, "Stream opened here"}}; private: CallDescriptionMap FnDescriptions = { @@ -376,37 +384,24 @@ 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; - - return ""; - } - }; - - const NoteTag *constructNoteTag(CheckerContext &C, SymbolRef StreamSym, - const std::string &Message) const { - return C.getNoteTag(NoteFn{&BT_ResourceLeak, StreamSym, 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()) + /// Create a NoteTag to display a note if a later bug report is generated. + /// The `BT` should contain all bug types that could be caused by the + /// operation at this location. + /// If later on the path a problematic event (reported bug) happens with the + /// same type, the last place is found with a note tag with that bug type. + const NoteTag * + constructNoteTag(CheckerContext &C, SymbolRef StreamSym, + llvm::SmallPtrSet &&BT) const { + + return C.getNoteTag([this, StreamSym, BT](PathSensitiveBugReport &BR) { + if (!BR.isInteresting(StreamSym) || !BT.contains(&BR.getBugType())) return ""; + // This is done to make the report only at the last location with the same + // note tag. BR.markNotInteresting(StreamSym); - return "Assuming stream reaches end-of-file here"; + return this->BugMessages.lookup(&BR.getBugType()); }); } @@ -500,8 +495,8 @@ StateNull->set(RetSym, StreamState::getOpenFailed(Desc)); C.addTransition(StateNotNull, - constructNoteTag(C, RetSym, "Stream opened here")); - C.addTransition(StateNull); + constructNoteTag(C, RetSym, {&BT_ResourceLeak})); + C.addTransition(StateNull, constructNoteTag(C, RetSym, {&BT_FileNull})); } void StreamChecker::preFreopen(const FnDescription *Desc, const CallEvent &Call, @@ -557,8 +552,10 @@ StateRetNull->set(StreamSym, StreamState::getOpenFailed(Desc)); C.addTransition(StateRetNotNull, - constructNoteTag(C, StreamSym, "Stream reopened here")); - C.addTransition(StateRetNull); + constructNoteTag(C, StreamSym, {&BT_ResourceLeak})); + C.addTransition( + StateRetNull, + constructNoteTag(C, StreamSym, {&BT_FileNull, &BT_UseAfterOpenFailed})); } void StreamChecker::evalFclose(const FnDescription *Desc, const CallEvent &Call, @@ -579,7 +576,7 @@ // and can not be used any more. State = State->set(Sym, StreamState::getClosed(Desc)); - C.addTransition(State); + C.addTransition(State, constructNoteTag(C, Sym, {&BT_UseAfterClose})); } void StreamChecker::preFread(const FnDescription *Desc, const CallEvent &Call, @@ -705,9 +702,12 @@ 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, constructNoteTag(C, StreamSym, + {&BT_StreamEof, + &BT_IndeterminatePosition})); else - C.addTransition(StateFailed); + C.addTransition(StateFailed, constructNoteTag(C, StreamSym, + {&BT_IndeterminatePosition})); } void StreamChecker::preFseek(const FnDescription *Desc, const CallEvent &Call, @@ -766,7 +766,9 @@ StreamState::getOpened(Desc, ErrorNone | ErrorFEof | ErrorFError, true)); C.addTransition(StateNotFailed); - C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym)); + C.addTransition(StateFailed, + constructNoteTag(C, StreamSym, + {&BT_StreamEof, &BT_IndeterminatePosition})); } void StreamChecker::evalClearerr(const FnDescription *Desc, @@ -901,9 +903,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 +921,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 +963,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 +974,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,30 @@ // 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() { + FILE *F = fopen("file", "r"); + if (!F) // expected-note {{'F' is non-null}} expected-note {{Taking false branch}} + return; + 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 +104,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; @@ -145,3 +169,62 @@ } 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}} + if (ferror(F)) { // 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}} + if (!feof(F)) // 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}} + if (ferror(F)) // 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}} + if (!ferror(F) && !feof(F)) // expected-note {{Taking true branch}} // expected-note {{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); +}