r354642 - [analyzer] MIGChecker: Fix an FN when the object is released in a destructor.

[Artem Dergachev via cfe-commits]

Author: dergachev
Date: Thu Feb 21 16:09:56 2019
New Revision: 354642

URL: http://llvm.org/viewvc/llvm-project?rev=354642&view=rev
Log:
[analyzer] MIGChecker: Fix an FN when the object is released in a destructor.

When a MIG server routine argument is released in an automatic destructor,
the Static Analyzer thinks that this happens after the return statement, and so
the violation of the MIG convention doesn't happen.

Of course, it doesn't quite work that way, so this is a false negative.

Add a hack that makes the checker double-check at the end of function
that no argument was released when the routine fails with an error.

rdar://problem/35380337

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

Modified:
    cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
    cfe/trunk/test/Analysis/mig.mm

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp?rev=354642&r1=354641&r2=354642&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp Thu Feb 21 16:09:56 2019
@@ -32,15 +32,30 @@ using namespace clang;
 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);
+  }
 
   class Visitor : public BugReporterVisitor {
   public:
@@ -140,7 +155,7 @@ void MIGChecker::checkPostCall(const Cal
   C.addTransition(C.getState()->set<ReleasedParameter>(PVD));
 }
 
-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

Modified: cfe/trunk/test/Analysis/mig.mm
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/mig.mm?rev=354642&r1=354641&r2=354642&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/mig.mm (original)
+++ cfe/trunk/test/Analysis/mig.mm Thu Feb 21 16:09:56 2019
@@ -56,6 +56,31 @@ kern_return_t release_twice(mach_port_na
                      // expected-note at -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}}
 }
 
+// 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{{Value passed through parameter 'address' is deallocated}}
+    }
+  } 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 a use-after-free vulnerability because the caller will try to deallocate it again}}
+    // expected-note at -4   {{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;
+}
+
 // 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;