[clang] e935a54 - [Analyzer][StreamChecker] Add note tags for file opening.
Balázs Kéri via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 22 02:13:57 PDT 2020
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 at -1 {{'F1' is non-null}}
+ // expected-note at -2 {{Taking false branch}}
+ return;
+ FILE *F2 = tmpfile();
+ if (!F2) {
+ // expected-note at -1 {{'F2' is non-null}}
+ // expected-note at -2 {{Taking false branch}}
+ fclose(F1);
+ return;
+ }
+ rewind(F2);
+ fclose(F2);
+ rewind(F1);
+}
+// expected-warning at -1 {{Opened stream never closed. Potential resource leak}}
+// expected-note at -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 at -1 {{'F' is non-null}}
+ // expected-note at -2 {{Taking false branch}}
+ return;
+}
+// expected-warning at -1 {{Opened stream never closed. Potential resource leak}}
+// expected-note at -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 at -1 {{'F' is non-null}}
+ // expected-note at -2 {{Taking false branch}}
+ return;
+ F = freopen(0, "w", F); // expected-note {{Stream reopened here}}
+ if (!F)
+ // expected-note at -1 {{'F' is non-null}}
+ // expected-note at -2 {{Taking false branch}}
+ return;
+}
+// expected-warning at -1 {{Opened stream never closed. Potential resource leak}}
+// expected-note at -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}}
More information about the cfe-commits
mailing list