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, + HelpText<"Check for erroneous conversions of number pointers into booleans">, + DescFile<"BoolConversionChecker">; + +} // end "alpha.osx" + let ParentPackage = Cocoa in { def ObjCAtSyncChecker : Checker<"AtSync">, 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("conv"); + assert(Conv); + const Expr *Osboolean = Result.Nodes.getNodeAs("osboolean"); + const Expr *Nsnumber = Result.Nodes.getNodeAs("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 { +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(); + Chk->Pedantic = + Mgr.getAnalyzerOptions().getBooleanOption("Pedantic", false, Chk); + } +} 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: 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: 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/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 +}