[PATCH] D61925: [analyzer] MIGChecker: Add support for os_ref_retain().

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added a reviewer: dcoughlin.
Herald added subscribers: cfe-commits, Charusso, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: clang.

Suppress MIG checker false positives that occur when the programmer increments 
the reference count before calling a MIG destructor, and the MIG destructor 
literally boils down to decrementing the reference count.


Repository:
  rC Clang

https://reviews.llvm.org/D61925

Files:
  clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  clang/test/Analysis/mig.mm


Index: clang/test/Analysis/mig.mm
===================================================================
--- clang/test/Analysis/mig.mm
+++ clang/test/Analysis/mig.mm
@@ -19,11 +19,24 @@
 typedef unsigned mach_port_t;
 typedef uint32_t UInt32;
 
+struct os_refcnt {};
+typedef struct os_refcnt os_refcnt_t;
+
+struct thread {
+  os_refcnt_t ref_count;
+};
+typedef struct thread *thread_t;
+
 kern_return_t vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
 kern_return_t mach_vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
 void mig_deallocate(vm_address_t, vm_size_t);
 kern_return_t mach_port_deallocate(ipc_space_t, mach_port_name_t);
 void ipc_port_release(ipc_port_t);
+void thread_deallocate(thread_t);
+
+static void os_ref_retain(struct os_refcnt *rc);
+
+#define thread_reference_internal(thread) os_ref_retain(&(thread)->ref_count);
 
 #define MIG_SERVER_ROUTINE __attribute__((mig_server_routine))
 
@@ -237,3 +250,10 @@
                            // expected-note@-1{{MIG callback fails with error after deallocating argument value}}
   }
 };
+
+MIG_SERVER_ROUTINE
+kern_return_t test_os_ref_retain(thread_t thread) {
+  thread_reference_internal(thread);
+  thread_deallocate(thread);
+  return KERN_ERROR; // no-warning
+}
Index: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -84,6 +84,8 @@
 #undef CALL
   };
 
+  CallDescription OsRefRetain{"os_ref_retain", 1};
+
   void checkReturnAux(const ReturnStmt *RS, CheckerContext &C) const;
 
 public:
@@ -105,10 +107,17 @@
 };
 } // end anonymous namespace
 
+// A flag that says that the programmer has called a MIG destructor
+// for at least one parameter.
 REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, bool)
-
-static const ParmVarDecl *getOriginParam(SVal V, CheckerContext &C) {
-  SymbolRef Sym = V.getAsSymbol();
+// A set of parameters for which the check is suppressed because
+// reference counting is being performed.
+REGISTER_SET_WITH_PROGRAMSTATE(RefCountedParameters, const ParmVarDecl *);
+
+static const ParmVarDecl *getOriginParam(SVal V, CheckerContext &C,
+                                         bool IncludeBaseRegions = false) {
+  // TODO: We should most likely always include base regions here.
+  SymbolRef Sym = V.getAsSymbol(IncludeBaseRegions);
   if (!Sym)
     return nullptr;
 
@@ -168,6 +177,19 @@
 }
 
 void MIGChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const {
+  if (Call.isCalled(OsRefRetain)) {
+    // If the code is doing reference counting over the parameter,
+    // it opens up an opportunity for safely calling a destructor function.
+    // TODO: We should still check for over-releases.
+    if (const ParmVarDecl *PVD =
+            getOriginParam(Call.getArgSVal(0), C, /*IncludeBaseRegions=*/true)) {
+      // We never need to clean up the program state because these are
+      // top-level parameters anyway, so they're always live.
+      C.addTransition(C.getState()->add<RefCountedParameters>(PVD));
+    }
+    return;
+  }
+
   if (!isInMIGCall(C))
     return;
 
@@ -178,10 +200,11 @@
   if (I == Deallocators.end())
     return;
 
+  ProgramStateRef State = C.getState();
   unsigned ArgIdx = I->second;
   SVal Arg = Call.getArgSVal(ArgIdx);
   const ParmVarDecl *PVD = getOriginParam(Arg, C);
-  if (!PVD)
+  if (!PVD || State->contains<RefCountedParameters>(PVD))
     return;
 
   const NoteTag *T = C.getNoteTag([this, PVD](BugReport &BR) -> std::string {
@@ -193,7 +216,7 @@
        << "\' is deallocated";
     return OS.str();
   });
-  C.addTransition(C.getState()->set<ReleasedParameter>(true), T);
+  C.addTransition(State->set<ReleasedParameter>(true), T);
 }
 
 // Returns true if V can potentially represent a "successful" kern_return_t.

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