Author: Balázs Kéri Date: 2020-06-22T11:15:35+02:00 New Revision: e935a540ea29d5de297595801aed1fb70fabfbf6
URL: https://github.com/llvm/llvm-project/commit/e935a540ea29d5de297595801aed1fb70fabfbf6 DIFF: https://github.com/llvm/llvm-project/commit/e935a540ea29d5de297595801aed1fb70fabfbf6.diff LOG: [Analyzer][StreamChecker] Add note tags for file opening. Summary: Bug reports of resource leak are now improved. If there are multiple resource leak paths for the same stream, only one wil be reported. Reviewers: Szelethus, xazax.hun, baloghadamsoftware, NoQ Reviewed By: Szelethus, NoQ Subscribers: NoQ, rnkovacs, xazax.hun, baloghadamsoftware, szepet, a.sidorin, mikhail.ramalho, Szelethus, donat.nagy, dkrupp, gamesh411, Charusso, martong, ASDenysPetrov, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D81407 Added: clang/test/Analysis/stream-note.c Modified: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp clang/test/Analysis/stream.c clang/test/Analysis/stream.cpp Removed: ################################################################################ diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index 7e10b2aa4356..8fe097826fad 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -216,8 +216,8 @@ class StreamChecker : public Checker<check::PreCall, eval::Call, "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 @@ class StreamChecker : public Checker<check::PreCall, eval::Call, 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 @@ inline void assertStreamStateOpened(const StreamState *SS) { "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<StreamMap>(StreamSym)) + N = N->getFirstPred(); + + const ExplodedNode *Pred = N; + while (N) { + State = N->getState(); + if (!State->get<StreamMap>(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 @@ void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call, StateNull = StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc)); - C.addTransition(StateNotNull); + C.addTransition(StateNotNull, + constructNoteTag(C, RetSym, "Stream opened here")); C.addTransition(StateNull); } @@ -476,7 +525,8 @@ void StreamChecker::evalFreopen(const FnDescription *Desc, StateRetNull = StateRetNull->set<StreamMap>(StreamSym, StreamState::getOpenFailed(Desc)); - C.addTransition(StateRetNotNull); + C.addTransition(StateRetNotNull, + constructNoteTag(C, StreamSym, "Stream reopened here")); C.addTransition(StateRetNull); } @@ -921,8 +971,38 @@ void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper, if (!N) continue; - C.emitReport(std::make_unique<PathSensitiveBugReport>( - 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 diff erent 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<PathSensitiveBugReport> R = + std::make_unique<PathSensitiveBugReport>( + 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 index 000000000000..08927e8902be --- /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 index dbbcc8715e0d..cd5d28ae8455 100644 --- 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 @@ void f_leak(int c) { 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 @@ void check_escape4() { 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 diff --git a/clang/test/Analysis/stream.cpp b/clang/test/Analysis/stream.cpp index c76b9d7498d0..7eca505bcaf5 100644 --- a/clang/test/Analysis/stream.cpp +++ b/clang/test/Analysis/stream.cpp @@ -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 typedef struct _IO_FILE FILE; extern FILE *fopen(const char *path, const char *mode); @@ -19,4 +19,4 @@ void f1() { void f2() { FILE *f = fopen("file", "r"); -} // expected-warning {{Opened File never closed. Potential Resource leak}} +} // expected-warning {{Opened stream never closed. Potential resource leak}} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
