[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.

433

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

72

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.

433

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

80

@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

80

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

80

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

80

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);
   }
 
   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";
 }

 }
 
 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) {
-    if (!SymReaper.isDead(CurItem.first))
+    SymbolRef ErrorSym = CurItem.second.getErrorSym();
+    // Keeping zombie handle symbols. In case the error symbol is dying later
+    // than the handle symbol we might produce spurious leak warnings (in case
+    // we find out later from the status code that the handle allocation failed
+    // in the first place).
+    if (!SymReaper.isDead(CurItem.first) ||
+        (ErrorSym && !SymReaper.isDead(ErrorSym)))
       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);
 }
 
 // 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
 // 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.

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 handleDieBeforeErrorSymbol01() {
+  zx_handle_t sa, sb;
+  zx_status_t status = zx_channel_create(0, &sa, &sb);
+  if (status < 0)
+    return;
+  __builtin_trap();
+}
+
+void handleDieBeforeErrorSymbol02() {
+  zx_handle_t sa, sb;
+  zx_status_t status = zx_channel_create(0, &sa, &sb);
+  // expected-note@-1 {{Handle allocated through 2nd parameter}}
+  if (status == 0) { // expected-note {{Assuming 'status' is equal to 0}}
+                     // expected-note@-1 {{Taking true branch}}
+    return; // expected-warning {{Potential leak of handle}}
+            // expected-note@-1 {{Potential leak of handle}}
+  }
+  __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);
 }