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

[NoQ]

Summary

The problem that was fixed in 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 because in the basic_test test the paths get merged before invoking the check::EndFunction callback, which makes it impossible to recover the return value (which never gets stored in the Environment because it's, well, a literal). 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.

[NoQ]

Rebase.

[dcoughlin]

Oh, interesting. I'm glad you thought of this!

[NoQ]

Rebase.

clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp

 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 
 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);
+  }
 };
 } // end anonymous namespace
 
 REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, bool);
 
 static const ParmVarDecl *getOriginParam(SVal V, CheckerContext &C) {
   SymbolRef Sym = V.getAsSymbol();
   if (!Sym)
const NoteTag *T = C.getNoteTag([PVD]() -> std::string {
     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 {
+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
   // 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
   // a random routine in the middle of nowhere, but given that the convention is
   // fairly weird and hard to follow in the first place, there's relatively
   // little motivation to spread it this way.

clang/test/Analysis/mig.mm

 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@-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@-1{{Assuming 'size' is > 10}}
+    // expected-note@-2{{Taking true branch}}
+    return KERN_ERROR;
+    // expected-note@-1{{Calling '~WillDeallocate'}}
+    // expected-note@-2{{Returning from '~WillDeallocate'}}
+    // expected-warning@-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@-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;
 @end
 
 @implementation I
 - (kern_return_t)fooAtPort:(mach_port_name_t)port
                withAddress:(vm_address_t)address