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

[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.

The problem that was fixed in D25326 <https://reviews.llvm.org/D25326> for inlined functions keeps biting us for the top-level functions as well. Namely, i had to use `check::PreStmt<ReturnStmt>` rather than `check::EndFunction` in D57558 <https://reviews.llvm.org/D57558> because in the `basic_test` test the paths get merged before invoking the `check::EndFunction` callback. I don't have an immediate fix for the whole overall problem, so for now, as a hack, `check::PreStmt<ReturnStmt>` keeps being used.

However, the problem with `check::PreStmt<ReturnStmt>` is that automatic destructors for function-local variables are not yet evaluated when this callback is invoked. This causes false negatives in MIGChecker: if memory is released in an automatic destructor and then an error code is returned, the bug is not found because the Static Analyzer evalues the destructor *after* the pre-return. Arguably, this may also be treated as a bug, depending on what does the "Pre" in `PreStmt<ReturnStmt>` stand for.

But regardless, for now it seems that we'd have to subscribe on both callbacks.


Repository:
  rC Clang

https://reviews.llvm.org/D58392

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
@@ -44,3 +44,28 @@
   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}}
 }
+
+// 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;
+}
Index: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -31,15 +31,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
 
@@ -103,7 +118,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.187408.patch
Type: text/x-patch
Size: 3835 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190219/bc45ca5d/attachment-0001.bin>