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

[balazske]

Summary

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

[balazske]

Why does this not work?
I get only warning for the first resource leak (in the test f_leak_2). How to fix this?

[Szelethus]

In D82845#2121940, @balazske wrote:

Why does this not work?
I get only warning for the first resource leak (in the test f_leak_2). How to fix this?

First off, are two errors detected by the checker? You could check this with debug.ViewExplodedGraph + clang/tools/analyzer/exploded-graph-rewriter.py.

I do suspect though that this is a uniqueing issue. Just out of curiosity, what would happen is you dump the leaked stream symbol into the warning message? (or whatever, just so that two distinct warning messages are emitted)

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

969

It is a realistic worry that either the predecessor exploded node or the CFGBlock is null? Could we assert this instead?

971

Ah, I see, we made it fatal accidentally.

[balazske]

I checked it with simple debug print, the LeakedSyms will contain 2 items with different StreamOpenNode. So I think these should be independent problem reports for the bug reporter.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

969

It may be simply more safe to check for it. I do not know if exactly if these can be null at this place.

[Szelethus]

In D82845#2122984, @balazske wrote:

I checked it with simple debug print, the LeakedSyms will contain 2 items with different StreamOpenNode. So I think these should be independent problem reports for the bug reporter.

It would still be beneficial to check the exploded graph to see whether it was constructed correctly, find the error nodes, etc. Have you tried modifying the warning message? That is considered during uniquing.

[baloghadamsoftware]

In D82845#2122984, @balazske wrote:

I checked it with simple debug print, the LeakedSyms will contain 2 items with different StreamOpenNode. So I think these should be independent problem reports for the bug reporter.

I wonder whether you can report two bugs on the same error node.

[Szelethus]

In D82845#2123283, @baloghadamsoftware wrote:

In D82845#2122984, @balazske wrote:

I checked it with simple debug print, the LeakedSyms will contain 2 items with different StreamOpenNode. So I think these should be independent problem reports for the bug reporter.

I wonder whether you can report two bugs on the same error node.

I recall that i did something similar in UninitializedObjectChecker when the option NotesAsWarnings is true.

[balazske]

The problem is that PathDiagnostic::Profile does not use the uniqueing locations. Two PathDiagnostic objects with different uniqueing location but otherwise same data are counted as "same" and only one is kept (BugReporter::FlushReport). How to fix this?

• Add uniqueing locations to the profile. May have unexpected effects to other checkers.
• Change something in the leak bug reports to make these different. Probably the location can be set to the allocation site (but the full path should be reported). Or include name of the leaked symbol in the report, but it may not exists always.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

969

Please use SuppressOnSink instead.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

969

That was my plan for the next patch. So I want to leave this as is for now (add a FIXME), it should be removed anyway.

[balazske]

Rebase, added a FIXME

[Szelethus]

I'd prefer if you moved f_leak_2 to stream-notes.c. Otherwise, LGTM.

clang/test/Analysis/stream.c

147–150 ↗

(On Diff #275311)

I'd prefer to see notes from D81407 here.

[balazske]

Checking notes in test f_leak_2.

[balazske]

Fixed formatting.

[balazske]

Rebase to master.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

ProgramStateRef ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C,
                                            ProgramStateRef State) const;
 
   /// Generate warning about stream in EOF state.
   /// There will be always a state transition into the passed State,
   /// by the new non-fatal error node or (if failed) a normal transition,
   /// 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 {
     // Recognize "global C functions" with only integral or pointer arguments
     // (and matching name) as stream functions.
     if (!Call.isGlobalCFunction())
       return nullptr;
     for (auto P : Call.parameters()) {
C.emitReport(std::make_unique<PathSensitiveBugReport>(
         "Read function called when stream is in EOF state. "
         "Function has no effect.",
         N));
     return;
   }
   C.addTransition(State);
 }
 
-void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
-                                     CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-
-  // 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 *
+StreamChecker::reportLeaks(const SmallVector<SymbolRef, 2> &LeakedSyms,
+                           CheckerContext &C, ExplodedNode *Pred) const {
+  // FIXME: Use BugType::SuppressOnSink instead.
+  if (Pred && Pred->getCFGBlock() && Pred->getCFGBlock()->hasNoReturnElement())
+    return Pred;
+
+  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() {
     //    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);
+    const ExplodedNode *StreamOpenNode = getAcquisitionSite(Err, LeakSym, 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,
-            "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(
     ProgramStateRef State, const InvalidatedSymbols &Escaped,
     const CallEvent *Call, PointerEscapeKind Kind) const {
   // Check for file-handling system call that is not handled by the checker.
   // FIXME: The checker should be updated to handle all system calls that take
   // 'FILE*' argument. These are now ignored.

clang/test/Analysis/stream-note.c

void check_note_freopen() {
   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}}
+
+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);
+}