[analyzer] Add "create_sink" annotation support to MagentaHandleChecker

[haowei]

Summary

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.

[dcoughlin]

I'm a bit surprised that this is needed for unit tests. Aren't unit tests supposed to clean up their own state? Leaking a scarce resource in one unit test can cause another unit test to fail, which can be hard to track down. Or is this for deliberately testing a scenario where a resource is leaked? If you're deliberately testing a leaking scenario, it seems to me that using #ifndef clang_analyzer would probably be an easier suppression mechanism for the test author to understand and maintain.

Can you elaborate on why you're running the analyzer on units tests and how reports from the analyzer on unit tests are used? I think it would be helpful for us to understand your work flow.

Also, in general, my feeling is the exposing the internal mechanisms of the analyzer (here, the notion of a sink) in an attribute is not good for users. Ideally, attributes should be used to describe the behavior of code that they annotate. This means that they also serve as documentation for the user and not just as an ad hoc mechanism to communicate with the analyzer.

[haowei]

An updated version was landed in f4c26d993bdc . This diff is no longer needed.

lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp

enum FuncKind {
   ANNOATATED_FUNC,
   // Functions that do not have any annotation at all
   UNANNOTATED_FUNC,
   // Functions have annotations, but cannot be processed correctly due to
   // mismatches to the arguments.
   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 {
   ALLOCATE,
   RELEASE,
   // Same as RELEASE but will not cause bifurcation in evalCall. Added due to
   // Magenta syscalls changes.
   RELEASE_ALWAYS,
   ESCAPE,
   NOESCAPE,
   // Annotated function will return 0 on succeess and less than 0 on failure
   // functions with this annotation will cause checker to bifurcate to two
   // during during evaluation, the successful state and failed state.
   BIFURCATE,
   // Annotated function can return any value.
   // The checker will not bifurcate the states
   SYMBOLIC,
   // 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 {
   enum Kind { VALUE, POINTER, ARRAY } K;
   // The index of the argument, start from 0.
   int ArgIdx;
   // The action of this argument. handle acquire/release/escape/noescape
   AnnotationFlag ArgAction;
MagentaHandleChecker::MagentaHandleChecker() {
 
   // Initialize the map for supported annotations.
   AnnotationMap = {{"handle_acquire", ALLOCATE},
                    {"handle_release", RELEASE},
                    {"handle_release_always", RELEASE_ALWAYS},
                    {"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
   // arrays, as it requires another argument for the counts. So stay with
   // this hard coded solution for now. We will clean it up once we have an
   // enhanced annotation in the future.
   FuncSpec ChannelReadSpec(BIFURCATE);
   ArgSpec ChannelReadArg0(0, NOESCAPE, ArgSpec::VALUE);
bool MagentaHandleChecker::evalCall(const CallExpr *CE,
 
   // Check if this function is known not to have annotation or known to contain
   // error in annotation
   if (FuncKindMap.count(FuncDecl)) {
     if (FuncKindMap[FuncDecl] == UNANNOTATED_FUNC ||
         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))
     return evalCallWithFuncSpec(CE, FuncDecl, C, FuncDeclMap[FuncDecl]);
 
   // Check if function has annotation
   if (generateSpecForFuncionDecl(FuncDecl)) {
     FuncSpec FS = FuncDeclMap[FuncDecl];

 bool MagentaHandleChecker::evalCallWithFuncSpec(const CallExpr *CE,
                                                 const FunctionDecl *FD,
                                                 CheckerContext &Ctx,
                                                 FuncSpec &FS) const {
   if (FS.RetAction == DONOTREPORT) {
     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();
   const LocationContext *LCtx = Ctx.getLocationContext();
 
   // Check the return type
   bool IsRetTypeInt = false;
   QualType RetType = CE->getCallReturnType(ASTCtx);
bool MagentaHandleChecker::generateSpecForFuncionDecl(
   FS.RetAction = SYMBOLIC;
   QualType RetType = FuncDecl->getReturnType();
   if (!FuncAnnotation.empty() && AnnotationMap.count(FuncAnnotation)) {
     FS.RetAction = AnnotationMap[FuncAnnotation];
     HasFuncDeclAnnotation = true;
     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;
   for (auto &PD : FuncDecl->parameters()) {
     ++ArgId;
     QualType ParamType = PD->getOriginalType();
     // We only process mx_handle_t type arguments
     if (ParamType.getAsString().find(HANDLE_TYPE_NAME))

test/Analysis/mxhandle.c

 void useHandle01(
 MX_SYSCALL_PARAM_ATTR(handle_use) mx_handle_t handle);
 
 void useHandle02(
 MX_SYSCALL_PARAM_ATTR(handle_use) mx_handle_t *handle);
 
 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);
   return 1;
 }
 
 // End of declaration
 int main(int argc, char* argv[]) {

 void checkNoLeak16() {
   mx_handle_t sa, sb;
   if (mx_channel_create(0, &sa, &sb) < 0) {
     return;
   }
   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);
 } // expected-warning 2 {{Potential leak of handle pointed to by}}
 
 
 void checkLeak02(int tag) {
   mx_handle_t sa, sb;