[analyzer] MIGChecker: Pour more data into the checker.

[NoQ]

Summary

Previous patches were about the infrastructure of the checker. Right now the infrastructure is pretty much complete, so let's make use. Namely:

• Add more release functions. For now only vm_deallocate() was supported. I'll also have a look at adding an attribute so that users could annotate their own releasing functions, but hardcoding a few popular APIs wouldn't hurt.
• Add a non-zero value that isn't an error; this value is -305 ("MIG_NO_REPLY") and it's fine to deallocate data when you are returning this error.
• Make sure that the mig_server_routine annotation is inherited. Mmm, not sure, i expected Sema to do this automatically. I'll ask in D58365.

[NoQ]

Rebase. Fix behavior when the return code is not constrained enough. Test the C++11 attribute syntax (just in case). Update comments.

[dcoughlin]

Aah, MIG_NO_REPLY.

LGTM.

clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp

42 ↗

(On Diff #187505)

Could you put a comment with an example indicating how CALL works for a particular example function? I think that will make is easier for folks to add support for new APIs in the future without getting it wrong.

125 ↗

(On Diff #187505)

Perhaps we could make it a Sema error if a method doesn't have a mig server routine annotation but it overrides a method that does. From a documentation/usability perspective it would be unfortunate if a human looking at a method to determine its convention would have to look at its super classes to see whether they have an annotation too.

Although, thinking about it more that might make it a source-breaking change if an API author were to add annotation on a super class. Then API clients would have to add the annotations to get their projects to build, which would not be great..

162 ↗

(On Diff #187505)

Could we rename this to "mayBeSuccess()" or something like that so that the name of function indicate the "potentially" part of the comment.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp

125 ↗

(On Diff #187505)

Yup, i believe that given that the checker is an optional thing (even if we make it opt-out the hard way), annotating APIs should also be optional. The primary use case for this override trick is the IOUserClient::externalMethod() API that gets annotated within IOKit itself and makes the attribute automatically apply to all overrides in all IOKit projects, automagically making them subject to testing. But if we make it an error to not annotate the overrides, it'd break every single IOKit project and require them to add dozens of annotations before it compiles, so i think it's not feasible.

We might as well hardcode the IOUserClient::externalMethod() thing (instead of annotating it) and in this case it would make much more sense to enforce fully annotating all overrides.

[NoQ]

Address comments.

cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp

 //== MIGChecker.cpp - MIG calling convention checker ------------*- C++ -*--==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines MIGChecker, a Mach Interface Generator calling convention
 // checker. Namely, in MIG callback implementation the following rules apply:
-// - When a server routine returns KERN_SUCCESS, it must take ownership of
-//   resources (and eventually release them).
-// - Additionally, when returning KERN_SUCCESS, all out-parameters must be
+// - When a server routine returns an error code that represents success, it
+//   must take ownership of resources passed to it (and eventually release
+//   them).
+// - Additionally, when returning success, all out-parameters must be
 //   initialized.
-// - When it returns anything except KERN_SUCCESS it must not take ownership,
-//   because the message and its descriptors will be destroyed by the server
-//   function.
+// - When it returns any other error code, it must not take ownership,
+//   because the message and its out-of-line parameters will be destroyed
+//   by the client that called the function.
 // For now we only check the last rule, as its violations lead to dangerous
 // use-after-free exploits.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Analysis/AnyCall.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #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>,
                                   check::EndFunction> {
   BugType BT{this, "Use-after-free (MIG calling convention violation)",
              categories::MemoryError};
 
-  CallDescription vm_deallocate { "vm_deallocate", 3 };
+  // The checker knows that an out-of-line object is deallocated if it is
+  // passed as an argument to one of these functions. If this object is
+  // additionally an argument of a MIG routine, the checker keeps track of that
+  // information and issues a warning when an error is returned from the
+  // respective routine.
+  std::vector<std::pair<CallDescription, unsigned>> Deallocators = {
+#define CALL(required_args, deallocated_arg, ...)                              \
+  {{{__VA_ARGS__}, required_args}, deallocated_arg}
+      // E.g., if the checker sees a C function 'vm_deallocate' that is
+      // defined on class 'IOUserClient' that has exactly 3 parameters, it knows
+      // that argument #1 (starting from 0, i.e. the second argument) is going
+      // to be consumed in the sense of the MIG consume-on-success convention.
+      CALL(3, 1, "vm_deallocate"),
+      CALL(3, 1, "mach_vm_deallocate"),
+      CALL(2, 0, "mig_deallocate"),
+      CALL(2, 1, "mach_port_deallocate"),
+      // E.g., if the checker sees a method 'releaseAsyncReference64()' that is
+      // defined on class 'IOUserClient' that takes exactly 1 argument, it knows
+      // that the argument is going to be consumed in the sense of the MIG
+      // consume-on-success convention.
+      CALL(1, 0, "IOUserClient", "releaseAsyncReference64"),
+#undef CALL
+  };
 
   void checkReturnAux(const ReturnStmt *RS, CheckerContext &C) const;
 
 public:
   void checkPostCall(const CallEvent &Call, 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
MIGChecker::Visitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC,
   return std::make_shared<PathDiagnosticEventPiece>(Loc, OS.str());
 }
 
 static const ParmVarDecl *getOriginParam(SVal V, CheckerContext &C) {
   SymbolRef Sym = V.getAsSymbol();
   if (!Sym)
     return nullptr;
 
-  const auto *VR = dyn_cast_or_null<VarRegion>(Sym->getOriginRegion());
+  // If we optimistically assume that the MIG routine never re-uses the storage
+  // that was passed to it as arguments when it invalidates it (but at most when
+  // it assigns to parameter variables directly), this procedure correctly
+  // determines if the value was loaded from the transitive closure of MIG
+  // routine arguments in the heap.
+  while (const MemRegion *MR = Sym->getOriginRegion()) {
+    const auto *VR = dyn_cast<VarRegion>(MR);
     if (VR && VR->hasStackParametersStorage() &&
            VR->getStackFrame()->inTopFrame())
       return cast<ParmVarDecl>(VR->getDecl());
 
+    const SymbolicRegion *SR = MR->getSymbolicBase();
+    if (!SR)
+      return nullptr;
+
+    Sym = SR->getSymbol();
+  }
+
   return nullptr;
 }
 
 static bool isInMIGCall(CheckerContext &C) {
   const LocationContext *LC = C.getLocationContext();
   const StackFrameContext *SFC;
   // Find the top frame.
   while (LC) {
if (Optional<AnyCall> AC = AnyCall::forDecl(D)) {
     if (!AC->getReturnType(C.getASTContext())
              .getCanonicalType()->isSignedIntegerType())
       return false;
   }
 
   if (D->hasAttr<MIGServerRoutineAttr>())
     return true;
 
+  // See if there's an annotated method in the superclass.
+  if (const auto *MD = dyn_cast<CXXMethodDecl>(D))
+    for (const auto *OMD: MD->overridden_methods())
+      if (OMD->hasAttr<MIGServerRoutineAttr>())
+        return true;
+
   return false;
 }
 
 void MIGChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const {
   if (!isInMIGCall(C))
     return;
 
-  if (!Call.isGlobalCFunction())
+  auto I = std::find_if(Deallocators.begin(), Deallocators.end(),
+                        [&](const std::pair<CallDescription, unsigned> &Item) {
+                          return Call.isCalled(Item.first);
+                        });
+  if (I == Deallocators.end())
     return;
 
-  if (!Call.isCalled(vm_deallocate))
-    return;
-
-  // TODO: Unhardcode "1".
-  SVal Arg = Call.getArgSVal(1);
+  unsigned ArgIdx = I->second;
+  SVal Arg = Call.getArgSVal(ArgIdx);
   const ParmVarDecl *PVD = getOriginParam(Arg, C);
   if (!PVD)
     return;
 
   C.addTransition(C.getState()->set<ReleasedParameter>(PVD));
 }
 
+// Returns true if V can potentially represent a "successful" kern_return_t.
+static bool mayBeSuccess(SVal V, CheckerContext &C) {
+  ProgramStateRef State = C.getState();
+
+  // Can V represent KERN_SUCCESS?
+  if (!State->isNull(V).isConstrainedFalse())
+    return true;
+
+  SValBuilder &SVB = C.getSValBuilder();
+  ASTContext &ACtx = C.getASTContext();
+
+  // Can V represent MIG_NO_REPLY?
+  static const int MigNoReply = -305;
+  V = SVB.evalEQ(C.getState(), V, SVB.makeIntVal(MigNoReply, ACtx.IntTy));
+  if (!State->isNull(V).isConstrainedTrue())
+    return true;
+
+  // If none of the above, it's definitely an error.
+  return false;
+}
+
 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
void MIGChecker::checkReturnAux(const ReturnStmt *RS, CheckerContext &C) const {
   if (!RS)
     return;
 
   ProgramStateRef State = C.getState();
   if (!State->get<ReleasedParameter>())
     return;
 
   SVal V = C.getSVal(RS);
-  if (!State->isNonNull(V).isConstrainedTrue())
+  if (mayBeSuccess(V, C))
     return;
 
   ExplodedNode *N = C.generateErrorNode();
   if (!N)
     return;
 
   auto R = llvm::make_unique<BugReport>(
       BT,

cfe/trunk/test/Analysis/mig.mm

 // RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\
 // RUN:                       -analyzer-output=text -fblocks -verify %s
 
+typedef unsigned uint32_t;
+
 // XNU APIs.
 
 typedef int kern_return_t;
 #define KERN_SUCCESS 0
 #define KERN_ERROR 1
+#define MIG_NO_REPLY (-305)
 
 typedef unsigned mach_port_name_t;
 typedef unsigned vm_address_t;
 typedef unsigned vm_size_t;
+typedef void *ipc_space_t;
+typedef unsigned long io_user_reference_t;
 
 kern_return_t vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
+kern_return_t mach_vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
+void mig_deallocate(vm_address_t, vm_size_t);
+kern_return_t mach_port_deallocate(ipc_space_t, mach_port_name_t);
 
 #define MIG_SERVER_ROUTINE __attribute__((mig_server_routine))
 
+// IOKit wrappers.
+
+class OSObject;
+typedef kern_return_t IOReturn;
+#define kIOReturnError 1
+
+enum {
+  kOSAsyncRef64Count = 8,
+};
+
+typedef io_user_reference_t OSAsyncReference64[kOSAsyncRef64Count];
+
+struct IOExternalMethodArguments {
+  io_user_reference_t *asyncReference;
+};
+
+struct IOExternalMethodDispatch {};
+
+class IOUserClient {
+public:
+  static IOReturn releaseAsyncReference64(OSAsyncReference64);
+
+  MIG_SERVER_ROUTINE
+  virtual IOReturn externalMethod(uint32_t selector, IOExternalMethodArguments *arguments,
+                                  IOExternalMethodDispatch *dispatch = 0, OSObject *target = 0, void *reference = 0);
+};
+
 
 // Tests.
 
 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); // expected-note{{Value passed through parameter 'address' is deallocated}}
   if (size > 10) { // expected-note{{Assuming 'size' is > 10}}
                    // expected-note@-1{{Taking true branch}}
void test_block_with_weird_return_type() {
   // function.
   Empty (^block)(mach_port_name_t, vm_address_t, vm_size_t) =
       ^MIG_SERVER_ROUTINE(mach_port_name_t port,
                           vm_address_t address, vm_size_t size) {
         vm_deallocate(port, address, size);
         return Empty{}; // no-crash
       };
 }
+
+// Test various APIs.
+MIG_SERVER_ROUTINE
+kern_return_t test_mach_vm_deallocate(mach_port_name_t port, vm_address_t address, vm_size_t size) {
+  mach_vm_deallocate(port, address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
+  return KERN_ERROR;                 // expected-warning{{MIG callback fails with error after deallocating argument value}}
+                                     // expected-note@-1{{MIG callback fails with error after deallocating argument value}}
+}
+
+MIG_SERVER_ROUTINE
+kern_return_t test_mach_port_deallocate(ipc_space_t space,
+                                        mach_port_name_t port) {
+  mach_port_deallocate(space, port); // expected-note{{Value passed through parameter 'port' is deallocated}}
+  return KERN_ERROR;                 // expected-warning{{MIG callback fails with error after deallocating argument value}}
+                                     // expected-note@-1{{MIG callback fails with error after deallocating argument value}}
+}
+
+MIG_SERVER_ROUTINE
+kern_return_t test_mig_deallocate(vm_address_t address, vm_size_t size) {
+  mig_deallocate(address, size); // expected-note{{Value passed through parameter 'address' is deallocated}}
+  return KERN_ERROR;             // expected-warning{{MIG callback fails with error after deallocating argument value}}
+                                 // expected-note@-1{{MIG callback fails with error after deallocating argument value}}
+}
+
+// Let's try the C++11 attribute spelling syntax as well.
+[[clang::mig_server_routine]]
+IOReturn test_releaseAsyncReference64(IOExternalMethodArguments *arguments) {
+  IOUserClient::releaseAsyncReference64(arguments->asyncReference); // expected-note{{Value passed through parameter 'arguments' is deallocated}}
+  return kIOReturnError;                                            // expected-warning{{MIG callback fails with error after deallocating argument value}}
+                                                                    // expected-note@-1{{MIG callback fails with error after deallocating argument value}}
+}
+
+MIG_SERVER_ROUTINE
+kern_return_t test_no_reply(ipc_space_t space, mach_port_name_t port) {
+  mach_port_deallocate(space, port);
+  return MIG_NO_REPLY; // no-warning
+}
+
+class MyClient: public IOUserClient {
+  // The MIG_SERVER_ROUTINE annotation is intentionally skipped.
+  // It should be picked up from the superclass.
+  IOReturn externalMethod(uint32_t selector, IOExternalMethodArguments *arguments,
+                          IOExternalMethodDispatch *dispatch = 0, OSObject *target = 0, void *reference = 0) override {
+
+    releaseAsyncReference64(arguments->asyncReference); // expected-note{{Value passed through parameter 'arguments' is deallocated}}
+    return kIOReturnError;                              // expected-warning{{MIG callback fails with error after deallocating argument value}}
+                                                        // expected-note@-1{{MIG callback fails with error after deallocating argument value}}
+  }
+};