[Analyzer][StreamChecker] Report every leak, clean up state.

Summary:
Report resource leaks with non-fatal error.
Report every resource leak.
Stream state is cleaned up at `checkDeadSymbols`.

Reviewers: Szelethus, baloghadamsoftware, NoQ

Reviewed By: Szelethus

Subscribers: 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/D82845

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

@@ -337,6 +337,12 @@ private:
   /// to ensure uniform handling.
   void reportFEofWarning(CheckerContext &C, ProgramStateRef State) const;
 
+  /// Emit resource leak warnings for the given symbols.
+  /// Createn a non-fatal error node for these, and returns it (if any warnings
+  /// were generated). Return value is non-null.
+  ExplodedNode *reportLeaks(const SmallVector<SymbolRef, 2> &LeakedSyms,
+                            CheckerContext &C, ExplodedNode *Pred) const;
+
   /// Find the description data of the function called by a call event.
   /// Returns nullptr if no function is recognized.
   const FnDescription *lookupFn(const CallEvent &Call) const {
@@ -956,28 +962,19 @@ void StreamChecker::reportFEofWarning(CheckerContext &C,
   C.addTransition(State);
 }
 
-void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
-                                     CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
+ExplodedNode *
+StreamChecker::reportLeaks(const SmallVector<SymbolRef, 2> &LeakedSyms,
+                           CheckerContext &C, ExplodedNode *Pred) const {
+  // Do not warn for non-closed stream at program exit.
+  // FIXME: Use BugType::SuppressOnSink instead.
+  if (Pred && Pred->getCFGBlock() && Pred->getCFGBlock()->hasNoReturnElement())
+    return Pred;
 
-  // TODO: Clean up the state.
-  const StreamMapTy &Map = State->get<StreamMap>();
-  for (const auto &I : Map) {
-    SymbolRef Sym = I.first;
-    const StreamState &SS = I.second;
-    if (!SymReaper.isDead(Sym) || !SS.isOpened())
-      continue;
-
-    ExplodedNode *N = C.generateErrorNode();
-    if (!N)
-      continue;
-
-    // Do not warn for non-closed stream at program exit.
-    ExplodedNode *Pred = C.getPredecessor();
-    if (Pred && Pred->getCFGBlock() &&
-        Pred->getCFGBlock()->hasNoReturnElement())
-      continue;
+  ExplodedNode *Err = C.generateNonFatalErrorNode(C.getState(), Pred);
+  if (!Err)
+    return Pred;
 
+  for (SymbolRef LeakSym : LeakedSyms) {
     // Resource leaks can result in multiple warning that describe the same kind
     // of programming error:
     //  void f() {
@@ -989,8 +986,7 @@ void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
     // 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);
+    const ExplodedNode *StreamOpenNode = getAcquisitionSite(Err, LeakSym, C);
     assert(StreamOpenNode && "Could not find place of stream opening.");
     PathDiagnosticLocation LocUsedForUniqueing =
         PathDiagnosticLocation::createBegin(
@@ -1000,12 +996,38 @@ void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
     std::unique_ptr<PathSensitiveBugReport> R =
         std::make_unique<PathSensitiveBugReport>(
             BT_ResourceLeak,
-            "Opened stream never closed. Potential resource leak.", N,
+            "Opened stream never closed. Potential resource leak.", Err,
             LocUsedForUniqueing,
             StreamOpenNode->getLocationContext()->getDecl());
-    R->markInteresting(Sym);
+    R->markInteresting(LeakSym);
     C.emitReport(std::move(R));
   }
+
+  return Err;
+}
+
+void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
+                                     CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+
+  llvm::SmallVector<SymbolRef, 2> LeakedSyms;
+
+  const StreamMapTy &Map = State->get<StreamMap>();
+  for (const auto &I : Map) {
+    SymbolRef Sym = I.first;
+    const StreamState &SS = I.second;
+    if (!SymReaper.isDead(Sym))
+      continue;
+    if (SS.isOpened())
+      LeakedSyms.push_back(Sym);
+    State = State->remove<StreamMap>(Sym);
+  }
+
+  ExplodedNode *N = C.getPredecessor();
+  if (!LeakedSyms.empty())
+    N = reportLeaks(LeakedSyms, C, N);
+
+  C.addTransition(State, N);
 }
 
 ProgramStateRef StreamChecker::checkPointerEscape(

clang/test/Analysis/stream-note.c

@@ -46,3 +46,34 @@ void check_note_freopen() {
 }
 // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
 // expected-note@-2 {{Opened stream never closed. Potential resource leak}}
+
+void check_note_leak_2(int c) {
+  FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}}
+  if (!F1)
+    // expected-note@-1 {{'F1' is non-null}}
+    // expected-note@-2 {{Taking false branch}}
+    // expected-note@-3 {{'F1' is non-null}}
+    // expected-note@-4 {{Taking false branch}}
+    return;
+  FILE *F2 = fopen("foo2.c", "r"); // expected-note {{Stream opened here}}
+  if (!F2) {
+    // expected-note@-1 {{'F2' is non-null}}
+    // expected-note@-2 {{Taking false branch}}
+    // expected-note@-3 {{'F2' is non-null}}
+    // expected-note@-4 {{Taking false branch}}
+    fclose(F1);
+    return;
+  }
+  if (c)
+    // expected-note@-1 {{Assuming 'c' is not equal to 0}}
+    // expected-note@-2 {{Taking true branch}}
+    // expected-note@-3 {{Assuming 'c' is not equal to 0}}
+    // expected-note@-4 {{Taking true branch}}
+    return;
+  // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
+  // expected-note@-2 {{Opened stream never closed. Potential resource leak}}
+  // expected-warning@-3 {{Opened stream never closed. Potential resource leak}}
+  // expected-note@-4 {{Opened stream never closed. Potential resource leak}}
+  fclose(F1);
+  fclose(F2);
+}