[PATCH] D58397: [analyzer] MIGChecker: Pour more data into the checker.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL354643: [analyzer] MIGChecker: Add support for more APIs.
(authored by dergachev, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D58397?vs=187876=187887#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58397/new/
https://reviews.llvm.org/D58397
Files:
cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
cfe/trunk/test/Analysis/mig.mm
Index: cfe/trunk/test/Analysis/mig.mm
===
--- cfe/trunk/test/Analysis/mig.mm
+++ cfe/trunk/test/Analysis/mig.mm
@@ -1,20 +1,55 @@
// 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.
@@ -123,3 +158,52 @@
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 {
+
+
[PATCH] D58397: [analyzer] MIGChecker: Pour more data into the checker.
NoQ updated this revision to Diff 187876.
NoQ added a comment.
Address comments.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58397/new/
https://reviews.llvm.org/D58397
Files:
clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
clang/test/Analysis/mig.mm
Index: clang/test/Analysis/mig.mm
===
--- clang/test/Analysis/mig.mm
+++ clang/test/Analysis/mig.mm
@@ -1,20 +1,55 @@
// 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.
@@ -123,3 +158,52 @@
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}}
+
[PATCH] D58397: [analyzer] MIGChecker: Pour more data into the checker.
NoQ marked an inline comment as done. NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:125 + // See if there's an annotated method in the superclass. + if (const auto *MD = dyn_cast(D)) dcoughlin wrote: > 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.. 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58397/new/ https://reviews.llvm.org/D58397 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58397: [analyzer] MIGChecker: Pour more data into the checker.
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
Aah, MIG_NO_REPLY.
LGTM.
Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:42
+ std::vector> Deallocators = {
+#define CALL(required_args, deallocated_arg, ...)
\
+ {{{__VA_ARGS__}, required_args}, deallocated_arg}
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.
Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:125
+ // See if there's an annotated method in the superclass.
+ if (const auto *MD = dyn_cast(D))
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..
Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:162
+// Returns true if V can potentially represent a "successful" kern_return_t.
+static bool isSuccess(SVal V, CheckerContext ) {
+ ProgramStateRef State = C.getState();
Could we rename this to "mayBeSuccess()" or something like that so that the
name of function indicate the "potentially" part of the comment.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58397/new/
https://reviews.llvm.org/D58397
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58397: [analyzer] MIGChecker: Pour more data into the checker.
NoQ updated this revision to Diff 187505.
NoQ added a comment.
Rebase. Fix behavior when the return code is not constrained enough. Test the
C++11 attribute syntax (just in case). Update comments.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58397/new/
https://reviews.llvm.org/D58397
Files:
clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
clang/test/Analysis/mig.mm
Index: clang/test/Analysis/mig.mm
===
--- clang/test/Analysis/mig.mm
+++ clang/test/Analysis/mig.mm
@@ -1,20 +1,55 @@
// 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.
@@ -123,3 +158,52 @@
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{{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}}
+}
+
+// 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{{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
[PATCH] D58397: [analyzer] MIGChecker: Pour more data into the checker.
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
.
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}}
+//
