[analyzer] MIGChecker: Implement bug reporter visitors.

[NoQ]

Summary

This adds two visitors to the checker:

• trackExpressionValue() in order to highlight where does the return value come from when it's not a literal.
• A tag-based visitor (as in D58367) that explains where parameters are deallocated.

[NoQ]

Rebase.

[dcoughlin]

Looks good to me. I have some a minor diagnostic wording suggestion in line.

clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp

109 ↗

(On Diff #187502)

Could we just have the note say "'name' is deallocated"?

Or "Value passed through parameter 'name' is deallocated"

The ".. is ... " construction matches our other checkers. (Like "Memory is released" from the Malloc Checker.)

clang/test/Analysis/mig.mm

52 ↗

(On Diff #187502)

A nice QoI improvement here (for a later patch, perhaps) would be to have this note use the macro name: "'ret initialized to KERN_ERROR'".

Users probably won't know that KERN_ERROR is 1.

[Charusso]

clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp

113 ↗

(On Diff #187502)

This is a cool approach, but it is difficult to use this API in other checkers. If you do not out-chain D58367 I would like to see something like that here:

SmallString<64> Str;
llvm::raw_svector_ostream OS(Str);
OS << "Deallocating object passed through parameter '" << PVD->getName()
   << '\'';

C.addNote(C.getState()->set<ReleasedParameter>(true), OS.str());

[Charusso]

clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp

47 ↗

(On Diff #187502)

; is not necessary.

[NoQ]

Thx!

clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp

109 ↗

(On Diff #187502)

Great point! I'd pick the latter because it's important to point out that the value is loaded from the parameter.

Hmm, btw, we should probably add more "visitors" in order to explain that the value is indeed copied from the parameter.

113 ↗

(On Diff #187502)

I'll reply in D58367 because it seems to be universally relevant :)

clang/test/Analysis/mig.mm

52 ↗

(On Diff #187502)

Yup, but that's a separate story, because this message is produced by a generic, checker-inspecific visitor. We'll have to teach trackNullOrUndefValue() trackExpressionValue() to be aware of macros, and it might turn out that we'd also want to mention macro names in messages that correspond to the subsequent copies of the same value (which, in turn, is tricky as it causes time paradoxes due to visiting order).

[NoQ]

clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp

47 ↗

(On Diff #187502)

Addressed in the earlier patch, D57558.

[NoQ]

Address comments.

@Charusso: I agreed not to rush for D58367 and implemented an old-style visitor here instead :)

[Charusso]

In D58368#1406490, @NoQ wrote:

Address comments.

@Charusso: I agreed not to rush for D58367 and implemented an old-style visitor here instead :)

Thanks you! I wanted to accept it when you remove it from the review-chain and after rewriting the summary. The latter is not that big deal, just wanted to let you know it remained the same.

cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp

class MIGChecker : public Checker<check::PostCall, check::PreStmt<ReturnStmt>> {
   BugType BT{this, "Use-after-free (MIG calling convention violation)",
              categories::MemoryError};
 
   CallDescription vm_deallocate { "vm_deallocate", 3 };
 
 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) {
   const LocationContext *LC = C.getLocationContext();
   const StackFrameContext *SFC;
   // Find the top frame.
   while (LC) {
     SFC = LC->getStackFrame();
void MIGChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const {
   if (!Call.isGlobalCFunction())
     return;
 
   if (!Call.isCalled(vm_deallocate))
     return;
 
   // 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 {
   // It is very unlikely that a MIG callback will be called from anywhere
   // within the project under analysis and the caller isn't itself a routine
   // that follows the MIG calling convention. Therefore we're safe to believe
   // that it's always the top frame that is of interest. There's a slight chance
   // that the user would want to enforce the MIG calling convention upon
void MIGChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const {
 
   auto R = llvm::make_unique<BugReport>(
       BT,
       "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",
       N);
 
+  R->addRange(RS->getSourceRange());
+  bugreporter::trackExpressionValue(N, RS->getRetValue(), *R, false);
+  R->addVisitor(llvm::make_unique<Visitor>());
   C.emitReport(std::move(R));
 }
 
 void ento::registerMIGChecker(CheckerManager &Mgr) {
   Mgr.registerChecker<MIGChecker>();
 }
 
 bool ento::shouldRegisterMIGChecker(const LangOptions &LO) {
   return true;
 }

cfe/trunk/test/Analysis/mig.mm

 // RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\
-// RUN:                    -fblocks -verify %s
+// RUN:                       -analyzer-output=text -fblocks -verify %s
 
 // XNU APIs.
 
 typedef int kern_return_t;
 #define KERN_SUCCESS 0
 #define KERN_ERROR 1
 
 typedef unsigned mach_port_name_t;
 typedef unsigned vm_address_t;
 typedef unsigned vm_size_t;
 
 kern_return_t vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
 
 #define MIG_SERVER_ROUTINE __attribute__((mig_server_routine))
 
 
 // Tests.
 
 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;
 }
 
 MIG_SERVER_ROUTINE
 kern_return_t test_unknown_return_value(mach_port_name_t port, vm_address_t address, vm_size_t size) {
   extern kern_return_t foo();
 
   vm_deallocate(port, address, size);
   // We don't know if it's a success or a failure.
   return foo(); // no-warning
 }
 
 // Make sure we don't crash when they forgot to write the return statement.
 MIG_SERVER_ROUTINE
 kern_return_t no_crash(mach_port_name_t port, vm_address_t address, vm_size_t size) {
   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;
 @end
 
 @implementation I
 - (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
 
 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}}
       };
 }
 
 void test_block_with_weird_return_type() {
   struct Empty {};
 
   // The block is written within a function so that it was actually analyzed as
   // a top-level function during analysis. If we were to write it as a global