[analyzer] Fix handle leak false positive when the handle dies too early

[xazax.hun]

Summary

In case the handle symbol dies too early, even before we check the status, we might generate spurious leak warnings.

This pattern is not very common in production code but it is very common in unittests where we do know that certain syscall will fail.

[xazax.hun]

• Minor refactoring.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

133–135

It already makes me mildly uncomfortable that our data is not "normalized", i.e. you can indicate the Schrödinger state by either adding an error symbol or changing from Allocated to MaybeAllocated. Do i understand it correctly that you're now adding a special state where the handle is still MaybeAllocated but there's no error symbol? Please comment this up.

415

I think it should be possible to implement it without checkLiveSymbols, because i don't have any reasons to believe that you can find the handle symbol by looking at the error symbol, or vice versa, so i think they aren't supposed to keep each other alive.

I.e., i think you can implement this by intentionally leaving zombie handles in your state until the respective error symbol dies. After all, you don't rely on any other metadata attached to your handles (eg., range constraints), you only need your map, right?

I'm open to discussion here. I understand that intentional zombies sound scary, but they also look slightly more correct to me. "The handle is definitely dead by now, but the successfulness of its birth is still a 'known unknown', so let's delay our warning and keep the corpse frozen until we find out if it was ever born to begin with" - sounds about right :/ It's probably not a big deal in any case, but i'm curious WDYT.

clang/test/Analysis/fuchsia_handle.cpp

69

I'd like to see how do notes look when the warning *is* emitted (i.e., same test but replace status < 0 with status >= 0).

We don't have a note for the collapse point of the Schrödinger state, right? (i think it was an unfortunate inevitable use case for a bug visitor because you can't have tags in evalAssume)

That is, how comfortable would it be for the user to see that the leak warning is emitted significantly later than the handle is dead? We already aren't great at positioning our leak warnings, but it makes it even more unobvious. I guess it's probably fine, but i want to see it.

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

133–135

Yeah, the reason is that, when the handle is a return value, we have no idea where the error symbol is. This would only happen if someone do not follow the API guidelines (in Fuchsia). I'll add a comment.

415

I see, yeah this makes sense to me. I will try this approach out and report back :)

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

133–135

Sorry, I was hasty and my head was full of another patch. It has nothing to do with returned handles. I used this to control when to let some symbols die. But with your suggested approach this factory is no longer needed.

[xazax.hun]

• Address review comments.

[xazax.hun]

I've rerun this on Fuchsia and it got the same results as the previous approach :).

[NoQ]

Yay, it actually worked!

[jroelofs]

clang/test/Analysis/fuchsia_handle.cpp

77

@xazax.hun, @dcoughlin Is it expected that these diagnostics be non-deterministic? I'm seeing this test case fail sporadically [1] because sometimes the analyzer sees that the 3rd argument leaks, and other times it sees that the 2nd does.

The easy fix here would be to adjust the regex to be flexible against that difference, but I didn't want to cover up an underlying issue by committing that blindly:

// expected-note@-1 {{Handle allocated through (2nd|3rd) parameter}}

1: http://lab.llvm.org:8080/green/job/clang-stage1-cmake-RA-expensive/17048/console

[jroelofs]

clang/test/Analysis/fuchsia_handle.cpp

77

erm, wrong link. this is the correct one:

http://lab.llvm.org:8080/green/job/clang-stage1-cmake-RA-expensive/17049/#showFailuresLink

[NoQ]

clang/test/Analysis/fuchsia_handle.cpp

77

Hmm, no, these diagnostics shouldn't be non-deterministic. It's a bug. Thanks a lot for noticing!

Non-determinism in *notes* shouldn't be too bad. Also investigation may get pretty long because diagnostic sorting and de-duplication is a highly automated process. I believe we should cover up the problem to suppress buildbot failures and investigate when we can. I'll commit the cover-up.

[NoQ]

clang/test/Analysis/fuchsia_handle.cpp

77

rGc26f237cef1

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

static HandleState getAllocated(ProgramStateRef State, HandleState S) {
     return HandleState(Kind::Allocated, nullptr);
   }
   static HandleState getReleased() {
     return HandleState(Kind::Released, nullptr);
   }
   static HandleState getEscaped() {
     return HandleState(Kind::Escaped, nullptr);
   }
+  static HandleState getWithoutError(HandleState S) {
+    return HandleState(S.K, nullptr);
+  }
 
   SymbolRef getErrorSym() const { return ErrorSym; }
 
   void Profile(llvm::FoldingSetNodeID &ID) const {
     ID.AddInteger(static_cast<int>(K));
     ID.AddPointer(ErrorSym);
   }
 
   LLVM_DUMP_METHOD void dump(raw_ostream &OS) const {
     switch (K) {
 #define CASE(ID)                                                               \
   case ID:                                                                     \
     OS << #ID;                                                                 \
     break;
       CASE(Kind::MaybeAllocated)
       CASE(Kind::Allocated)
       CASE(Kind::Released)
       CASE(Kind::Escaped)
     }
+    if (ErrorSym) {
+      OS << " ErrorSym: ";
+      ErrorSym->dumpToStream(OS);
+    }
   }
 
   LLVM_DUMP_METHOD void dump() const { dump(llvm::errs()); }
 };
 
 template <typename Attr> static bool hasFuchsiaAttr(const Decl *D) {
   return D->hasAttr<Attr>() && D->getAttr<Attr>()->getHandleType() == "Fuchsia";
 }
 
 class FuchsiaHandleChecker
     : public Checker<check::PostCall, check::PreCall, check::DeadSymbols,
-                     check::PointerEscape, eval::Assume> {
+                     check::LiveSymbols, check::PointerEscape, eval::Assume> {
   BugType LeakBugType{this, "Fuchsia handle leak", "Fuchsia Handle Error",
                       /*SuppressOnSink=*/true};
   BugType DoubleReleaseBugType{this, "Fuchsia handle double release",
                                "Fuchsia Handle Error"};
   BugType UseAfterReleaseBugType{this, "Fuchsia handle use after release",
                                  "Fuchsia Handle Error"};
 
 public:
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
+  void checkLiveSymbols(ProgramStateRef State, SymbolReaper &SR) const;
   ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
                              bool Assumption) const;
   ProgramStateRef checkPointerEscape(ProgramStateRef State,
                                      const InvalidatedSymbols &Escaped,
                                      const CallEvent *Call,
                                      PointerEscapeKind Kind) const;
 
   ExplodedNode *reportLeaks(ArrayRef<SymbolRef> LeakedHandles,

 }
 
 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) {
+    SymbolRef ErrorSym = CurItem.second.getErrorSym();
+    if (ErrorSym && SymReaper.isDead(ErrorSym))
+      State = State->set<HStateMap>(
+          CurItem.first, HandleState::getWithoutError(CurItem.second));
     if (!SymReaper.isDead(CurItem.first))
       continue;
     if (CurItem.second.isAllocated() || CurItem.second.maybeAllocated())
       LeakedSyms.push_back(CurItem.first);
     State = State->remove<HStateMap>(CurItem.first);
   }
 
   ExplodedNode *N = C.getPredecessor();
   if (!LeakedSyms.empty())
     N = reportLeaks(LeakedSyms, C, N);
 
   C.addTransition(State, N);
 }
 
+void FuchsiaHandleChecker::checkLiveSymbols(ProgramStateRef State,
+                                            SymbolReaper &SymReaper) const {
+  HStateMapTy TrackedHandles = State->get<HStateMap>();
+  for (auto &CurItem : TrackedHandles) {
+    if (CurItem.second.getErrorSym())
+      SymReaper.markLive(CurItem.first);
+  }
+}
+
 // Acquiring a handle is not always successful. In Fuchsia most functions
 // return a status code that determines the status of the handle.
 // When we split the path based on this status code we know that on one
 // path we do have the handle and on the other path the acquire failed.
 // This method helps avoiding false positive leak warnings on paths where
 // 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

clang/test/Analysis/fuchsia_handle.cpp

void checkInvalidHandle2() {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
   if (sb != ZX_HANDLE_INVALID)
     zx_handle_close(sb);
   if (sa != ZX_HANDLE_INVALID)
     zx_handle_close(sa);
 }
 
+void handleDieBeforeErrorSymbol() {
+  zx_handle_t sa, sb;
+  zx_status_t status = zx_channel_create(0, &sa, &sb);
+  if (status < 0)
+    return;
+  __builtin_trap();
+}
+
 void checkNoCrash01() {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
   moreArgs(sa, 1, 2, 3, 4, 5);
   lessArgs(sa);
   zx_handle_close(sa);
   zx_handle_close(sb);
 }
 
 void checkNoLeak01() {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);