[PATCH] D58392: [analyzer] MIGChecker: Fix false negatives for releases in automatic destructors.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 187503.
NoQ added a comment.

Rebase.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58392/new/

https://reviews.llvm.org/D58392

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
@@ -56,6 +56,31 @@
                      // 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}}
 }
 
+// Make sure we find the bug when the object is destroyed within an
+// automatic destructor.
+MIG_SERVER_ROUTINE
+kern_return_t test_vm_deallocate_in_automatic_dtor(mach_port_name_t port, vm_address_t address, vm_size_t size) {
+  struct WillDeallocate {
+    mach_port_name_t port;
+    vm_address_t address;
+    vm_size_t size;
+    ~WillDeallocate() {
+      vm_deallocate(port, address, size); // expected-note{{Deallocating object passed through parameter 'address'}}
+    }
+  } will_deallocate{port, address, size};
+
+ if (size > 10) {
+    // expected-note at -1{{Assuming 'size' is > 10}}
+    // expected-note at -2{{Taking true branch}}
+    return KERN_ERROR;
+    // expected-note at -1{{Calling '~WillDeallocate'}}
+    // expected-note at -2{{Returning from '~WillDeallocate'}}
+    // expected-warning at -3{{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 -4   {{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;
+}
+
 // 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;
Index: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -32,15 +32,30 @@
 using namespace ento;
 
 namespace {
-class MIGChecker : public Checker<check::PostCall, check::PreStmt<ReturnStmt>> {
+class MIGChecker : public Checker<check::PostCall, check::PreStmt<ReturnStmt>,
+                                  check::EndFunction> {
   BugType BT{this, "Use-after-free (MIG calling convention violation)",
              categories::MemoryError};
 
   CallDescription vm_deallocate { "vm_deallocate", 3 };
 
+  void checkReturnAux(const ReturnStmt *RS, CheckerContext &C) const;
+
 public:
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
-  void checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const;
+
+  // HACK: We're making two attempts to find the bug: checkEndFunction
+  // should normally be enough but it fails when the return value is a literal
+  // that never gets put into the Environment and ends of function with multiple
+  // returns get agglutinated across returns, preventing us from obtaining
+  // the return value. The problem is similar to https://reviews.llvm.org/D25326
+  // but now we step into it in the top-level function.
+  void checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const {
+    checkReturnAux(RS, C);
+  }
+  void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const {
+    checkReturnAux(RS, C);
+  }
 };
 } // end anonymous namespace
 
@@ -113,7 +128,7 @@
   C.addTransition(C.getState()->set<ReleasedParameter>(true), T);
 }
 
-void MIGChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const {
+void MIGChecker::checkReturnAux(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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D58392.187503.patch
Type: text/x-patch
Size: 3829 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190220/7cddc414/attachment.bin>