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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: NoQ, haowei.
xazax.hun added a project: clang.
Herald added subscribers: Charusso, gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73151

Files:
  clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
  clang/test/Analysis/fuchsia_handle.cpp


Index: clang/test/Analysis/fuchsia_handle.cpp
===================================================================
--- clang/test/Analysis/fuchsia_handle.cpp
+++ clang/test/Analysis/fuchsia_handle.cpp
@@ -63,6 +63,14 @@
     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);
Index: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
@@ -130,6 +130,9 @@
   static HandleState getEscaped() {
     return HandleState(Kind::Escaped, nullptr);
   }
+  static HandleState getWithoutError(HandleState S) {
+    return HandleState(S.K, nullptr);
+  }
 
   SymbolRef getErrorSym() const { return ErrorSym; }
 
@@ -149,6 +152,10 @@
       CASE(Kind::Released)
       CASE(Kind::Escaped)
     }
+    if (ErrorSym) {
+      OS << " ErrorSym: ";
+      ErrorSym->dumpToStream(OS);
+    }
   }
 
   LLVM_DUMP_METHOD void dump() const { dump(llvm::errs()); }
@@ -160,7 +167,7 @@
 
 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",
@@ -172,6 +179,7 @@
   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,
@@ -381,6 +389,10 @@
   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())
@@ -395,6 +407,16 @@
   C.addTransition(State, N);
 }
 
+void FuchsiaHandleChecker::checkLiveSymbols(ProgramStateRef State,
+                                            SymbolReaper &SymReaper) const {
+  HStateMapTy TrackedHandles = State->get<HStateMap>();
+  for (auto &CurItem : TrackedHandles) {
+    SymbolRef ErrorSym = CurItem.second.getErrorSym();
+    if (ErrorSym)
+      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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D73151.239468.patch
Type: text/x-patch
Size: 3382 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200122/ce87e9a5/attachment.bin>