[analyzer] Suppress warning when bug path contains noreturn function or return from main in MagentaHandleChecker

[haowei]

Summary

This patch will suppress handle leak warnings if the path of a bug reported by MagentaHandleChecker will reach a no return function or will reach the end of main function. As these two scenarios mean program crash and the leaked handle will be recycled by OS, there is no point to report these bugs.

[NoQ]

lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp

483–498

I think the analyzer core should do this. This code already has a global effect on the analysis - it's enough for one checker to generate the sink. Additionally, there's also the CFG-based variant of suppress-on-sink, which would need to be extended to support this as well - this other variant kicks in when the analysis was interrupted before reaching the sink (see D35673 and D35674).

[xazax.hun]

lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp

483–498

Do we want to do this unconditionally? Are all of the resources cleaned up on all of the supported OSes, or maybe for some leak issues it still makes sense to warn in these cases? Or we simply favor false negatives over false positives in this case (might make sense)?

[NoQ]

lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp

483–498

We could add a flag to setSuppressOnSink() as an orthogonal change if it turns out that we need it; i'm not aware of any stuff that badly needs to be cleaned up before normal program termination in the existing checkers.

[haowei]

lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp

483–498

I also think this should be done in a separate checker like NoReturnFunctionChecker or within the analyzer as some other resource leak checkers might need this. I am also thinking D36475 should be in a separate checker as well. That annotation support really helps us a lot in analyzing unit test code and it might be useful for others if someone run into similar situation like us. But I cannot decide if we should use the annotate attribute or define a new attribute just for the analyzer.

[dcoughlin]

I'm worried about doing this for main(). I think many people would find it surprising if analyzing main() were to behave differently than other functions. I'm particularly worried because I think it is quite common to create a small test program with only main() to understand the behavior of the analyzer.

[haowei]

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

lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp

 static const char *ANNOTATION_PREFIX = "mx_";
 static const char *HANDLE_TYPE_NAME = "mx_handle_t";
 static const char *SYSCALL_RETURN_TYPE_NAME = "mx_status_t";
 static const int MAX_HANDLE_IN_ARRAY = 64;
 typedef std::pair<const ExplodedNode *, const MemRegion *> AllocInfo;
 
 class MagentaHandleChecker
     : public Checker<check::DeadSymbols, check::PointerEscape, eval::Call,
-                     check::PostCall> {
+                     check::PostCall, check::PreStmt<ReturnStmt>> {
   std::unique_ptr<BugType> LeakBugType;
   std::unique_ptr<BugType> DoubleReleaseBugType;
   std::unique_ptr<BugType> UseAfterFreeBugType;
 
   mutable llvm::DenseMap<const FunctionDecl *, FuncSpec> FuncDeclMap;
   mutable llvm::StringMap<FuncSpec> SpecialFuncDeclMap;
   mutable llvm::DenseMap<const FunctionDecl *, FuncKind> FuncKindMap;
   mutable llvm::StringMap<AnnotationFlag> AnnotationMap;
 
 public:
   MagentaHandleChecker();
   // Checker callbacks
   void checkPostCall(const CallEvent &Call, CheckerContext &Ctx) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &Ctx) const;
   bool evalCall(const CallExpr *CE, CheckerContext &Ctx) const;
+  void checkPreStmt(const ReturnStmt *RS, CheckerContext &Ctx) const;
   ProgramStateRef checkPointerEscape(ProgramStateRef State,
                                      const InvalidatedSymbols &Escaped,
                                      const CallEvent *Call,
                                      PointerEscapeKind Kind) const;
 
 private:
   // Evaluate handle related function call with annotation information
   bool evalCallWithFuncSpec(const CallExpr *CE, const FunctionDecl *FD,
MagentaBugVisitor::VisitNode(const ExplodedNode *N, const ExplodedNode *PrevN,
   }
   return nullptr;
 }
 
 MagentaHandleChecker::MagentaHandleChecker() {
   // Initialize the bug types.
   LeakBugType.reset(
       new BugType(this, "MX handle leak", "Magenta Handle Error"));
+  LeakBugType->setSuppressOnSink(true);
   DoubleReleaseBugType.reset(
       new BugType(this, "MX handle double release", "Magenta Handle Error"));
   UseAfterFreeBugType.reset(
       new BugType(this, "MX handle use after free", "Magenta Handle Error"));
 
   // Initialize the map for supported annotations.
   AnnotationMap = {{"handle_acquire", ALLOCATE},
                    {"handle_release", RELEASE},
void MagentaHandleChecker::checkPostCall(const CallEvent &Call,
                                          CheckerContext &Ctx) const {
   ProgramStateRef State = Ctx.getState();
   const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
   if (!FD || !FuncKindMap.count(FD) || (FuncKindMap[FD] != ANNOATATED_FUNC)) {
     processUninlinedCalls(State, Call, Ctx);
   }
 }
 
+void MagentaHandleChecker::checkPreStmt(const ReturnStmt *RS,
+                                        CheckerContext &Ctx) const {
+  ProgramStateRef State = Ctx.getState();
+  const StackFrameContext *SFCtx = Ctx.getStackFrame();
+  if (!SFCtx || !SFCtx->inTopFrame())
+    return;
+  const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(SFCtx->getDecl());
+  if (!FD)
+    return;
+  const IdentifierInfo *II = FD->getIdentifier();
+  if (!II)
+    return;
+
+  if (II->isStr("main"))
+    Ctx.generateSink(State, Ctx.getPredecessor());
+}
+
 ProgramStateRef MagentaHandleChecker::checkPointerEscape(
     ProgramStateRef State, const InvalidatedSymbols &Escaped,
     const CallEvent *Call, PointerEscapeKind Kind) const {
   for (const SymbolRef &EscapedSymbol : Escaped) {
     const HandleState *CurHS = State->get<HStateMap>(EscapedSymbol);
     if (CurHS) {
       State = State->remove<HStateMap>(EscapedSymbol);
       State =

test/Analysis/mxhandle.c

 // Handle should not escape when mx_handle_use is present. It should work
 // for both value type and pointer type arguments.
 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__));
+
+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[]) {
+  if (argc == 2)
+    return leakingFunc(argc, argv); // Should not report any bugs here
+  return 0;
+}
 
 void checkNoLeak01() {
   mx_handle_t sa, sb;
   mx_channel_create(0, &sa, &sb);
   mx_handle_close(sa);
   mx_handle_close(sb);
 }
 
void checkNoLeak15(int tag) {
   if (tag) {
     escapeMethod04(sa);
     escapeMethod04(sb);
   }
   mx_handle_close(sa);
   mx_handle_close(sb);
 }
 
+void checkNoLeak16() {
+  mx_handle_t sa, sb;
+  if (mx_channel_create(0, &sa, &sb) < 0) {
+    return;
+  }
+  noReturnFunc(); // 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;