[Analyzer][StreamChecker] Add note tags for file opening.

[balazske]

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.

[Szelethus]

If there are multiple resource leak paths for the same stream, only one wil be reported.

What is the rationale behind this? Do all leaks from the same opening describe the same kind of error? Is this based on observations on a codebase? I'm not against it -- but nor am I immediately for it. Otherwise LGTM.

[balazske]

In D81407#2080273, @Szelethus wrote:

If there are multiple resource leak paths for the same stream, only one wil be reported.

What is the rationale behind this? Do all leaks from the same opening describe the same kind of error? Is this based on observations on a codebase?

The code was taken from FuchsiaHandleChecker. I do not know which approach to use, it may be that reporting all leak paths is better, these are really different problems. But I did not like getting many similar looking bug reports at a function that opens a file and then in various error cases stops the program by a "noreturn" function. At every such case a false positive resource leak is reported (I think it is OK to not close the file at stopping the program specially if there is some kind of error). Or the checker can be improved to find at a checkDeadSymbols if the program execution is stopping and do not report resource leak in that case.

[Szelethus]

Alright, I'm sold. How about we add a checker option for it? I don't actually insist, just an idea. @xazax.hun, how has this feature played out?

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

394

How about getAcquisitionSite, and a line of comment:

Searches for the ExplodedNode where the file descriptor was acquired for Sym.

398–399

I see what you mean, but I'd phrase this differently, and place it...

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.

961

...here!

clang/test/Analysis/stream-note.c

1

core package! Also, we don't specify -analyzer-store region explicitly, we even wondered whether we should just remove the option altogether. Fun fact, clang_analyze_cc1 actually expands to an invocation that contains it anyways.

[balazske]

• Report every path of resource leak.
• Do not report if non-returning function was encountered.

[balazske]

• Added tests.

[Szelethus]

In D81407#2087624, @balazske wrote:

• Report every path of resource leak.

I thought we agreed on the uniqueing being great?

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

376–377

Another thing you might want to check is that the warning is coming from your checker. The symbol may be marked as interesting by another checker for a completely unrelated reason. The easiest way to check that is usually to compare the report's bug type to your checker's bug type.

(we should absolutely automate this)

[NoQ]

Do i understand correctly that the checker is no longer "missing limbs" and we should consider turning it on by default? If so, @balazske could you prioritize hunting down the remaining false positives above adding new checks / hunting down false negatives, so that users could finally start taking advantage of the checker?

[Szelethus]

I'd still like to see more NoteTags such as "File read failed, end-of-file indicator set on 'F'", and a final evaluation would be nice, but otherwise this checker looks amazing.

[balazske]

• Rebase
• Added check for checker in NoteTag function.

[balazske]

• Re-added the location uniqueing feature.

[balazske]

Corrected command line arguments in tests.

[Szelethus]

Yay! Getting so close to enabling this by default. I'm a big fan of your work on this checker.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

378

I think is is going to be good enough until we automate things.

clang/test/Analysis/stream.c

1

Nice catch.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

394

Ok, so this is a tiny auxiliary visitor.

I wish we could set uniqueing location post-factum from within the note tag. Unfortunately we can't because notes are generated after uniqueing :(

I'd like you to experiment with tracking this information as part of the state instead so that you didn't need to perform an additional scan. I'd be happy if it helps you avoid performing this additional pass.

I.e., when you're opening the file, add all the information you need for building your uniqueing location into the stream state (not the node though, just the program point or something like that). Then retrieve it in O(1) when you're emitting the report.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

394

Another thing we could try is to implement such post-processing pass globally for all checkers. I.e., instead of an optional uniqueing location accept an optional lambda that looks at a node and answers whether this node should be used as a uniqueing location. Then before report deduplication scan each report that supplies such lambda and update its uniqueing location. That'll probably require some work on BugReporter but that sounds like a nice way to avoid all the boilerplate.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

394

Storing the "acquisition site" in the state is the natural way of doing this. Probably I should not think that existing checkers do things the best way, this function is taken from FuchsiaHandleChecker (or here it has a specific reason?). And not storing the data in the state is a bit less memory consumption if this matters.

394

We should check how many checkers can benefit from such a "uniqueing location callback". Normally the checker should know what the uniqueing location is, at least when the bug report is created. The location is naturally obtained from the state that the checker maintains, at least if we want to avoid scans like getAcquisitionSite.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

394

Probably I should not think that existing checkers do things the best way

Well, if you ever find yourself copy-pasting a large chunk of code from a different checker and using it almost unchanged, it's a good indication that the checker API needs to be improved. You can't do things "the best way" in a single checker in isolation, it's a collective effort. You're a programmer, you can change everything, no need to be confined in your checker.

We should check how many checkers can benefit from such a "uniqueing location callback". Normally the checker should know what the uniqueing location is, at least when the bug report is created.

No, i don't think it ever happens automagically. It's either tracked in the state specifically for that purpose or scanned backwards. So i'd rather believe that every checker that has non-default uniqueing locations will benefit from such facility.

[Szelethus]

Balázs, could you please add the checker option within this patch? If we find that the option works well (removes a lot of useless reports) I'd be happy to help implement that uniqueing pass.

CmdLineOption<Boolean,
              "UniqueLeaks",
              "Only display a single report for each leaked stream object, rather than a report for each path of execution on which the same stream was leaked",
              "false",
              InAlpha>,

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

394

Another thing we could try is to implement such post-processing pass globally for all checkers. I.e., instead of an optional uniqueing location accept an optional lambda that looks at a node and answers whether this node should be used as a uniqueing location. Then before report deduplication scan each report that supplies such lambda and update its uniqueing location. That'll probably require some work on BugReporter but that sounds like a nice way to avoid all the boilerplate.

I've lately found the uniqueing location quite wonky, because its hardly ever the location we want to unique by, but it is the only way to pull it off with the existing infrastructure. More often then not the uniqueing point is a variable, a stream object, or something that isn't a location, but can sort of be tied to a location. So, lets go with this suggestion!

@balazske As long as this is an off-by-default hidden checker option, I think we can commit this code for experimentation purposes. I believe it is correct for 99% of the cases. But I agree, we need to tie its enabling to a more robust solution.

Lets add some comments:
// HACK: This is essentially a tiny bugreporter visitor, but before the trimming of the exploded graph (so it may contain directed cycles still). We use it to acquire a uniqueing location, but it would be better if we could unique by the actual ExplodedNode instead. We should probably implement a pass before bug report generation that takes a lambda that looks at a node and answers whether this node should be used as a uniqueing node in favor of the currect location-based technique. Note that FuchsiaHandleChecker does something very similar as well.

[NoQ]

Balázs, could you please add the checker option within this patch?

I'd rather have this decision made globally. Like, for all leaks, or something like that. Our behavior should be consistent.

[balazske]

I do not understand fully this "globally". A new option should be added that affects all checkers that detect some kind of resource leak? And then implement that kind of report uniqueness in all checkers that detect resource leak.

Other possible solution: Leave the current way of checker specific options, and add a kind of "meta-option" that can set multiple (checker) options in a batch. For our case this would set the report uniqueing option for every checker that supports it. Still it remains possible to set options separately for each checker.

[Szelethus]

I see where you're coming from @NoQ. What do you think, @balazske? I think there is is still value in this implementation as a debug option to gather data, so that we don't invest a lot of time creating a robust infrastructure for an idea that might not work out.

In D81407#2102641, @balazske wrote:

I do not understand fully this "globally". A new option should be added that affects all checkers that detect some kind of resource leak?

Yup, its a fair point that all leaks describe the same kind of bug, even if the root cause of that bug may come from different kinds of programming errors, so it makes sense to unique them all the same way.

And then implement that kind of report uniqueness in all checkers that detect resource leak.

That could be helped additionally by creating a distinct LeakBugReport, derived from PathSensitiveBugReport, that would take non-optional uniqueing lambda to find the ExplodedNode responsible for the resource acquisition. Or the actual ExplodedNode itself.

Other possible solution: Leave the current way of checker specific options, and add a kind of "meta-option" that can set multiple (checker) options in a batch. For our case this would set the report uniqueing option for every checker that supports it. Still it remains possible to set options separately for each checker.

That could be achieved with Artem's proposed package system (or hashtags): D77866#2069144 (Package options are a thing even today). However, if we had a LeakBugReport class, we could implement the option with regular analyzer configs.

[NoQ]

In D81407#2102951, @Szelethus wrote:

That could be helped additionally by creating a distinct LeakBugReport, derived from PathSensitiveBugReport, that would take non-optional uniqueing lambda to find the ExplodedNode responsible for the resource acquisition. Or the actual ExplodedNode itself.

That's an awesome idea, i'm speechless :)

In D81407#2102641, @balazske wrote:

I do not understand fully this "globally". A new option should be added that affects all checkers that detect some kind of resource leak? And then implement that kind of report uniqueness in all checkers that detect resource leak.

Yes, that's probably the best approach. If you want to experiment a lot with this stuff, you probably want data from more different checkers than just yours (i expect your checker to be relatively quiet compared to, say, MallocChecker that'll provide a lot more input to your experiment). I'd only go for an ability to configure checkers individually if we have any signal at all that they *need* to be configured individually; otherwise enforcing consistent user experience is a good thing.

[balazske]

So for this patch it would be OK to have the uniqueing location as it is now. A next large change can be to add the global resource leak report uniqueing feature, this changes anyway more existing checkers (including this one). (Still I want to finish other improvements in the StreamChecker.)

[NoQ]

Yup, i think so!

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

BuiltinBug BT_IndeterminatePosition{
       "Can cause undefined behavior."};
   BuiltinBug BT_IllegalWhence{this, "Illegal whence argument",
                               "The whence argument to fseek() should be "
                               "SEEK_SET, SEEK_END, or SEEK_CUR."};
   BuiltinBug BT_StreamEof{this, "Stream already in EOF",
                           "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;
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
   ProgramStateRef checkPointerEscape(ProgramStateRef State,
                                      const InvalidatedSymbols &Escaped,
                                      const CallEvent *Call,
const FnDescription *lookupFn(const CallEvent &Call) const {
     for (auto P : Call.parameters()) {
       QualType T = P->getType();
       if (!T->isIntegralOrEnumerationType() && !T->isPointerType())
         return nullptr;
     }
 
     return FnDescriptions.lookup(Call);
   }
+
+  static const ExplodedNode *getAcquireSite(const ExplodedNode *N,
+                                            SymbolRef Sym, CheckerContext &Ctx);
+};
+
+struct NoteFn {
+  SymbolRef StreamSym;
+  std::string Message;
+
+  std::string operator()(PathSensitiveBugReport &BR) const {
+    if (BR.isInteresting(StreamSym))
+      return Message;
+
+    return "";
+  }
 };
 
 } // end anonymous namespace
 
 REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
 
 inline void assertStreamStateOpened(const StreamState *SS) {
   assert(SS->isOpened() &&
          "Previous create of error node for non-opened stream failed?");
 }
 
+const ExplodedNode *StreamChecker::getAcquireSite(const ExplodedNode *N,
+                                                  SymbolRef Sym,
+                                                  CheckerContext &Ctx) {
+  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>(Sym))
+    N = N->getFirstPred();
+
+  const ExplodedNode *Pred = N;
+  while (N) {
+    State = N->getState();
+    if (!State->get<StreamMap>(Sym))
+      return Pred;
+    Pred = N;
+    N = N->getFirstPred();
+  }
+  return nullptr;
+}
+
 void StreamChecker::checkPreCall(const CallEvent &Call,
                                  CheckerContext &C) const {
   const FnDescription *Desc = lookupFn(Call);
   if (!Desc || !Desc->PreFn)
     return;
 
   Desc->PreFn(this, Desc, Call, C);
 }
void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
   std::tie(StateNotNull, StateNull) =
       C.getConstraintManager().assumeDual(State, RetVal);
 
   StateNotNull =
       StateNotNull->set<StreamMap>(RetSym, StreamState::getOpened(Desc));
   StateNull =
       StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc));
 
-  C.addTransition(StateNotNull);
+  const NoteTag *T = C.getNoteTag(NoteFn{RetSym, "Stream opened here"});
+  C.addTransition(StateNotNull, T);
   C.addTransition(StateNull);
 }
 
 void StreamChecker::preFreopen(const FnDescription *Desc, const CallEvent &Call,
                                CheckerContext &C) const {
   // Do not allow NULL as passed stream pointer but allow a closed stream.
   ProgramStateRef State = C.getState();
   State = ensureStreamNonNull(getStreamArg(Desc, Call), C, State);
void StreamChecker::evalFreopen(const FnDescription *Desc,
   ProgramStateRef StateRetNull = State->BindExpr(CE, C.getLocationContext(),
                                                  C.getSValBuilder().makeNull());
 
   StateRetNotNull =
       StateRetNotNull->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
   StateRetNull =
       StateRetNull->set<StreamMap>(StreamSym, StreamState::getOpenFailed(Desc));
 
-  C.addTransition(StateRetNotNull);
+  const NoteTag *T = C.getNoteTag(NoteFn{StreamSym, "Stream reopened here"});
+  C.addTransition(StateRetNotNull, T);
   C.addTransition(StateRetNull);
 }
 
 void StreamChecker::evalFclose(const FnDescription *Desc, const CallEvent &Call,
                                CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   SymbolRef Sym = getStreamArg(Desc, Call).getAsSymbol();
   if (!Sym)
for (const auto &I : Map) {
     const StreamState &SS = I.second;
     if (!SymReaper.isDead(Sym) || !SS.isOpened())
       continue;
 
     ExplodedNode *N = C.generateErrorNode();
     if (!N)
       continue;
 
-    C.emitReport(std::make_unique<PathSensitiveBugReport>(
-        BT_ResourceLeak, BT_ResourceLeak.getDescription(), N));
+    ;
+    const ExplodedNode *AcquireNode = getAcquireSite(N, Sym, C);
+    assert(AcquireNode && "Could not find place of stream opening.");
+    PathDiagnosticLocation LocUsedForUniqueing =
+        PathDiagnosticLocation::createBegin(
+            AcquireNode->getStmtForDiagnostics(), C.getSourceManager(),
+            AcquireNode->getLocationContext());
+
+    std::unique_ptr<PathSensitiveBugReport> R =
+        std::make_unique<PathSensitiveBugReport>(
+            BT_ResourceLeak, BT_ResourceLeak.getDescription(), N,
+            LocUsedForUniqueing, AcquireNode->getLocationContext()->getDecl());
+    R->markInteresting(Sym);
+    C.emitReport(std::move(R));
   }
 }
 
 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

clang/test/Analysis/stream-note.c

+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.Stream -analyzer-store region -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}}

clang/test/Analysis/stream.c

 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.Stream -analyzer-store region -verify %s
 
 #include "Inputs/system-header-simulator.h"
 
 void check_fread() {
   FILE *fp = tmpfile();
   fread(0, 0, 0, fp); // expected-warning {{Stream pointer might be NULL}}
   fclose(fp);
 }
void f_reopen_after_close(void) {
   fclose(p);
 }
 
 void f_leak(int c) {
   FILE *p = fopen("foo.c", "r");
   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);
 }
 
 FILE *f_null_checked(void) {
   FILE *p = fopen("foo.c", "r");
   if (p)
     return p; // no-warning
   else