NoQ updated this revision to Diff 75739.
NoQ marked 2 inline comments as done.
NoQ added a comment.
Consider a lot more dispatch_once_t regions: improve diagnostics for local
structs containing predicates, find ivar structs with predicates.
Address a couple of review comments, discuss the rest.
https://reviews.llvm.org/D25909
Files:
lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
test/Analysis/dispatch-once.m
Index: test/Analysis/dispatch-once.m
===================================================================
--- /dev/null
+++ test/Analysis/dispatch-once.m
@@ -0,0 +1,82 @@
+// RUN: %clang_cc1 -w -fblocks -analyze -analyzer-checker=core,osx.API,unix.Malloc -verify %s
+// RUN: %clang_cc1 -w -fblocks -fobjc-arc -analyze -analyzer-checker=core,osx.API,unix.Malloc -verify %s
+
+#include "Inputs/system-header-simulator-objc.h"
+
+typedef unsigned long size_t;
+void *calloc(size_t nmemb, size_t size);
+
+typedef void (^dispatch_block_t)(void);
+typedef long dispatch_once_t;
+void dispatch_once(dispatch_once_t *predicate, dispatch_block_t block);
+
+void test_stack() {
+ dispatch_once_t once;
+ dispatch_once(&once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the local variable 'once' for the predicate value. Using such transient memory for the predicate is potentially dangerous. Perhaps you intended to declare the variable as 'static'?}}
+}
+
+void test_static_local() {
+ static dispatch_once_t once;
+ dispatch_once(&once, ^{}); // no-warning
+}
+
+void test_heap_var() {
+ dispatch_once_t *once = calloc(1, sizeof(dispatch_once_t));
+ // Use regexps to check that we're NOT suggesting to make this static.
+ dispatch_once(once, ^{}); // expected-warning-re{{{{^Call to 'dispatch_once' uses heap-allocated memory for the predicate value. Using such transient memory for the predicate is potentially dangerous$}}}}
+}
+
+void test_external_pointer(dispatch_once_t *once) {
+ // External pointer does not necessarily point to the heap.
+ dispatch_once(once, ^{}); // no-warning
+}
+
+typedef struct {
+ dispatch_once_t once;
+} Struct;
+
+void test_local_struct() {
+ Struct s;
+ dispatch_once(&s.once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the local variable 's' for the predicate value. Using such transient memory for the predicate is potentially dangerous. Perhaps you intended to declare the variable as 'static'?}}
+}
+
+void test_heap_struct() {
+ Struct *s = calloc(1, sizeof(Struct));
+ dispatch_once(&s->once, ^{}); // expected-warning{{Call to 'dispatch_once' uses heap-allocated memory for the predicate value.}}
+}
+
+@interface Object : NSObject {
+@public
+ dispatch_once_t once;
+ Struct s;
+}
+- (void)test_ivar_from_inside;
+- (void)test_ivar_struct_from_inside;
+@end
+
+@implementation Object
+- (void)test_ivar_from_inside {
+ dispatch_once(&once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the instance variable 'once' for the predicate value.}}
+}
+- (void)test_ivar_struct_from_inside {
+ dispatch_once(&s.once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the instance variable 's' for the predicate value.}}
+}
+@end
+
+void test_ivar_from_alloc_init() {
+ Object *o = [[Object alloc] init];
+ dispatch_once(&o->once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the instance variable 'once' for the predicate value.}}
+}
+void test_ivar_struct_from_alloc_init() {
+ Object *o = [[Object alloc] init];
+ dispatch_once(&o->s.once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the instance variable 's' for the predicate value.}}
+}
+
+void test_ivar_from_external_obj(Object *o) {
+ // ObjC object pointer always points to the heap.
+ dispatch_once(&o->once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the instance variable 'once' for the predicate value.}}
+}
+
+void test_ivar_struct_from_external_obj(Object *o) {
+ dispatch_once(&o->s.once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the instance variable 's' for the predicate value.}}
+}
Index: lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
@@ -33,6 +33,8 @@
class MacOSXAPIChecker : public Checker< check::PreStmt<CallExpr> > {
mutable std::unique_ptr<BugType> BT_dispatchOnce;
+ static const ObjCIvarRegion *getParentIvarRegion(const MemRegion *R);
+
public:
void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
@@ -49,20 +51,35 @@
// dispatch_once and dispatch_once_f
//===----------------------------------------------------------------------===//
+const ObjCIvarRegion *
+MacOSXAPIChecker::getParentIvarRegion(const MemRegion *R) {
+ const SubRegion *SR = dyn_cast<SubRegion>(R);
+ while (SR) {
+ if (const ObjCIvarRegion *IR = dyn_cast<ObjCIvarRegion>(SR))
+ return IR;
+ SR = dyn_cast<SubRegion>(SR->getSuperRegion());
+ }
+ return nullptr;
+}
+
void MacOSXAPIChecker::CheckDispatchOnce(CheckerContext &C, const CallExpr *CE,
StringRef FName) const {
if (CE->getNumArgs() < 1)
return;
// Check if the first argument is stack allocated. If so, issue a warning
// because that's likely to be bad news.
- ProgramStateRef state = C.getState();
- const MemRegion *R =
- state->getSVal(CE->getArg(0), C.getLocationContext()).getAsRegion();
- if (!R || !isa<StackSpaceRegion>(R->getMemorySpace()))
+ const MemRegion *R = C.getSVal(CE->getArg(0)).getAsRegion();
+ if (!R)
return;
- ExplodedNode *N = C.generateErrorNode(state);
+ // Global variables are fine.
+ const MemRegion *RB = R->getBaseRegion();
+ const MemSpaceRegion *RS = RB->getMemorySpace();
+ if (isa<GlobalsSpaceRegion>(RS))
+ return;
+
+ ExplodedNode *N = C.generateErrorNode();
if (!N)
return;
@@ -82,14 +99,26 @@
SmallString<256> S;
llvm::raw_svector_ostream os(S);
+ bool SuggestStatic = false;
os << "Call to '" << FName << "' uses";
- if (const VarRegion *VR = dyn_cast<VarRegion>(R))
+ if (const VarRegion *VR = dyn_cast<VarRegion>(RB)) {
+ // We filtered out globals earlier, so it must be a local variable.
os << " the local variable '" << VR->getDecl()->getName() << '\'';
- else
+ SuggestStatic = true;
+ } else if (const ObjCIvarRegion *IVR = getParentIvarRegion(R))
+ os << " the instance variable '" << IVR->getDecl()->getName() << '\'';
+ else if (isa<HeapSpaceRegion>(RS))
+ os << " heap-allocated memory";
+ else if (isa<UnknownSpaceRegion>(RS)) {
+ // FIXME: Presence of an IVar region has priority over this branch, because
+ // ObjC objects are on the heap even if the core doesn't realize this.
+ // Make core realize that all ObjC objects are on the heap.
+ return;
+ } else
os << " stack allocated memory";
os << " for the predicate value. Using such transient memory for "
"the predicate is potentially dangerous.";
- if (isa<VarRegion>(R) && isa<StackLocalsSpaceRegion>(R->getMemorySpace()))
+ if (SuggestStatic)
os << " Perhaps you intended to declare the variable as 'static'?";
auto report = llvm::make_unique<BugReport>(*BT_dispatchOnce, os.str(), N);
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
