Index: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp @@ -20,6 +20,7 @@ // //===----------------------------------------------------------------------===// +#include "clang/Analysis/AnyCall.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" @@ -55,8 +56,8 @@ VR->getStackFrame()->inTopFrame(); } -// This function will probably be replaced with looking up annotations. -static bool isInMIGCall(const LocationContext *LC) { +static bool isInMIGCall(CheckerContext &C) { + const LocationContext *LC = C.getLocationContext(); const StackFrameContext *SFC; // Find the top frame. while (LC) { @@ -64,21 +65,27 @@ LC = SFC->getParent(); } - const auto *FD = dyn_cast(SFC->getDecl()); - if (!FD) - return false; + const Decl *D = SFC->getDecl(); + + if (Optional AC = AnyCall::forDecl(D)) { + // Even though there's a Sema warning when the return type of an annotated + // function is not a kern_return_t, this warning isn't an error, so we need + // an extra sanity check here. + // FIXME: AnyCall doesn't support blocks yet, so they remain unchecked + // for now. + if (!AC->getReturnType(C.getASTContext()) + .getCanonicalType()->isSignedIntegerType()) + return false; + } + + if (D->hasAttr()) + return true; - // FIXME: This is an unreliable (even if surprisingly reliable) heuristic. - // The real solution here is to make MIG annotate its callbacks in - // autogenerated headers so that we didn't need to think hard if it's - // actually a MIG callback. - QualType T = FD->getReturnType(); - return T.getCanonicalType()->isIntegerType() && - T.getAsString() == "kern_return_t"; + return false; } void MIGChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const { - if (!isInMIGCall(C.getStackFrame())) + if (!isInMIGCall(C)) return; if (!Call.isGlobalCFunction()) @@ -105,7 +112,7 @@ if (!C.inTopFrame()) return; - if (!isInMIGCall(C.getStackFrame())) + if (!isInMIGCall(C)) return; // We know that the function is non-void, but what if the return statement Index: clang/test/Analysis/mig.mm =================================================================== --- clang/test/Analysis/mig.mm +++ clang/test/Analysis/mig.mm @@ -1,4 +1,5 @@ -// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG -verify %s +// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\ +// RUN: -fblocks -verify %s // XNU APIs. @@ -12,8 +13,12 @@ kern_return_t vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t); +#define MIG_SERVER_ROUTINE __attribute__((mig_server_routine)) + + // 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); if (size > 10) { @@ -22,6 +27,7 @@ return KERN_SUCCESS; } +MIG_SERVER_ROUTINE kern_return_t test_unknown_return_value(mach_port_name_t port, vm_address_t address, vm_size_t size) { extern kern_return_t foo(); @@ -31,6 +37,48 @@ } // Make sure we don't crash when they forgot to write the return statement. +MIG_SERVER_ROUTINE kern_return_t no_crash(mach_port_name_t port, vm_address_t address, vm_size_t size) { vm_deallocate(port, address, size); } + +// 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 + ofSize:(vm_size_t)size MIG_SERVER_ROUTINE { + vm_deallocate(port, address, size); + return KERN_ERROR; // expected-warning{{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}} +} +@end + +void test_block() { + kern_return_t (^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 KERN_ERROR; // expected-warning{{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}} + }; +} + +void test_block_with_weird_return_type() { + struct Empty {}; + + // The block is written within a function so that it was actually analyzed as + // a top-level function during analysis. If we were to write it as a global + // variable of block type instead, it would not have been analyzed, because + // ASTConsumer won't find the block's code body within the VarDecl. + // At the same time, we shouldn't call it from the function, because otherwise + // it will be analyzed as an inlined function rather than as a top-level + // 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 + }; +}