NoQ created this revision.
NoQ added reviewers: zaks.anna, dcoughlin.
NoQ added a subscriber: cfe-commits.
If you store a boolean value as an Objective-C object `NSNumber *X`, and want
to see if it's true or false, than it's not a good idea to write this check as
'X == YES'. Because X is a pointer, and it is unlikely that it's equal to 1.
You perhaps wanted to say `[X boolValue]` instead.
Similarly, in XNU/driver code, if you have a C++ object `OSBoolean *X`, which
points to one of the two immutable objects - `kOSBooleanTrue` or
`kOSBooleanFalse` , it's not a good idea to convert `X` to a C++ bool directly
(eg. `bool B = X`). You want to compare it to `kOSBooleanTrue` or call the
`->isTrue()` method instead.
Bugs of both kinds have been noticed in real-world code, so i'm attempting to
make a simple checker for that. It is purely AST-based and moreover uses only
ASTMatchers for everything (which makes it the first analyzer checker to use
AST matchers). In fact i'm not sure if this checker should be in the analyzer
or in clang-tidy; there should be no problem to move it around.
I'm still going to tweak these matchers a bit, as i'm in the middle of
gathering statistics.
https://reviews.llvm.org/D22968
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp
lib/StaticAnalyzer/Checkers/CMakeLists.txt
test/Analysis/Inputs/system-header-simulator-objc.h
test/Analysis/bool-conversion.cpp
test/Analysis/bool-conversion.m
Index: test/Analysis/bool-conversion.m
===================================================================
--- /dev/null
+++ test/Analysis/bool-conversion.m
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -fblocks -w -analyze -analyzer-checker=alpha.osx.BoolConversion %s -verify
+// RUN: %clang_cc1 -fblocks -w -analyze -analyzer-checker=alpha.osx.BoolConversion -analyzer-config alpha.osx.BoolConversion:Pedantic=true -DPEDANTIC %s -verify
+
+#include "Inputs/system-header-simulator-objc.h"
+
+void bad(const NSNumber *p) {
+#ifdef PEDANTIC
+ if (p) {} // expected-warning{{}}
+ if (!p) {} // expected-warning{{}}
+ if (p == YES) {} // expected-warning{{}}
+ if (p == NO) {} // expected-warning{{}}
+ (!p) ? 1 : 2; // expected-warning{{}}
+ (BOOL)p; // expected-warning{{}}
+#endif
+ BOOL x = p; // expected-warning{{}}
+ x = p; // expected-warning{{}}
+ x = (p == YES); // expected-warning{{}}
+}
+
+typedef const NSNumber *SugaredNumber;
+void bad_sugared(SugaredNumber p) {
+ p == YES; // expected-warning{{}}
+}
+
+void good(const NSNumber *p) {
+ if ([p boolValue] == NO) {} // no-warning
+ if ([p boolValue] == YES) {} // no-warning
+ BOOL x = [p boolValue]; // no-warning
+}
+
+void suppression(const NSNumber *p) {
+ if (p == NULL) {} // no-warning
+ if (p == nil) {} // no-warning
+}
Index: test/Analysis/bool-conversion.cpp
===================================================================
--- /dev/null
+++ test/Analysis/bool-conversion.cpp
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -w -std=c++11 -analyze -analyzer-checker=alpha.osx.BoolConversion %s -verify
+// RUN: %clang_cc1 -w -std=c++11 -analyze -analyzer-checker=alpha.osx.BoolConversion -analyzer-config alpha.osx.BoolConversion:Pedantic=true -DPEDANTIC %s -verify
+
+#define NULL ((void *)0)
+#include "Inputs/system-header-simulator-cxx.h" // for nullptr
+
+class OSBoolean {
+public:
+ virtual bool isTrue() const;
+ virtual bool isFalse() const;
+};
+
+extern const OSBoolean *const &kOSBooleanFalse;
+extern const OSBoolean *const &kOSBooleanTrue;
+
+void bad(const OSBoolean *p) {
+#ifdef PEDANTIC
+ if (p) {} // expected-warning{{}}
+ if (!p) {} // expected-warning{{}}
+ p ? 1 : 2; // expected-warning{{}}
+ (bool)p; // expected-warning{{}}
+#endif
+ bool x = p; // expected-warning{{}}
+ x = p; // expected-warning{{}}
+}
+
+typedef bool sugared_bool;
+typedef const OSBoolean *sugared_OSBoolean;
+void bad_sugared(sugared_OSBoolean p) {
+ sugared_bool x = p; // expected-warning{{}}
+}
+
+void good(const OSBoolean *p) {
+ bool x = p->isTrue(); // no-warning
+ (bool)p->isFalse(); // no-warning
+ if (p == kOSBooleanTrue) {} // no-warning
+}
+
+void suppression(const OSBoolean *p) {
+ if (p == NULL) {} // no-warning
+ bool y = (p == nullptr); // no-warning
+}
Index: test/Analysis/Inputs/system-header-simulator-objc.h
===================================================================
--- test/Analysis/Inputs/system-header-simulator-objc.h
+++ test/Analysis/Inputs/system-header-simulator-objc.h
@@ -10,10 +10,16 @@
typedef signed long CFIndex;
typedef signed char BOOL;
+#define YES ((BOOL)1)
+#define NO ((BOOL)0)
+
typedef unsigned long NSUInteger;
typedef unsigned short unichar;
typedef UInt16 UniChar;
+#define NULL ((void *)0)
+#define nil ((id)0)
+
enum {
NSASCIIStringEncoding = 1,
NSNEXTSTEPStringEncoding = 2,
Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt
===================================================================
--- lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -9,6 +9,7 @@
ArrayBoundCheckerV2.cpp
BasicObjCFoundationChecks.cpp
BoolAssignmentChecker.cpp
+ BoolConversionChecker.cpp
BuiltinFunctionChecker.cpp
CStringChecker.cpp
CStringSyntaxChecker.cpp
@@ -87,6 +88,7 @@
LINK_LIBS
clangAST
+ clangASTMatchers
clangAnalysis
clangBasic
clangLex
Index: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp
===================================================================
--- /dev/null
+++ lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp
@@ -0,0 +1,165 @@
+//===- BoolConversionChecker.cpp ---------------------------------*- C++ -*-==//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines BoolConversionChecker, which checks for a particular
+// common mistake when dealing with NSNumber and OSBoolean objects:
+// When having a pointer P to such object, neither `if (P)' nor `bool X = P'
+// is the correct way of converting such object to a boolean value - instead,
+// both methods convert the pointer value to a boolean value, which evaluates
+// to "true" for any valid pointer.
+//
+// If the code intentionally tries to figure out if the pointer is null,
+// the suppression idiom would be `(P == NULL)' or `(P == nullptr)' (in C++11)
+// or `(P == nil)' (in Objective-C).
+//
+// This checker has an option "Pedantic" (boolean), which enables detection of
+// more conversion patterns (which are most likely more harmless, and therefore
+// are more likely to produce false positives) - disabled by default,
+// enable with `-analyzer-config osx.BoolConversion:Pedantic=true'.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ClangSACheckers.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
+
+using namespace clang;
+using namespace ento;
+using namespace ast_matchers;
+
+namespace {
+class Callback : public MatchFinder::MatchCallback {
+ const CheckerBase *C;
+ BugReporter &BR;
+ AnalysisDeclContext *ADC;
+
+public:
+ Callback(const CheckerBase *C, BugReporter &BR, AnalysisDeclContext *ADC)
+ : C(C), BR(BR), ADC(ADC) {}
+ virtual void run(const MatchFinder::MatchResult &Result) {
+ // This callback only throws reports. All the logic is in the matchers.
+ const Stmt *Conv = Result.Nodes.getNodeAs<Stmt>("conv");
+ assert(Conv);
+ const Expr *Osboolean = Result.Nodes.getNodeAs<Expr>("osboolean");
+ const Expr *Nsnumber = Result.Nodes.getNodeAs<Expr>("nsnumber");
+ bool IsObjC = (bool)Nsnumber;
+ const Expr *Obj = IsObjC ? Nsnumber : Osboolean;
+ assert(Obj);
+
+ llvm::SmallString<64> Msg;
+ llvm::raw_svector_ostream OS(Msg);
+ OS << "Converting '" << Obj->getType().getCanonicalType().getAsString()
+ << "' to a plain boolean value: probably a forgotten "
+ << (IsObjC ? "'[boolValue]'" : "'->isTrue()'");
+ BR.EmitBasicReport(
+ ADC->getDecl(), C, "Suspicious boolean conversion", "Logic error",
+ OS.str(),
+ PathDiagnosticLocation::createBegin(Obj, BR.getSourceManager(), ADC),
+ Conv->getSourceRange());
+ }
+};
+
+class BoolConversionChecker : public Checker<check::ASTCodeBody> {
+public:
+ bool Pedantic;
+
+ void checkASTCodeBody(const Decl *D, AnalysisManager &AM,
+ BugReporter &BR) const {
+ MatchFinder F;
+ Callback CB(this, BR, AM.getAnalysisDeclContext(D));
+
+ auto OSBooleanExprM =
+ expr(ignoringParenImpCasts(
+ expr(hasType(hasCanonicalType(
+ pointerType(pointee(hasCanonicalType(
+ recordType(hasDeclaration(
+ cxxRecordDecl(hasName(
+ "OSBoolean")))))))))).bind("osboolean")));
+
+ auto NSNumberExprM =
+ expr(ignoringParenImpCasts(expr(hasType(hasCanonicalType(
+ objcObjectPointerType(pointee(
+ qualType(hasCanonicalType(
+ qualType(hasDeclaration(
+ objcInterfaceDecl(hasName(
+ "NSNumber"))))))))))).bind("nsnumber")));
+
+ auto SuspiciousExprM =
+ anyOf(OSBooleanExprM, NSNumberExprM);
+
+ auto AnotherNSNumberExprM =
+ expr(equalsBoundNode("nsnumber"));
+
+ auto ObjCBooleanTypeM =
+ typedefType(hasDeclaration(typedefDecl(hasName("BOOL"))));
+
+ auto AnyBooleanTypeM =
+ qualType(anyOf(booleanType(), ObjCBooleanTypeM));
+
+ auto ConversionThroughAssignmentM =
+ binaryOperator(hasOperatorName("="),
+ hasLHS(hasType(AnyBooleanTypeM)),
+ hasRHS(SuspiciousExprM));
+
+ auto ConversionThroughBranchingM =
+ ifStmt(hasCondition(SuspiciousExprM));
+
+ auto ConversionThroughComparisonM =
+ binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+ hasEitherOperand(NSNumberExprM),
+ hasEitherOperand(ignoringParenImpCasts(
+ hasType(ObjCBooleanTypeM))));
+
+ auto ConversionThroughConditionalOperatorM =
+ conditionalOperator(
+ hasCondition(SuspiciousExprM),
+ unless(hasTrueExpression(hasDescendant(AnotherNSNumberExprM))),
+ unless(hasFalseExpression(hasDescendant(AnotherNSNumberExprM))));
+
+ auto ConversionThroughExclamationMarkM =
+ unaryOperator(hasOperatorName("!"), has(expr(SuspiciousExprM)));
+
+ auto ConversionThroughExplicitCastM =
+ explicitCastExpr(hasType(AnyBooleanTypeM),
+ has(expr(SuspiciousExprM)));
+
+ auto ConversionThroughInitializerM =
+ declStmt(hasSingleDecl(
+ varDecl(hasType(AnyBooleanTypeM),
+ hasInitializer(SuspiciousExprM))));
+
+ auto FinalM = Pedantic
+ ? stmt(anyOf(ConversionThroughAssignmentM,
+ ConversionThroughBranchingM,
+ ConversionThroughComparisonM,
+ ConversionThroughConditionalOperatorM,
+ ConversionThroughExclamationMarkM,
+ ConversionThroughExplicitCastM,
+ ConversionThroughInitializerM)).bind("conv")
+ : stmt(anyOf(ConversionThroughAssignmentM,
+ ConversionThroughComparisonM,
+ ConversionThroughInitializerM)).bind("conv");
+
+ F.addMatcher(stmt(forEachDescendant(FinalM)), &CB);
+ F.match(*D->getBody(), AM.getASTContext());
+ }
+};
+}
+
+void ento::registerBoolConversionChecker(CheckerManager &Mgr) {
+ const LangOptions &LO = Mgr.getLangOpts();
+ if (LO.CPlusPlus || LO.ObjC2) {
+ BoolConversionChecker *Chk = Mgr.registerChecker<BoolConversionChecker>();
+ Chk->Pedantic =
+ Mgr.getAnalyzerOptions().getBooleanOption("Pedantic", false, Chk);
+ }
+}
Index: include/clang/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -454,6 +454,15 @@
} // end "osx"
+let ParentPackage = OSXAlpha in {
+
+def BoolConversionChecker : Checker<"BoolConversion">,
+ InPackage<OSX>,
+ HelpText<"Check for erroneous conversions of number pointers into booleans">,
+ DescFile<"BoolConversionChecker">;
+
+} // end "alpha.osx"
+
let ParentPackage = Cocoa in {
def ObjCAtSyncChecker : Checker<"AtSync">,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
