haowei created this revision.
Herald added a subscriber: xazax.hun.

This patch adds "mx_create_sink" annotation support to MagentaHandleChecker to 
address the false positives found in Magenta unit tests. After this patch, when 
a call to a function contains this annotation, MagentaHandleChecker will create 
a sink node which will suppress the handle leaks warnings if the leak is 
post-dominated by this function (similar to a noreturn function).

The target problem it tries to solve can be illustrated in following example:

  static bool handle_info_test(void) {
      BEGIN_TEST;
  
      mx_handle_t event;
      ASSERT_EQ(mx_event_create(0u, &event), 0, "");
      mx_handle_t duped;
      mx_status_t status = mx_handle_duplicate(event, MX_RIGHT_SAME_RIGHTS, 
&duped);
      ASSERT_EQ(status, MX_OK, "");
      // ....
  }

Unlike the "assert" keyword in C++, the ASSERT_EQ macro will not terminate the 
execution of the unit test program (in other words, it will not invoke a 
noreturn function), instead it will return false and the error will be recorded 
by the unit test runner. Therefore, before this patch, the MagentaHandleChecker 
will report handle leaks on these assertion failures as the function reaches 
return and symbol that contains the acquired handles are acquired. These 
reports are not helpful as assertion failures in unit tests mean test failures. 
With the help of this patch, we add a call to a function with "mx_create_sink" 
annotation in the failure branch in definition "ASSERT_EQ". In this case, the 
MagentaHandleChecker will create a sink node in each assertion failure and 
suppress the warnings if the leaks are post-dominated by assertion failures.


https://reviews.llvm.org/D36475

Files:
  lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp
  test/Analysis/mxhandle.c

Index: test/Analysis/mxhandle.c
===================================================================
--- test/Analysis/mxhandle.c
+++ test/Analysis/mxhandle.c
@@ -60,6 +60,8 @@
 
 void noReturnFunc() __attribute__((__noreturn__));
 
+void sinkFunc() MX_SYSCALL_PARAM_ATTR(create_sink);
+
 int leakingFunc(int argc, char* argv[]) {
   mx_handle_t sa, sb;
   mx_channel_create(0, &sa, &sb);
@@ -228,6 +230,14 @@
   noReturnFunc(); // Should not report any bugs here
 }
 
+void checkNoLeak17() {
+  mx_handle_t sa, sb;
+  if (mx_channel_create(0, &sa, &sb) < 0) {
+    return;
+  }
+  sinkFunc(); // Should not report any bugs here
+}
+
 void checkLeak01() {
   mx_handle_t sa, sb;
   mx_channel_create(0, &sa, &sb);
Index: lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp
@@ -123,7 +123,9 @@
   UNPROCESSED_FUNC,
   // When a bug is found in function with this flag, do not report this bug
   // Used to suppress known false positives.
-  DONOTREPORT_FUNC
+  DONOTREPORT_FUNC,
+  // When a function has 'create_sink' annotation , create a sink node.
+  SINK_FUNC
 };
 
 enum AnnotationFlag {
@@ -144,7 +146,8 @@
   // This value is not describing the possible return value of a annotated
   // function. It basically tell the checker do not report any bugs found in
   // this function. Used to suppress false positive reports.
-  DONOTREPORT
+  DONOTREPORT,
+  CREATE_SINK
 };
 
 struct ArgSpec {
@@ -416,7 +419,8 @@
                    {"handle_escape", ESCAPE},
                    {"handle_use", NOESCAPE},
                    {"may_fail", BIFURCATE},
-                   {"suppress_warning", DONOTREPORT}};
+                   {"suppress_warning", DONOTREPORT},
+                   {"create_sink", CREATE_SINK}};
 
   // Hard coded FuncSpec for mx_channel_read and mx_channel_write
   // We currently don't have a clean way to annotate handles passed through
@@ -455,6 +459,10 @@
         FuncKindMap[FuncDecl] == UNPROCESSED_FUNC ||
         FuncKindMap[FuncDecl] == DONOTREPORT_FUNC)
       return false;
+    if (FuncKindMap[FuncDecl] == SINK_FUNC) {
+      C.generateSink(C.getState(), C.getPredecessor());
+      return true;
+    }
   }
   // Check if the function has annotation and has already been processed before
   if (FuncDeclMap.count(FuncDecl))
@@ -545,6 +553,11 @@
     FuncKindMap[FD] = DONOTREPORT_FUNC;
     return false;
   }
+  if (FS.RetAction == CREATE_SINK) {
+    FuncKindMap[FD] = SINK_FUNC;
+    Ctx.generateSink(Ctx.getState(), Ctx.getPredecessor());
+    return true;
+  }
   SmallVector<SVal, 3> invalidateSVal;
   ProgramStateRef State = Ctx.getState();
   ASTContext &ASTCtx = State->getStateManager().getContext();
@@ -1035,13 +1048,14 @@
     HasValidAnnotation = true;
   }
   // Return type is not mx_status_t but has annotation other than
-  // SYMBOLIC|DONOTREPORT Consider it as an error
+  // SYMBOLIC|DONOTREPORT|CREATE_SINK Consider it as an error
   if (RetType.getAsString().find(SYSCALL_RETURN_TYPE_NAME) &&
-      (FS.RetAction != SYMBOLIC && FS.RetAction != DONOTREPORT))
+      (FS.RetAction != SYMBOLIC && FS.RetAction != DONOTREPORT &&
+       FS.RetAction != CREATE_SINK))
     return false;
 
   if (FS.RetAction != BIFURCATE && FS.RetAction != SYMBOLIC &&
-      FS.RetAction != DONOTREPORT)
+      FS.RetAction != DONOTREPORT && FS.RetAction != CREATE_SINK)
     return false;
   // Extract ParamDecl's annotation.
   int ArgId = -1;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to