[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

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 7e10b2a..8fe0978 100644 (file)
--- 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 @@ private:
 
     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 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<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 (file)
index 0000000..08927e8
--- /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 dbbcc87..cd5d28a 100644 (file)
--- 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 c76b9d7..7eca505 100644 (file)
--- 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}}