[PATCH] D58368: [analyzer] MIGChecker: Implement bug reporter visitors.

[Artem Dergachev via Phabricator via cfe-commits]

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

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 <https://reviews.llvm.org/D58367>) that explains where parameters are deallocated.


Repository:
  rC Clang

https://reviews.llvm.org/D58368

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


Index: clang/test/Analysis/mig.cpp
===================================================================
--- clang/test/Analysis/mig.cpp
+++ clang/test/Analysis/mig.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG -verify %s
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\
+// RUN:                       -analyzer-output=text -verify %s
+
 
 // XNU APIs.
 
@@ -16,9 +18,11 @@
 
 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{{Deallocating object passed through parameter 'address'}}
+  if (size > 10) { // expected-note{{Assuming 'size' is > 10}}
+                   // expected-note at -1{{Taking true branch}}
     return KERN_ERROR; // expected-warning{{MIG callback fails with error after deallocating argument value. This is use-after-free vulnerability because caller will try to deallocate it again}}
+                       // expected-note at -1{{MIG callback fails with error after deallocating argument value. This is use-after-free vulnerability because caller will try to deallocate it again}}
   }
   return KERN_SUCCESS;
 }
@@ -28,3 +32,15 @@
 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{{Deallocating object passed through parameter 'addr1'}}
+  vm_deallocate(port, addr2, size); // expected-note{{Deallocating object passed through parameter 'addr2'}}
+  return ret; // expected-warning{{MIG callback fails with error after deallocating argument value. This is use-after-free vulnerability because caller will try to deallocate it again}}
+                     // expected-note at -1{{MIG callback fails with error after deallocating argument value. This is use-after-free vulnerability because caller will try to deallocate it again}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -45,14 +45,17 @@
 
 REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, bool);
 
-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(const LocationContext *LC) {
@@ -86,8 +89,18 @@
 
   // 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;
+
+  const NoteTag *T = C.getNoteTag([PVD]() -> std::string {
+    SmallString<64> Str;
+    llvm::raw_svector_ostream OS(Str);
+    OS << "Deallocating object passed through parameter '" << PVD->getName()
+       << '\'';
+    return OS.str();
+  });
+  C.addTransition(C.getState()->set<ReleasedParameter>(true), T);
 }
 
 void MIGChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const {
@@ -129,6 +142,8 @@
       "deallocate it again",
       N);
 
+  R->addRange(RS->getSourceRange());
+  bugreporter::trackExpressionValue(N, RS->getRetValue(), *R, false);
   C.emitReport(std::move(R));
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D58368.187299.patch
Type: text/x-patch
Size: 4190 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190219/ca357062/attachment-0001.bin>