[analyzer] Add NumberObjectConversion checker.

When dealing with objects that represent numbers, such as Objective-C NSNumber,
the language provides little protection from accidentally interpreting
the value of a pointer to such object as the value of the number represented
by the object. Results of such mis-interpretation may be unexpected.

The checker attempts to fill this gap in cases when the code is obviously
incorrect.

With "Pedantic" option enabled, this checker enforces a coding style to
completely prevent errors of this kind (off by default).

Differential Revision: https://reviews.llvm.org/D22968

llvm-svn: 284473
This commit is contained in:
Artem Dergachev 2016-10-18 11:06:28 +00:00
parent f0b4e5db16
commit 940c770d27
6 changed files with 431 additions and 0 deletions

View File

@ -474,6 +474,11 @@ def CStringNotNullTerm : Checker<"NotNullTerminated">,
let ParentPackage = OSX in {
def NumberObjectConversionChecker : Checker<"NumberObjectConversion">,
InPackage<OSX>,
HelpText<"Check for erroneous conversions of objects representing numbers into numbers">,
DescFile<"NumberObjectConversionChecker.cpp">;
def MacOSXAPIChecker : Checker<"API">,
InPackage<OSX>,
HelpText<"Check for proper uses of various Apple APIs">,

View File

@ -54,6 +54,7 @@ add_clang_library(clangStaticAnalyzerCheckers
NoReturnFunctionChecker.cpp
NonNullParamChecker.cpp
NullabilityChecker.cpp
NumberObjectConversionChecker.cpp
ObjCAtSyncChecker.cpp
ObjCContainersASTChecker.cpp
ObjCContainersChecker.cpp

View File

@ -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<check::ASTCodeBody> {
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<Stmt>("pedantic") != nullptr);
if (IsPedanticMatch && !C->Pedantic)
return;
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);
ASTContext &ACtx = ADC->getASTContext();
if (const Expr *CheckIfNull =
Result.Nodes.getNodeAs<Expr>("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<QualType>("int_type") != nullptr)
OS << "integer value";
else if (Result.Nodes.getNodeAs<QualType>("objc_bool_type") != nullptr)
OS << "BOOL value";
else if (Result.Nodes.getNodeAs<QualType>("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<NumberObjectConversionChecker>();
Chk->Pedantic =
Mgr.getAnalyzerOptions().getBooleanOption("Pedantic", false, Chk);
}
}

View File

@ -10,10 +10,16 @@ typedef unsigned short UInt16;
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 @@ NSFastEnumerationState;
@end
@interface NSNumber : NSValue - (char)charValue;
- (id)initWithInt:(int)value;
- (BOOL)boolValue;
@end @class NSString;
@interface NSArray : NSObject <NSCopying, NSMutableCopying, NSCoding, NSFastEnumeration> - (NSUInteger)count;
@end @interface NSArray (NSArrayCreation) + (id)array;

View File

@ -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
}

View File

@ -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
}