[analyzer] Add path notes to FuchsiaHandleCheck

[xazax.hun]

Summary

I also added a missing const to a BugReporter API.

There are some TODOs for potential future updates if we want richer notes during reporting.

[NoQ]

Sooooo... note tags?

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

234

Will we ever emit a report against an escaped symbol? If not, there will never be a report to attach the note to.

235–236

If the function is implemented as an eager state split, having a note is great and easy to implement.

If it's implemented as a Schrödinger state split, then i think we should still emit a note at the collapse point (especially given that it may happen in a deeper stack frame than the call that produced the symbol). But this makes me recognize the need for note tags in evalAssume.

513

I wonder how terrible would it be to allow bug visitors (or note tags, which are just an "API sugar" for bug visitors) to mutate the uniqueing location. Because such information is naturally available in the visitor.

It most likely won't work because the information is necessary before the visitors are run. But it would have been nice though, so i feel as if we're missing an opportunity here.

[xazax.hun]

I was thinking about note tags, but since we iterate on the argument/parameter pairs and might change the state in each iteration we would need to add a new transition after each change. I was wondering if using note tags would worth the cost (both in performance due to the additional number of nodes and in code complexity due to making transitions more often).

[xazax.hun]

Sorry, my previous comment might not be clear. The point is, the same call might both acquire and release handles (there are such calls in Fuchsia), so we might end up adding more than one note for a call for which we would need to add more than one transitions. If you think it is still cleaner that way, I could do that as well :)

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

234

I think there might be cases where we go from an escaped symbol to released and so on. But you are right, it would not contribute to understanding the actual bug report.

235–236

I agree.

513

On one hand, it would be nice :) On the other hand we could have more than one visitor and it would be a hell to debug when the visitors disagree an the uniqueing location. If we can come up with an API that is not that easy to misuse, I am in favor of a change like that.

[NoQ]

In D70725#1765981, @xazax.hun wrote:

the same call might both acquire and release handles (there are such calls in Fuchsia), so we might end up adding more than one note for a call for which we would need to add more than one transitions

Hmm, would this be too functional?:

std::vector<std::function> notes;
for (arg : args) {
  if (isAcquired(param)) {
    State = State->set(arg, Acquired);
    notes.push_back([](Report) {
      if (Report.isInteresting(arg))
        return "Handle acquired here";
    });
  }
  if (isReleased(param)) {
    State = State->set(arg, Released);
    notes.push_back([](Report) {
      if (Report.isInteresting(arg))
        return "Handle released here";
    });
  }
}

C.addTransition(State, C.getNoteTag(
    // We might as well add a C.getNoteTag() overload
    // to do this automatically.
    [std::move(notes)](Report) {
      for (note : notes)
        note();
    }));

Or, well, yeah, chain the nodes together; you anyway have to do this occasionally due to clumsiness of the rest of the API.

[NoQ]

note()

Of course i mean something like this:

std::string text = note();
if (!text.empty())
  return text;

(and return something like "" on the other return path in the inner lambdas).

[xazax.hun]

• Use the new notes API for path notes.

[xazax.hun]

You were right, the new API looks cleaner even in this case :)

[xazax.hun]

Just a small side note. If the state was same as the previous we do not end up creating a new ExplodedNode right? Is this also the case when we add a NoteTag?

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

358–374

Hmm, so we have C++14 now and I can do a move capture right? Will update in the next iteration.

[Charusso]

In D70725#1767579, @xazax.hun wrote:

Just a small side note. If the state was same as the previous we do not end up creating a new ExplodedNode right? Is this also the case when we add a NoteTag?

It only happens for evaluation when you cannot evaluate something. Other than that Pre/PostCall working fine to add a node with the NoteTag.

[NoQ]

Yay, it actually works!

I only have minor comments and ignorable hand-waving now.

In D70725#1767643, @Charusso wrote:

In D70725#1767579, @xazax.hun wrote:

Just a small side note. If the state was same as the previous we do not end up creating a new ExplodedNode right? Is this also the case when we add a NoteTag?

It only happens for evaluation when you cannot evaluate something. Other than that Pre/PostCall working fine to add a node with the NoteTag.

Tag pointers are part of the ProgramPoint's identity, which in turn are part of the ExplodedNode's identity. If you make a new note tag and transition into it for the first time, the new node is guaranteed to be fresh.

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

210

N = N->getFirstPred().

219

return N;? (eliminates a variable)

223

N = N->getFirstPred().

359–362

Ugh, it sucks that we have to do this by hand.

Why would the symbol be interesting if the bugtype is wrong? You imagine something like division by a zero handle? Do we really mind the updates to the handle highlighted in this case? I guess we do.

Maybe we should make "note tag tags" to avoid users writing this by hand. Like, note tags are tagged with note tag tags and the report is also tagged with a note tag tag and the tag is invoked only if the tag tag on the report matches the tag tag on the tag. Setting a tag tag on the report will be equivalent to calling an "addVisitor" on the report in the visitor API, which was actually a good part of the visitor API.

Just thinking out loud, please ignore. This code is a nice example to generalize from.

365

Another option here is to concatenate the notes if there are indeed two interesting handles at once: `Handle is allocated through 2nd parameter; Handle is released through 3rd parameter". In this checker it probably never happens, as every report is about exactly one handle (is it true for leak reports though?).

[xazax.hun]

• Address review comments.

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

359–362

I totally agree :) But if we do something like this please do not call the tag tag a tag. It will confuse people. :D

365

I think this is true for leaks as well, as we will generate a new report for each of the symbols.

[xazax.hun]

In D70725#1767673, @NoQ wrote:

In D70725#1767643, @Charusso wrote:

In D70725#1767579, @xazax.hun wrote:

Just a small side note. If the state was same as the previous we do not end up creating a new ExplodedNode right? Is this also the case when we add a NoteTag?

It only happens for evaluation when you cannot evaluate something. Other than that Pre/PostCall working fine to add a node with the NoteTag.

Tag pointers are part of the ProgramPoint's identity, which in turn are part of the ExplodedNode's identity. If you make a new note tag and transition into it for the first time, the new node is guaranteed to be fresh.

This is what I suspected. I wonder if all the checks end up using NoteTags will the increased number of nodes ever be a concern? Maybe if the state is the same as the previous we should just not add the note tag?

For example, my checker before the note tag would only generate new nodes if there was an actual change to the state. After this change it probably ends up adding new nodes with empty note tags all the time. This could be bad both for performance and debugging. What do you think?

[NoQ]

Mmm, right, i guess you should also skip adding the tag if the notes array is empty.

There might be more complicated use-cases (especially in ExprEngine itself) where you may end up adding a tag even if the state doesn't change. For instance, when changing an Environment value from absent to UnknownVal, the state doesn't get actually updated. Or making range assumptions over UnknownVal. But i'd argue that we do indeed want the note tag in such cases because it is still worth annotating the ExplodedGraph with an explanation of what's happening. Eg., a note "Assuming a condition is false" makes sense even if the condition is an UnknownVal and no actual constraints are added.

Another example: in a recent StreamChecker review we've been talking about a peculiar stream function that by design closes a file descriptor if it's open but does nothing if the descriptor is already closed. I believe this event deserves a note "The descriptor remains closed" (possibly prunable) even though there is no change in the state. This is something we couldn't do with our usual visitor-based approach which involves observing changes in the state (we technically could, via pattern-matching over the program point, but that'd directly duplicate a lot more of checker logic than usual).

[xazax.hun]

• Only add note tag if we have actual notes.

[xazax.hun]

In D70725#1767942, @NoQ wrote:

Mmm, right, i guess you should also skip adding the tag if the notes array is empty.

There might be more complicated use-cases (especially in ExprEngine itself) where you may end up adding a tag even if the state doesn't change. For instance, when changing an Environment value from absent to UnknownVal, the state doesn't get actually updated. Or making range assumptions over UnknownVal. But i'd argue that we do indeed want the note tag in such cases because it is still worth annotating the ExplodedGraph with an explanation of what's happening. Eg., a note "Assuming a condition is false" makes sense even if the condition is an UnknownVal and no actual constraints are added.

Another example: in a recent StreamChecker review we've been talking about a peculiar stream function that by design closes a file descriptor if it's open but does nothing if the descriptor is already closed. I believe this event deserves a note "The descriptor remains closed" (possibly prunable) even though there is no change in the state. This is something we couldn't do with our usual visitor-based approach which involves observing changes in the state (we technically could, via pattern-matching over the program point, but that'd directly duplicate a lot more of checker logic than usual).

I see, that makes sense. I updated the code. I was hoping for a low hanging fruit to make this more user friendly :)

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

307

I will capitalize this name.

[NoQ]

Looks great, thanks!~

[xazax.hun]

Thanks for the reviews!

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

public:
 
   /// Create a PathSensitiveBugReport with a custom uniqueing location.
   ///
   /// The reports that have the same report location, description, bug type, and
   /// ranges are uniqued - only one of the equivalent reports will be presented
   /// to the user. This method allows to rest the location which should be used
   /// for uniquing reports. For example, memory leaks checker, could set this to
   /// the allocation site, rather then the location where the bug is reported.
-  PathSensitiveBugReport(BugType &bt, StringRef desc,
+  PathSensitiveBugReport(const BugType &bt, StringRef desc,
                          const ExplodedNode *errorNode,
                          PathDiagnosticLocation LocationToUnique,
                          const Decl *DeclToUnique)
       : BugReport(Kind::PathSensitive, bt, desc), ErrorNode(errorNode),
         ErrorNodeRange(getStmt() ? getStmt()->getSourceRange() : SourceRange()),
         UniqueingLocation(LocationToUnique), UniqueingDecl(DeclToUnique) {
     assert(errorNode);
   }

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

public:
 
   void printState(raw_ostream &Out, ProgramStateRef State, const char *NL,
                   const char *Sep) const override;
 };
 } // end anonymous namespace
 
 REGISTER_MAP_WITH_PROGRAMSTATE(HStateMap, SymbolRef, HandleState)
 
+static const ExplodedNode *getAcquireSite(const ExplodedNode *N, SymbolRef Sym,
+                                          CheckerContext &Ctx) {
+  ProgramStateRef State = N->getState();
+  // When bug type is handle leak, exploded node N does not have state info for
+  // leaking handle. Get the predecessor of N instead.
+  if (!State->get<HStateMap>(Sym))
+    N = N->getFirstPred();
+
+  const ExplodedNode *Pred = N;
+  while (N) {
+    State = N->getState();
+    if (!State->get<HStateMap>(Sym)) {
+      const HandleState *HState = Pred->getState()->get<HStateMap>(Sym);
+      if (HState && (HState->isAllocated() || HState->maybeAllocated()))
+        return N;
+    }
+    Pred = N;
+    N = N->getFirstPred();
+  }
+  return nullptr;
+}
+
 /// Returns the symbols extracted from the argument or null if it cannot be
 /// found.
 SymbolRef getFuchsiaHandleSymbol(QualType QT, SVal Arg, ProgramStateRef State) {
   int PtrToHandleLevel = 0;
   while (QT->isAnyPointerType() || QT->isReferenceType()) {
     ++PtrToHandleLevel;
     QT = QT->getPointeeType();
   }
   if (const auto *HandleType = QT->getAs<TypedefType>()) {
     if (HandleType->getDecl()->getName() != HandleTypeName)
       return nullptr;
     if (PtrToHandleLevel > 1) {
       // Not supported yet.
       return nullptr;
     }
 
     if (PtrToHandleLevel == 0) {
       return Arg.getAsSymbol();
     } else {
       assert(PtrToHandleLevel == 1);
       if (Optional<Loc> ArgLoc = Arg.getAs<Loc>())

 void FuchsiaHandleChecker::checkPostCall(const CallEvent &Call,
                                          CheckerContext &C) const {
   const FunctionDecl *FuncDecl = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
   if (!FuncDecl)
     return;
 
   ProgramStateRef State = C.getState();
 
+  std::vector<std::function<std::string(BugReport & BR)>> Notes;
   SymbolRef ResultSymbol = nullptr;
   if (const auto *TypeDefTy = FuncDecl->getReturnType()->getAs<TypedefType>())
     if (TypeDefTy->getDecl()->getName() == ErrorTypeName)
       ResultSymbol = Call.getReturnValue().getAsSymbol();
 
   // Function returns an open handle.
   if (hasFuchsiaAttr<AcquireHandleAttr>(FuncDecl)) {
     SymbolRef RetSym = Call.getReturnValue().getAsSymbol();
for (unsigned Arg = 0; Arg < Call.getNumArgs(); ++Arg) {
 
     const HandleState *HState = State->get<HStateMap>(Handle);
     if (HState && HState->isEscaped())
       continue;
     if (hasFuchsiaAttr<ReleaseHandleAttr>(PVD)) {
       if (HState && HState->isReleased()) {
         reportDoubleRelease(Handle, Call.getArgSourceRange(Arg), C);
         return;
+      } else {
+        Notes.push_back([Handle](BugReport &BR) {
+          auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
+          if (auto IsInteresting = PathBR->getInterestingnessKind(Handle)) {
+            return "Handle released here.";
           } else
+            return "";
+        });
         State = State->set<HStateMap>(Handle, HandleState::getReleased());
+      }
     } else if (hasFuchsiaAttr<AcquireHandleAttr>(PVD)) {
+      Notes.push_back([Handle](BugReport &BR) {
+        auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
+        if (auto IsInteresting = PathBR->getInterestingnessKind(Handle)) {
+          return "Handle allocated here.";
+        } else
+          return "";
+      });
       State = State->set<HStateMap>(
           Handle, HandleState::getMaybeAllocated(ResultSymbol));
     }
   }
-  C.addTransition(State);
+  const NoteTag *T = nullptr;
+  if (!Notes.empty()) {
+    T = C.getNoteTag(
+        [this, Notes{std::move(Notes)}](BugReport &BR) -> std::string {
+          if (&BR.getBugType() != &UseAfterReleaseBugType &&
+              &BR.getBugType() != &LeakBugType &&
+              &BR.getBugType() != &DoubleReleaseBugType)
+            return "";
+          for (auto &Note : Notes) {
+            std::string Text = Note(BR);
+            if (!Text.empty())
+              return Text;
+          }
+          return "";
+        });
+  }
+  C.addTransition(State, T);
 }
 
 void FuchsiaHandleChecker::checkDeadSymbols(SymbolReaper &SymReaper,
                                             CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   SmallVector<SymbolRef, 2> LeakedSyms;
   HStateMapTy TrackedHandles = State->get<HStateMap>();
   for (auto &CurItem : TrackedHandles) {

 // the function failed.
 // Moreover, when a handle is known to be zero (the invalid handle),
 // we no longer can follow the symbol on the path, becaue the constant
 // zero will be used instead of the symbol. We also do not need to release
 // an invalid handle, so we remove the corresponding symbol from the state.
 ProgramStateRef FuchsiaHandleChecker::evalAssume(ProgramStateRef State,
                                                  SVal Cond,
                                                  bool Assumption) const {
+  // TODO: add notes about successes/fails for APIs.
   ConstraintManager &Cmr = State->getConstraintManager();
   HStateMapTy TrackedHandles = State->get<HStateMap>();
   for (auto &CurItem : TrackedHandles) {
     ConditionTruthVal HandleVal = Cmr.isNull(State, CurItem.first);
     if (HandleVal.isConstrainedTrue()) {
       // The handle is invalid. We can no longer follow the symbol on this path.
       State = State->remove<HStateMap>(CurItem.first);
     }

 
 void FuchsiaHandleChecker::reportBug(SymbolRef Sym, ExplodedNode *ErrorNode,
                                      CheckerContext &C,
                                      const SourceRange *Range,
                                      const BugType &Type, StringRef Msg) const {
   if (!ErrorNode)
     return;
 
-  auto R = std::make_unique<PathSensitiveBugReport>(Type, Msg, ErrorNode);
+  std::unique_ptr<PathSensitiveBugReport> R;
+  if (Type.isSuppressOnSink()) {
+    const ExplodedNode *AcquireNode = getAcquireSite(ErrorNode, Sym, C);
+    if (AcquireNode) {
+      PathDiagnosticLocation LocUsedForUniqueing =
+          PathDiagnosticLocation::createBegin(
+              AcquireNode->getStmtForDiagnostics(), C.getSourceManager(),
+              AcquireNode->getLocationContext());
+
+      R = std::make_unique<PathSensitiveBugReport>(
+          Type, Msg, ErrorNode, LocUsedForUniqueing,
+          AcquireNode->getLocationContext()->getDecl());
+    }
+  }
+  if (!R)
+    R = std::make_unique<PathSensitiveBugReport>(Type, Msg, ErrorNode);
   if (Range)
     R->addRange(*Range);
   R->markInteresting(Sym);
   C.emitReport(std::move(R));
 }
 
 void ento::registerFuchsiaHandleChecker(CheckerManager &mgr) {
   mgr.registerChecker<FuchsiaHandleChecker>();

clang/test/Analysis/fuchsia_handle.cpp

-// RUN: %clang_analyze_cc1 -analyzer-checker=core,fuchsia.HandleChecker -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,fuchsia.HandleChecker -analyzer-output=text \
+// RUN:     -verify %s
 
 typedef __typeof__(sizeof(int)) size_t;
 typedef int zx_status_t;
 typedef __typeof__(sizeof(int)) zx_handle_t;
 typedef unsigned int uint32_t;
 #define NULL ((void *)0)
 #define ZX_HANDLE_INVALID 0
 
void checkNoLeak06() {
   if (zx_channel_create(0, &sa, &sb))
     return;
   zx_handle_close(sa);
   zx_handle_close(sb);
 } 
 
 void checkLeak01(int tag) {
   zx_handle_t sa, sb;
-  if (zx_channel_create(0, &sa, &sb))
-    return;
+  if (zx_channel_create(0, &sa, &sb)) // expected-note    {{Handle allocated here}}
+    return;                           // expected-note@-1 {{Assuming the condition is false}}
+                                      // expected-note@-2 {{Taking false branch}}
   use1(&sa);
-  if (tag)
+  if (tag) // expected-note {{Assuming 'tag' is 0}}
     zx_handle_close(sa);
+  // expected-note@-2 {{Taking false branch}}
   use2(sb); // expected-warning {{Potential leak of handle}}
+  // expected-note@-1 {{Potential leak of handle}}
   zx_handle_close(sb);
 }
 
+void checkReportLeakOnOnePath(int tag) {
+  zx_handle_t sa, sb;
+  if (zx_channel_create(0, &sa, &sb)) // expected-note {{Handle allocated here}}
+    return;                           // expected-note@-1 {{Assuming the condition is false}}
+                                      // expected-note@-2 {{Taking false branch}}
+  zx_handle_close(sb);
+  switch(tag) { // expected-note {{Control jumps to the 'default' case at line}} 
+    case 0:
+      use2(sa);
+      return;
+    case 1:
+      use2(sa);
+      return;
+    case 2:
+      use2(sa);
+      return;
+    case 3:
+      use2(sa);
+      return;
+    case 4:
+      use2(sa);
+      return;
+    default:
+      use2(sa);
+      return; // expected-warning {{Potential leak of handle}}
+              // expected-note@-1 {{Potential leak of handle}}
+  }
+}
+
 void checkDoubleRelease01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
-  if (tag)
-    zx_handle_close(sa);
+  // expected-note@-1 {{Handle allocated here}}
+  if (tag) // expected-note {{Assuming 'tag' is not equal to 0}}
+    zx_handle_close(sa); // expected-note {{Handle released here}}
+  // expected-note@-2 {{Taking true branch}}
   zx_handle_close(sa); // expected-warning {{Releasing a previously released handle}}
+  // expected-note@-1 {{Releasing a previously released handle}}
   zx_handle_close(sb);
 }
 
 void checkUseAfterFree01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
+  // expected-note@-1 {{Handle allocated here}}
+  // expected-note@-2 {{Handle allocated here}}
+  // expected-note@+2 {{Taking true branch}}
+  // expected-note@+1 {{Taking false branch}}
   if (tag) {
-    zx_handle_close(sa);
+    // expected-note@-1 {{Assuming 'tag' is not equal to 0}}
+    zx_handle_close(sa); // expected-note {{Handle released here}}
     use1(&sa); // expected-warning {{Using a previously released handle}}
+    // expected-note@-1 {{Using a previously released handle}}
   }
-  zx_handle_close(sb);
+  // expected-note@-6 {{Assuming 'tag' is 0}}
+  zx_handle_close(sb); // expected-note {{Handle released here}}
   use2(sb); // expected-warning {{Using a previously released handle}}
+  // expected-note@-1 {{Using a previously released handle}}
 }
 
 void checkMemberOperatorIndices() {
   zx_handle_t sa, sb, sc;
   zx_channel_create(0, &sa, &sb);
   zx_handle_close(sb);
   MyType t;
   sc = t + sa;