NoQ created this revision.
NoQ added a reviewer: dcoughlin.
Herald added subscribers: cfe-commits, jdoerfert, dkrupp, donat.nagy,
Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: clang.
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 -315 ("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
<https://reviews.llvm.org/D58365>.
Repository:
rC Clang
https://reviews.llvm.org/D58397
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
@@ -1,21 +1,60 @@
// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\
// RUN: -analyzer-output=text -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);
#define MIG_SERVER_ROUTINE __attribute__((mig_server_routine))
+ 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);
+
+
+// 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{{Deallocating object passed through parameter 'address'}}
@@ -69,3 +108,51 @@
}
return KERN_SUCCESS;
}
+
+// 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{{Deallocating object passed through parameter 'address'}}
+ 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{{Deallocating object passed through parameter 'port'}}
+ 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{{Deallocating object passed through parameter 'address'}}
+ 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
+IOReturn test_releaseAsyncReference64(IOExternalMethodArguments *arguments) {
+ IOUserClient::releaseAsyncReference64(arguments->asyncReference); // expected-note{{Deallocating object passed through parameter 'arguments'}}
+ 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{{Deallocating object passed through parameter 'arguments'}}
+ 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}}
+ }
+};
Index: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -36,7 +36,16 @@
BugType BT{this, "Use-after-free (MIG calling convention violation)",
categories::MemoryError};
- CallDescription vm_deallocate { "vm_deallocate", 3 };
+ std::vector<std::pair<CallDescription, unsigned>> Deallocators = {
+#define CALL(required_args, deallocated_arg, ...) \
+ {{{__VA_ARGS__}, required_args}, deallocated_arg}
+ CALL(3, 1, "vm_deallocate"),
+ CALL(3, 1, "mach_vm_deallocate"),
+ CALL(2, 0, "mig_deallocate"),
+ CALL(2, 1, "mach_port_deallocate"),
+ CALL(1, 0, "IOUserClient", "releaseAsyncReference64"),
+#undef CALL
+ };
void checkReturnAux(const ReturnStmt *RS, CheckerContext &C) const;
@@ -65,10 +74,23 @@
if (!Sym)
return nullptr;
- const auto *VR = dyn_cast_or_null<VarRegion>(Sym->getOriginRegion());
- if (VR && VR->hasStackParametersStorage() &&
- VR->getStackFrame()->inTopFrame())
- return cast<ParmVarDecl>(VR->getDecl());
+ // 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;
}
@@ -88,22 +110,35 @@
// 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.
- return FD->hasAttr<MIGServerRoutineAttr>() &&
- FD->getReturnType().getCanonicalType()->isSignedIntegerType();
+ if (!FD->getReturnType().getCanonicalType()->isSignedIntegerType())
+ return false;
+
+ if (FD->hasAttr<MIGServerRoutineAttr>())
+ return true;
+
+ // See if there's an annotated method in the superclass.
+ if (const auto *MD = dyn_cast<CXXMethodDecl>(FD))
+ 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.getStackFrame()))
return;
- if (!Call.isGlobalCFunction())
- return;
-
- if (!Call.isCalled(vm_deallocate))
+ 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;
// 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;
@@ -118,6 +153,26 @@
C.addTransition(C.getState()->set<ReleasedParameter>(true), T);
}
+static bool isSuccess(SVal V, CheckerContext &C) {
+ ProgramStateRef State = C.getState();
+
+ // Does V represent KERN_SUCCESS?
+ if (State->isNull(V).isConstrainedTrue())
+ return true;
+
+ SValBuilder &SVB = C.getSValBuilder();
+ ASTContext &ACtx = C.getASTContext();
+
+ // Does 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 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
@@ -143,7 +198,7 @@
return;
SVal V = C.getSVal(RS);
- if (!State->isNonNull(V).isConstrainedTrue())
+ if (isSuccess(V, C))
return;
ExplodedNode *N = C.generateErrorNode();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
