[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
diff --git a/clang/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp
new file mode 100644
index 0000000..36c5579
--- /dev/null
+++ b/clang/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<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);
+ }
+}