[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 updated this revision to Diff 240337.
xazax.hun added a comment.

- Address review comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73151/new/

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
@@ -66,6 +66,26 @@
     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);
Index: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
@@ -149,6 +149,10 @@
       CASE(Kind::Released)
       CASE(Kind::Escaped)
     }
+    if (ErrorSym) {
+      OS << " ErrorSym: ";
+      ErrorSym->dumpToStream(OS);
+    }
   }
 
   LLVM_DUMP_METHOD void dump() const { dump(llvm::errs()); }
@@ -401,7 +405,13 @@
   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);



Index: clang/test/Analysis/fuchsia_handle.cpp
===================================================================
--- clang/test/Analysis/fuchsia_handle.cpp
+++ clang/test/Analysis/fuchsia_handle.cpp
@@ -66,6 +66,26 @@
     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);
Index: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
@@ -149,6 +149,10 @@
       CASE(Kind::Released)
       CASE(Kind::Escaped)
     }
+    if (ErrorSym) {
+      OS << " ErrorSym: ";
+      ErrorSym->dumpToStream(OS);
+    }
   }
 
   LLVM_DUMP_METHOD void dump() const { dump(llvm::errs()); }
@@ -401,7 +405,13 @@
   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);

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits