From 3ce78f54edcfc881377a9e567715caf2f0be2abc Mon Sep 17 00:00:00 2001
From: Yu Shan <shanyu@google.com>
Date: Mon, 7 Dec 2020 10:58:44 -0800
Subject: [PATCH] [analyzer] Ignore annotations if func is inlined.

When we annotating a function header so that it could be used by other
TU, we also need to make sure the function is parsed correctly within
the same TU. So if we can find the function's implementation,
ignore the annotations, otherwise, false positive would occur.
Move the escape by value case to post call and do not escape the handle
if the function is inlined and we have analyzed the handle.

Differential Revision: https://reviews.llvm.org/D91902
---
 .../Checkers/FuchsiaHandleChecker.cpp              | 17 +++++++---
 clang/test/Analysis/fuchsia_handle.cpp             | 39 ++++++++++++++++++++++
 2 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
index d4901eb..c246a8d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
@@ -331,11 +331,6 @@ void FuchsiaHandleChecker::checkPreCall(const CallEvent &Call,
           return;
         }
       }
-      if (!hasFuchsiaAttr<UseHandleAttr>(PVD) &&
-          PVD->getType()->isIntegerType()) {
-        // Working around integer by-value escapes.
-        State = State->set<HStateMap>(Handle, HandleState::getEscaped());
-      }
     }
   }
   C.addTransition(State);
@@ -347,6 +342,10 @@ void FuchsiaHandleChecker::checkPostCall(const CallEvent &Call,
   if (!FuncDecl)
     return;
 
+  // If we analyzed the function body, then ignore the annotations.
+  if (C.wasInlined)
+    return;
+
   ProgramStateRef State = C.getState();
 
   std::vector<std::function<std::string(BugReport & BR)>> Notes;
@@ -417,6 +416,14 @@ void FuchsiaHandleChecker::checkPostCall(const CallEvent &Call,
         });
         State = State->set<HStateMap>(
             Handle, HandleState::getMaybeAllocated(ResultSymbol));
+      } else if (!hasFuchsiaAttr<UseHandleAttr>(PVD) &&
+                 PVD->getType()->isIntegerType()) {
+        // Working around integer by-value escapes.
+        // The by-value escape would not be captured in checkPointerEscape.
+        // If the function was not analyzed (otherwise wasInlined should be
+        // true) and there is no annotation on the handle, we assume the handle
+        // is escaped.
+        State = State->set<HStateMap>(Handle, HandleState::getEscaped());
       }
     }
   }
diff --git a/clang/test/Analysis/fuchsia_handle.cpp b/clang/test/Analysis/fuchsia_handle.cpp
index 911ab7a..99f0449 100644
--- a/clang/test/Analysis/fuchsia_handle.cpp
+++ b/clang/test/Analysis/fuchsia_handle.cpp
@@ -315,6 +315,45 @@ void checkHandlePtrInStructureLeak() {
   // expected-note@-1 {{Potential leak of handle}}
 }
 
+// Assume this function's declaration that has the release annotation is in one
+// header file while its implementation is in another file. We have to annotate
+// the declaration because it might be used outside the TU.
+// We also want to make sure it is okay to call the function within the same TU.
+zx_status_t test_release_handle(zx_handle_t handle ZX_HANDLE_RELEASE) {
+  return zx_handle_close(handle);
+}
+
+void checkReleaseImplementedFunc() {
+  zx_handle_t a, b;
+  zx_channel_create(0, &a, &b);
+  zx_handle_close(a);
+  test_release_handle(b);
+}
+
+void use_handle(zx_handle_t handle) {
+  // Do nothing.
+}
+
+void test_call_by_value() {
+  zx_handle_t a, b;
+  zx_channel_create(0, &a, &b);
+  zx_handle_close(a);
+  use_handle(b);
+  zx_handle_close(b);
+}
+
+void test_call_by_value_leak() {
+  zx_handle_t a, b;
+  zx_channel_create(0, &a, &b); // expected-note {{Handle allocated through 3rd parameter}}
+  zx_handle_close(a);
+  // Here we are passing handle b as integer value to a function that could be
+  // analyzed by the analyzer, thus the handle should not be considered escaped.
+  // After the function 'use_handle', handle b is still tracked and should be
+  // reported leaked.
+  use_handle(b);
+} // expected-warning {{Potential leak of handle}}
+// expected-note@-1 {{Potential leak of handle}}
+
 // RAII
 
 template <typename T>
-- 
2.7.4