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 @@ -216,8 +216,8 @@ "Read function called when stream is in EOF state. " "Function has no effect."}; BuiltinBug BT_ResourceLeak{ - this, "Resource Leak", - "Opened File never closed. Potential Resource leak."}; + this, "Resource leak", + "Opened stream never closed. Potential resource leak."}; public: void checkPreCall(const CallEvent &Call, CheckerContext &C) const; @@ -365,6 +365,33 @@ return FnDescriptions.lookup(Call); } + + /// Generate a message for BugReporterVisitor if the stored symbol is + /// marked as interesting by the actual bug report. + struct NoteFn { + const CheckerNameRef CheckerName; + SymbolRef StreamSym; + std::string Message; + + std::string operator()(PathSensitiveBugReport &BR) const { + if (BR.isInteresting(StreamSym) && + CheckerName == BR.getBugType().getCheckerName()) + return Message; + + return ""; + } + }; + + const NoteTag *constructNoteTag(CheckerContext &C, SymbolRef StreamSym, + const std::string &Message) const { + return C.getNoteTag(NoteFn{getCheckerName(), StreamSym, Message}); + } + + /// Searches for the ExplodedNode where the file descriptor was acquired for + /// StreamSym. + static const ExplodedNode *getAcquisitionSite(const ExplodedNode *N, + SymbolRef StreamSym, + CheckerContext &C); }; } // end anonymous namespace @@ -376,6 +403,27 @@ "Previous create of error node for non-opened stream failed?"); } +const ExplodedNode *StreamChecker::getAcquisitionSite(const ExplodedNode *N, + SymbolRef StreamSym, + CheckerContext &C) { + ProgramStateRef State = N->getState(); + // When bug type is resource leak, exploded node N may not have state info + // for leaked file descriptor, but predecessor should have it. + if (!State->get(StreamSym)) + N = N->getFirstPred(); + + const ExplodedNode *Pred = N; + while (N) { + State = N->getState(); + if (!State->get(StreamSym)) + return Pred; + Pred = N; + N = N->getFirstPred(); + } + + return nullptr; +} + void StreamChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { const FnDescription *Desc = lookupFn(Call); @@ -421,7 +469,8 @@ StateNull = StateNull->set(RetSym, StreamState::getOpenFailed(Desc)); - C.addTransition(StateNotNull); + C.addTransition(StateNotNull, + constructNoteTag(C, RetSym, "Stream opened here")); C.addTransition(StateNull); } @@ -476,7 +525,8 @@ StateRetNull = StateRetNull->set(StreamSym, StreamState::getOpenFailed(Desc)); - C.addTransition(StateRetNotNull); + C.addTransition(StateRetNotNull, + constructNoteTag(C, StreamSym, "Stream reopened here")); C.addTransition(StateRetNull); } @@ -921,8 +971,38 @@ if (!N) continue; - C.emitReport(std::make_unique( - BT_ResourceLeak, BT_ResourceLeak.getDescription(), N)); + // Do not warn for non-closed stream at program exit. + ExplodedNode *Pred = C.getPredecessor(); + if (Pred && Pred->getCFGBlock() && + Pred->getCFGBlock()->hasNoReturnElement()) + continue; + + // Resource leaks can result in multiple warning that describe the same kind + // of programming error: + // void f() { + // FILE *F = fopen("a.txt"); + // if (rand()) // state split + // return; // warning + // } // warning + // While this isn't necessarily true (leaking the same stream could result + // from a different kinds of errors), the reduction in redundant reports + // makes this a worthwhile heuristic. + // FIXME: Add a checker option to turn this uniqueing feature off. + + const ExplodedNode *StreamOpenNode = getAcquisitionSite(N, Sym, C); + assert(StreamOpenNode && "Could not find place of stream opening."); + PathDiagnosticLocation LocUsedForUniqueing = + PathDiagnosticLocation::createBegin( + StreamOpenNode->getStmtForDiagnostics(), C.getSourceManager(), + StreamOpenNode->getLocationContext()); + + std::unique_ptr R = + std::make_unique( + BT_ResourceLeak, BT_ResourceLeak.getDescription(), N, + LocUsedForUniqueing, + StreamOpenNode->getLocationContext()->getDecl()); + R->markInteresting(Sym); + C.emitReport(std::move(R)); } } diff --git a/clang/test/Analysis/stream-note.c b/clang/test/Analysis/stream-note.c new file mode 100644 --- /dev/null +++ b/clang/test/Analysis/stream-note.c @@ -0,0 +1,48 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream -analyzer-output text -verify %s + +#include "Inputs/system-header-simulator.h" + +void check_note_at_correct_open() { + FILE *F1 = tmpfile(); // expected-note {{Stream opened here}} + if (!F1) + // expected-note@-1 {{'F1' is non-null}} + // expected-note@-2 {{Taking false branch}} + return; + FILE *F2 = tmpfile(); + if (!F2) { + // expected-note@-1 {{'F2' is non-null}} + // expected-note@-2 {{Taking false branch}} + fclose(F1); + return; + } + rewind(F2); + fclose(F2); + rewind(F1); +} +// expected-warning@-1 {{Opened stream never closed. Potential resource leak}} +// expected-note@-2 {{Opened stream never closed. Potential resource leak}} + +void check_note_fopen() { + FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}} + if (!F) + // expected-note@-1 {{'F' is non-null}} + // expected-note@-2 {{Taking false branch}} + return; +} +// expected-warning@-1 {{Opened stream never closed. Potential resource leak}} +// expected-note@-2 {{Opened stream never closed. Potential resource leak}} + +void check_note_freopen() { + FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}} + 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}} + if (!F) + // expected-note@-1 {{'F' is non-null}} + // expected-note@-2 {{Taking false branch}} + return; +} +// expected-warning@-1 {{Opened stream never closed. Potential resource leak}} +// expected-note@-2 {{Opened stream never closed. Potential resource leak}} diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c --- a/clang/test/Analysis/stream.c +++ b/clang/test/Analysis/stream.c @@ -1,4 +1,4 @@ -// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.Stream -analyzer-store region -verify %s +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream -verify %s #include "Inputs/system-header-simulator.h" @@ -139,7 +139,7 @@ if (!p) return; if(c) - return; // expected-warning {{Opened File never closed. Potential Resource leak}} + return; // expected-warning {{Opened stream never closed. Potential resource leak}} fclose(p); } @@ -240,3 +240,28 @@ fwrite("1", 1, 1, F); // expected-warning {{might be 'indeterminate'}} fclose(F); } + +int Test; +_Noreturn void handle_error(); + +void check_leak_noreturn_1() { + FILE *F1 = tmpfile(); + if (!F1) + return; + if (Test == 1) { + handle_error(); // no warning + } + rewind(F1); +} // expected-warning {{Opened stream never closed. Potential resource leak}} + +// Check that "location uniqueing" works. +// This results in reporting only one occurence of resource leak for a stream. +void check_leak_noreturn_2() { + FILE *F1 = tmpfile(); + if (!F1) + return; + if (Test == 1) { + return; // expected-warning {{Opened stream never closed. Potential resource leak}} + } + rewind(F1); +} // no warning