[analyzer] MIGChecker: Improve intermediate diagnostic notes.

Add a BugReporterVisitor for highlighting the events of deallocating a
parameter. All such events are relevant to the emitted report (as long as the
report is indeed emitted), so all of them will get highlighted.

Add a trackExpressionValue visitor for highlighting where does the error return
code come from.

Do not add a trackExpressionValue visitor for highlighting how the deallocated
argument(s) was(were) copied around. This still remains to be implemented.

rdar://problem/35380337

Differential Revision: https://reviews.llvm.org/D58368

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@354641 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/StaticAnalyzer/Checkers/MIGChecker.cpp b/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
index 92fd4b7f8a3e959929555e41c93a4c62a83bcb68..e67bea5a0da839e54ae4a2f58b507fbf7db35d2d 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -41,19 +41,56 @@ class MIGChecker : public Checker<check::PostCall, check::PreStmt<ReturnStmt>> {
 public:
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
   void checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const;
+
+  class Visitor : public BugReporterVisitor {
+  public:
+    void Profile(llvm::FoldingSetNodeID &ID) const {
+      static int X = 0;
+      ID.AddPointer(&X);
+    }
+
+    std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
+        BugReporterContext &BRC, BugReport &R);
+  };
 };
 } // end anonymous namespace
 
-REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, bool)
+// FIXME: It's a 'const ParmVarDecl *' but there's no ready-made GDM traits
+// specialization for this sort of types.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, const void *)
+
+std::shared_ptr<PathDiagnosticPiece>
+MIGChecker::Visitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC,
+                               BugReport &R) {
+  const auto *NewPVD = static_cast<const ParmVarDecl *>(
+      N->getState()->get<ReleasedParameter>());
+  const auto *OldPVD = static_cast<const ParmVarDecl *>(
+      N->getFirstPred()->getState()->get<ReleasedParameter>());
+  if (OldPVD == NewPVD)
+    return nullptr;
+
+  assert(NewPVD && "What is deallocated cannot be un-deallocated!");
+  SmallString<64> Str;
+  llvm::raw_svector_ostream OS(Str);
+  OS << "Value passed through parameter '" << NewPVD->getName()
+     << "' is deallocated";
+
+  PathDiagnosticLocation Loc =
+      PathDiagnosticLocation::create(N->getLocation(), BRC.getSourceManager());
+  return std::make_shared<PathDiagnosticEventPiece>(Loc, OS.str());
+}
 
-static bool isCurrentArgSVal(SVal V, CheckerContext &C) {
+static const ParmVarDecl *getOriginParam(SVal V, CheckerContext &C) {
   SymbolRef Sym = V.getAsSymbol();
   if (!Sym)
-    return false;
+    return nullptr;
 
   const auto *VR = dyn_cast_or_null<VarRegion>(Sym->getOriginRegion());
-  return VR && VR->hasStackParametersStorage() &&
-         VR->getStackFrame()->inTopFrame();
+  if (VR && VR->hasStackParametersStorage() &&
+         VR->getStackFrame()->inTopFrame())
+    return cast<ParmVarDecl>(VR->getDecl());
+
+  return nullptr;
 }
 
 static bool isInMIGCall(CheckerContext &C) {
@@ -96,8 +133,11 @@ void MIGChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const {
 
   // TODO: Unhardcode "1".
   SVal Arg = Call.getArgSVal(1);
-  if (isCurrentArgSVal(Arg, C))
-    C.addTransition(C.getState()->set<ReleasedParameter>(true));
+  const ParmVarDecl *PVD = getOriginParam(Arg, C);
+  if (!PVD)
+    return;
+
+  C.addTransition(C.getState()->set<ReleasedParameter>(PVD));
 }
 
 void MIGChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const {
@@ -139,6 +179,9 @@ void MIGChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const {
       "deallocate it again",
       N);
 
+  R->addRange(RS->getSourceRange());
+  bugreporter::trackExpressionValue(N, RS->getRetValue(), *R, false);
+  R->addVisitor(llvm::make_unique<Visitor>());
   C.emitReport(std::move(R));
 }
 

diff --git a/test/Analysis/mig.mm b/test/Analysis/mig.mm
index 6c70bfc31442eca7bedd28dccdfd8be9359032ae..6bcd45b36a169b93dfc126a64d27ca30c341ad70 100644 (file)
--- a/test/Analysis/mig.mm
+++ b/test/Analysis/mig.mm
@@ -1,5 +1,5 @@
 // RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\
-// RUN:                    -fblocks -verify %s
+// RUN:                       -analyzer-output=text -fblocks -verify %s
 
 // XNU APIs.
 
@@ -20,9 +20,11 @@ kern_return_t vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
 
 MIG_SERVER_ROUTINE
 kern_return_t basic_test(mach_port_name_t port, vm_address_t address, vm_size_t size) {
-  vm_deallocate(port, address, size);
-  if (size > 10) {
+  vm_deallocate(port, address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
+  if (size > 10) { // expected-note{{Assuming 'size' is > 10}}
+                   // expected-note@-1{{Taking true branch}}
     return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+                                                                                        // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
   }
   return KERN_SUCCESS;
 }
@@ -42,6 +44,18 @@ kern_return_t no_crash(mach_port_name_t port, vm_address_t address, vm_size_t si
   vm_deallocate(port, address, size);
 }
 
+// When releasing two parameters, add a note for both of them.
+// Also when returning a variable, explain why do we think that it contains
+// a non-success code.
+MIG_SERVER_ROUTINE
+kern_return_t release_twice(mach_port_name_t port, vm_address_t addr1, vm_address_t addr2, vm_size_t size) {
+  kern_return_t ret = KERN_ERROR; // expected-note{{'ret' initialized to 1}}
+  vm_deallocate(port, addr1, size); // expected-note{{Value passed through parameter 'addr1' is deallocated}}
+  vm_deallocate(port, addr2, size); // expected-note{{Value passed through parameter 'addr2' is deallocated}}
+  return ret; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+                     // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+}
+
 // Check that we work on Objective-C messages and blocks.
 @interface I
 - (kern_return_t)fooAtPort:(mach_port_name_t)port withAddress:(vm_address_t)address ofSize:(vm_size_t)size;
@@ -51,8 +65,9 @@ kern_return_t no_crash(mach_port_name_t port, vm_address_t address, vm_size_t si
 - (kern_return_t)fooAtPort:(mach_port_name_t)port
                withAddress:(vm_address_t)address
                     ofSize:(vm_size_t)size MIG_SERVER_ROUTINE {
-  vm_deallocate(port, address, size);
+  vm_deallocate(port, address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
   return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+                     // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
 }
 @end
 
@@ -60,8 +75,9 @@ void test_block() {
   kern_return_t (^block)(mach_port_name_t, vm_address_t, vm_size_t) =
       ^MIG_SERVER_ROUTINE (mach_port_name_t port,
                            vm_address_t address, vm_size_t size) {
-        vm_deallocate(port, address, size);
+        vm_deallocate(port, address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
         return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
+                           // expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
       };
 }