Index: include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- include/clang/StaticAnalyzer/Checkers/Checkers.td +++ include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -474,6 +474,11 @@ let ParentPackage = OSX in { +def NumberObjectConversionChecker : Checker<"NumberObjectConversion">, + InPackage, + HelpText<"Check for erroneous conversions of number pointers into numbers">, + DescFile<"NumberObjectConversionChecker">; + def MacOSXAPIChecker : Checker<"API">, InPackage, HelpText<"Check for proper uses of various Apple APIs">, Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt =================================================================== --- lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -54,6 +54,7 @@ NoReturnFunctionChecker.cpp NonNullParamChecker.cpp NullabilityChecker.cpp + NumberObjectConversionChecker.cpp ObjCAtSyncChecker.cpp ObjCContainersASTChecker.cpp ObjCContainersChecker.cpp Index: lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp =================================================================== --- /dev/null +++ lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp @@ -0,0 +1,267 @@ +//===- NumberObjectConversionChecker.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 NumberObjectConversionChecker, which checks for a +// particular common mistake when dealing with numbers represented as objects +// passed around by pointers. Namely, the language allows to reinterpret the +// pointer as a number directly, often without throwing any warnings, +// but in most cases the result of such conversion is clearly unexpected, +// as pointer value, rather than number value represented by the pointee object, +// becomes the result of such operation. +// +// Currently the checker supports the Objective-C NSNumber class, +// and the OSBoolean class found in macOS low-level code; the latter +// can only hold boolean values. +// +// 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, +// enabled with `-analyzer-config osx.NumberObjectConversion: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" +#include "clang/Lex/Lexer.h" +#include "llvm/ADT/APSInt.h" + +using namespace clang; +using namespace ento; +using namespace ast_matchers; + +namespace { + +class NumberObjectConversionChecker : public Checker { +public: + bool Pedantic; + + void checkASTCodeBody(const Decl *D, AnalysisManager &AM, + BugReporter &BR) const; +}; + +class Callback : public MatchFinder::MatchCallback { + const NumberObjectConversionChecker *C; + BugReporter &BR; + AnalysisDeclContext *ADC; + +public: + Callback(const NumberObjectConversionChecker *C, + BugReporter &BR, AnalysisDeclContext *ADC) + : C(C), BR(BR), ADC(ADC) {} + virtual void run(const MatchFinder::MatchResult &Result); +}; +} // end of anonymous namespace + +void Callback::run(const MatchFinder::MatchResult &Result) { + bool IsPedanticMatch = (Result.Nodes.getNodeAs("pedantic") != nullptr); + if (IsPedanticMatch && !C->Pedantic) + return; + + 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); + + ASTContext &ACtx = ADC->getASTContext(); + + if (const Expr *CheckIfNull = + Result.Nodes.getNodeAs("check_if_null")) { + // We consider NULL to be a pointer, even if it is defined as a plain 0. + // FIXME: Introduce a matcher to implement this logic? + SourceLocation Loc = CheckIfNull->getLocStart(); + if (Loc.isMacroID()) { + StringRef MacroName = Lexer::getImmediateMacroName( + Loc, ACtx.getSourceManager(), ACtx.getLangOpts()); + if (MacroName != "YES" && MacroName != "NO") + return; + } else { + // Otherwise, comparison of pointers to 0 might still be intentional. + // See if this is the case. + llvm::APSInt Result; + if (CheckIfNull->IgnoreParenCasts()->EvaluateAsInt( + Result, ACtx, Expr::SE_AllowSideEffects)) { + if (Result == 0) { + if (!C->Pedantic) + return; + IsPedanticMatch = true; + } + } + } + } + + llvm::SmallString<64> Msg; + llvm::raw_svector_ostream OS(Msg); + OS << "Converting '" + << Obj->getType().getCanonicalType().getUnqualifiedType().getAsString() + << "' to a plain "; + + if (Result.Nodes.getNodeAs("int_type") != nullptr) + OS << "integer value"; + else if (Result.Nodes.getNodeAs("objc_bool_type") != nullptr) + OS << "BOOL value"; + else if (Result.Nodes.getNodeAs("cpp_bool_type") != nullptr) + OS << "bool value"; + else + OS << "boolean value for branching"; + + if (IsPedanticMatch) { + if (IsObjC) { + OS << "; please compare the pointer to nil instead " + "to suppress this warning"; + } else { + OS << "; please compare the pointer to NULL or nullptr instead " + "to suppress this warning"; + } + } else { + OS << "; pointer value is being used instead"; + } + + BR.EmitBasicReport( + ADC->getDecl(), C, "Suspicious number object conversion", "Logic error", + OS.str(), + PathDiagnosticLocation::createBegin(Obj, BR.getSourceManager(), ADC), + Conv->getSourceRange()); +} + +void NumberObjectConversionChecker::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")); + + // The .bind here is in order to compose the error message more accurately. + auto ObjCBooleanTypeM = + qualType(typedefType(hasDeclaration( + typedefDecl(hasName("BOOL"))))).bind("objc_bool_type"); + + // The .bind here is in order to compose the error message more accurately. + auto AnyBooleanTypeM = + qualType(anyOf(qualType(booleanType()).bind("cpp_bool_type"), + ObjCBooleanTypeM)); + + + // The .bind here is in order to compose the error message more accurately. + auto AnyNumberTypeM = + qualType(hasCanonicalType(isInteger()), + unless(typedefType(hasDeclaration( + typedefDecl(matchesName("^::u?intptr_t$")))))) + .bind("int_type"); + + auto AnyBooleanOrNumberTypeM = + qualType(anyOf(AnyBooleanTypeM, AnyNumberTypeM)); + + auto AnyBooleanOrNumberExprM = + expr(ignoringParenImpCasts(expr(hasType(AnyBooleanOrNumberTypeM)))); + + auto ConversionThroughAssignmentM = + binaryOperator(hasOperatorName("="), + hasLHS(AnyBooleanOrNumberExprM), + hasRHS(SuspiciousExprM)); + + auto ConversionThroughBranchingM = + ifStmt(hasCondition(SuspiciousExprM)) + .bind("pedantic"); + + auto ConversionThroughCallM = + callExpr(hasAnyArgument(allOf(hasType(AnyBooleanOrNumberTypeM), + ignoringParenImpCasts(SuspiciousExprM)))); + + // We bind "check_if_null" to modify the warning message + // in case it was intended to compare a pointer to 0 with a relatively-ok + // construct "x == 0" or "x != 0". + auto ConversionThroughEquivalenceM = + binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")), + hasEitherOperand(SuspiciousExprM), + hasEitherOperand(AnyBooleanOrNumberExprM + .bind("check_if_null"))); + + auto ConversionThroughComparisonM = + binaryOperator(anyOf(hasOperatorName(">="), hasOperatorName(">"), + hasOperatorName("<="), hasOperatorName("<")), + hasEitherOperand(SuspiciousExprM), + hasEitherOperand(AnyBooleanOrNumberExprM)); + + auto ConversionThroughConditionalOperatorM = + conditionalOperator( + hasCondition(SuspiciousExprM), + unless(hasTrueExpression(hasDescendant(AnotherNSNumberExprM))), + unless(hasFalseExpression(hasDescendant(AnotherNSNumberExprM)))) + .bind("pedantic"); + + auto ConversionThroughExclamationMarkM = + unaryOperator(hasOperatorName("!"), has(expr(SuspiciousExprM))) + .bind("pedantic"); + + auto ConversionThroughExplicitBooleanCastM = + explicitCastExpr(hasType(AnyBooleanTypeM), + has(expr(SuspiciousExprM))) + .bind("pedantic"); + + auto ConversionThroughExplicitNumberCastM = + explicitCastExpr(hasType(AnyNumberTypeM), + has(expr(SuspiciousExprM))); + + auto ConversionThroughInitializerM = + declStmt(hasSingleDecl( + varDecl(hasType(AnyBooleanOrNumberTypeM), + hasInitializer(SuspiciousExprM)))); + + auto FinalM = stmt(anyOf(ConversionThroughAssignmentM, + ConversionThroughBranchingM, + ConversionThroughCallM, + ConversionThroughComparisonM, + ConversionThroughConditionalOperatorM, + ConversionThroughEquivalenceM, + ConversionThroughExclamationMarkM, + ConversionThroughExplicitBooleanCastM, + ConversionThroughExplicitNumberCastM, + ConversionThroughInitializerM)).bind("conv"); + + F.addMatcher(stmt(forEachDescendant(FinalM)), &CB); + F.match(*D->getBody(), AM.getASTContext()); +} + +void ento::registerNumberObjectConversionChecker(CheckerManager &Mgr) { + const LangOptions &LO = Mgr.getLangOpts(); + if (LO.CPlusPlus || LO.ObjC2) { + NumberObjectConversionChecker *Chk = + Mgr.registerChecker(); + Chk->Pedantic = + Mgr.getAnalyzerOptions().getBooleanOption("Pedantic", false, Chk); + } +} 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, @@ -72,6 +78,7 @@ @end @interface NSNumber : NSValue - (char)charValue; - (id)initWithInt:(int)value; +- (BOOL)boolValue; @end @class NSString; @interface NSArray : NSObject - (NSUInteger)count; @end @interface NSArray (NSArrayCreation) + (id)array; Index: test/Analysis/number-object-conversion.cpp =================================================================== --- /dev/null +++ test/Analysis/number-object-conversion.cpp @@ -0,0 +1,65 @@ +// RUN: %clang_cc1 -triple i386-apple-darwin10 -w -std=c++11 -analyze -analyzer-checker=osx.NumberObjectConversion %s -verify +// RUN: %clang_cc1 -triple i386-apple-darwin10 -w -std=c++11 -analyze -analyzer-checker=osx.NumberObjectConversion -analyzer-config osx.NumberObjectConversion: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 takes_bool(bool); + +void bad(const OSBoolean *p) { +#ifdef PEDANTIC + if (p) {} // expected-warning{{Converting 'const class OSBoolean *' to a plain boolean value for branching; please compare the pointer to NULL or nullptr instead to suppress this warning}} + if (!p) {} // expected-warning{{Converting 'const class OSBoolean *' to a plain boolean value for branching; please compare the pointer to NULL or nullptr instead to suppress this warning}} + p ? 1 : 2; // expected-warning{{Converting 'const class OSBoolean *' to a plain boolean value for branching; please compare the pointer to NULL or nullptr instead to suppress this warning}} + (bool)p; // expected-warning{{Converting 'const class OSBoolean *' to a plain bool value; please compare the pointer to NULL or nullptr instead to suppress this warning}} +#endif + bool x = p; // expected-warning{{Converting 'const class OSBoolean *' to a plain bool value; pointer value is being used instead}} + x = p; // expected-warning{{Converting 'const class OSBoolean *' to a plain bool value; pointer value is being used instead}} + takes_bool(p); // expected-warning{{Converting 'const class OSBoolean *' to a plain bool value; pointer value is being used instead}} + takes_bool(x); // no-warning +} + +typedef bool sugared_bool; +typedef const OSBoolean *sugared_OSBoolean; +void bad_sugared(sugared_OSBoolean p) { + sugared_bool x = p; // expected-warning{{Converting 'const class OSBoolean *' to a plain bool value; pointer value is being used instead}} +} + +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 +} + +// Conversion of a pointer to an intptr_t is fine. +typedef long intptr_t; +typedef unsigned long uintptr_t; +typedef long fintptr_t; // Fake, for testing the regex. +void test_intptr_t(const OSBoolean *p) { + (long)p; // expected-warning{{Converting 'const class OSBoolean *' to a plain integer value; pointer value is being used instead}} + (intptr_t)p; // no-warning + (unsigned long)p; // expected-warning{{Converting 'const class OSBoolean *' to a plain integer value; pointer value is being used instead}} + (uintptr_t)p; // no-warning + (fintptr_t)p; // expected-warning{{Converting 'const class OSBoolean *' to a plain integer value; pointer value is being used instead}} +} + +// Test a different definition of NULL. +#undef NULL +#define NULL 0 +void test_non_pointer_NULL(const OSBoolean *p) { + if (p == NULL) {} // no-warning +} Index: test/Analysis/number-object-conversion.m =================================================================== --- /dev/null +++ test/Analysis/number-object-conversion.m @@ -0,0 +1,86 @@ +// RUN: %clang_cc1 -triple i386-apple-darwin10 -fblocks -w -analyze -analyzer-checker=osx.NumberObjectConversion %s -verify +// RUN: %clang_cc1 -triple i386-apple-darwin10 -fblocks -w -analyze -analyzer-checker=osx.NumberObjectConversion -analyzer-config osx.NumberObjectConversion:Pedantic=true -DPEDANTIC %s -verify +// RUN: %clang_cc1 -triple i386-apple-darwin10 -fblocks -fobjc-arc -w -analyze -analyzer-checker=osx.NumberObjectConversion %s -verify +// RUN: %clang_cc1 -triple i386-apple-darwin10 -fblocks -fobjc-arc -w -analyze -analyzer-checker=osx.NumberObjectConversion -analyzer-config osx.NumberObjectConversion:Pedantic=true -DPEDANTIC %s -verify + +#include "Inputs/system-header-simulator-objc.h" + +void takes_boolean(BOOL); +void takes_integer(int); + +void bad(NSNumber *p) { +#ifdef PEDANTIC + if (p) {} // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}} + if (!p) {} // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}} + (!p) ? 1 : 2; // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}} + (BOOL)p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; please compare the pointer to nil instead to suppress this warning}} + if (p == 0) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; please compare the pointer to nil instead to suppress this warning}} + if (p > 0) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}} +#endif + if (p == YES) {} // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}} + if (p == NO) {} // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}} + BOOL x = p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}} + x = p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}} + x = (p == YES); // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}} + if (p == 1) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}} + int y = p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}} + y = p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}} + takes_boolean(p); // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}} + takes_integer(p); // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}} + takes_boolean(x); // no-warning + takes_integer(y); // no-warning +} + +typedef NSNumber *SugaredNumber; +void bad_sugared(SugaredNumber p) { + p == YES; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}} +} + +@interface I : NSObject { +@public + NSNumber *ivar; + NSNumber *prop; +} +- (NSNumber *)foo; +@property(copy) NSNumber *prop; +@end + +@implementation I +@synthesize prop; +@end + +void bad_ivar(I *i) { + i->ivar == YES; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}} + i->prop == YES; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}} + [i foo] == YES; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}} +} + +void good(NSNumber *p) { + if ([p boolValue] == NO) {} // no-warning + if ([p boolValue] == YES) {} // no-warning + BOOL x = [p boolValue]; // no-warning +} + +void suppression(NSNumber *p) { + if (p == NULL) {} // no-warning + if (p == nil) {} // no-warning +} + +// Conversion of a pointer to an intptr_t is fine. +typedef long intptr_t; +typedef unsigned long uintptr_t; +typedef long fintptr_t; // Fake, for testing the regex. +void test_intptr_t(NSNumber *p) { + (long)p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}} + (intptr_t)p; // no-warning + (unsigned long)p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}} + (uintptr_t)p; // no-warning + (fintptr_t)p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}} +} + +// Test a different definition of NULL. +#undef NULL +#define NULL 0 +void test_non_pointer_NULL(NSNumber *p) { + if (p == NULL) {} // no-warning +}