//===- NumberObjectConversionChecker.cpp -------------------------*- C++ -*-==// // // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // See https://llvm.org/LICENSE.txt for license information. // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // //===----------------------------------------------------------------------===// // // 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 "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.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; ASTContext &ACtx = ADC->getASTContext(); if (const Expr *CheckIfNull = Result.Nodes.getNodeAs("check_if_null")) { // Unless the macro indicates that the intended type is clearly not // a pointer type, we should avoid warning on comparing pointers // to zero literals in non-pedantic mode. // FIXME: Introduce an AST matcher to implement the macro-related logic? bool MacroIndicatesWeShouldSkipTheCheck = false; SourceLocation Loc = CheckIfNull->getBeginLoc(); if (Loc.isMacroID()) { StringRef MacroName = Lexer::getImmediateMacroName( Loc, ACtx.getSourceManager(), ACtx.getLangOpts()); if (MacroName == "NULL" || MacroName == "nil") return; if (MacroName == "YES" || MacroName == "NO") MacroIndicatesWeShouldSkipTheCheck = true; } if (!MacroIndicatesWeShouldSkipTheCheck) { Expr::EvalResult EVResult; if (CheckIfNull->IgnoreParenCasts()->EvaluateAsInt( EVResult, ACtx, Expr::SE_AllowSideEffects)) { llvm::APSInt Result = EVResult.Val.getInt(); if (Result == 0) { if (!C->Pedantic) return; IsPedanticMatch = true; } } } } const Stmt *Conv = Result.Nodes.getNodeAs("conv"); assert(Conv); const Expr *ConvertedCObject = Result.Nodes.getNodeAs("c_object"); const Expr *ConvertedCppObject = Result.Nodes.getNodeAs("cpp_object"); const Expr *ConvertedObjCObject = Result.Nodes.getNodeAs("objc_object"); bool IsCpp = (ConvertedCppObject != nullptr); bool IsObjC = (ConvertedObjCObject != nullptr); const Expr *Obj = IsObjC ? ConvertedObjCObject : IsCpp ? ConvertedCppObject : ConvertedCObject; assert(Obj); bool IsComparison = (Result.Nodes.getNodeAs("comparison") != nullptr); bool IsOSNumber = (Result.Nodes.getNodeAs("osnumber") != nullptr); bool IsInteger = (Result.Nodes.getNodeAs("int_type") != nullptr); bool IsObjCBool = (Result.Nodes.getNodeAs("objc_bool_type") != nullptr); bool IsCppBool = (Result.Nodes.getNodeAs("cpp_bool_type") != nullptr); llvm::SmallString<64> Msg; llvm::raw_svector_ostream OS(Msg); // Remove ObjC ARC qualifiers. QualType ObjT = Obj->getType().getUnqualifiedType(); // Remove consts from pointers. if (IsCpp) { assert(ObjT.getCanonicalType()->isPointerType()); ObjT = ACtx.getPointerType( ObjT->getPointeeType().getCanonicalType().getUnqualifiedType()); } if (IsComparison) OS << "Comparing "; else OS << "Converting "; OS << "a pointer value of type '" << ObjT.getAsString() << "' to a "; std::string EuphemismForPlain = "primitive"; std::string SuggestedApi = IsObjC ? (IsInteger ? "" : "-boolValue") : IsCpp ? (IsOSNumber ? "" : "getValue()") : "CFNumberGetValue()"; if (SuggestedApi.empty()) { // A generic message if we're not sure what API should be called. // FIXME: Pattern-match the integer type to make a better guess? SuggestedApi = "a method on '" + ObjT.getAsString() + "' to get the scalar value"; // "scalar" is not quite correct or common, but some documentation uses it // when describing object methods we suggest. For consistency, we use // "scalar" in the whole sentence when we need to use this word in at least // one place, otherwise we use "primitive". EuphemismForPlain = "scalar"; } if (IsInteger) OS << EuphemismForPlain << " integer value"; else if (IsObjCBool) OS << EuphemismForPlain << " BOOL value"; else if (IsCppBool) OS << EuphemismForPlain << " bool value"; else // Branch condition? OS << EuphemismForPlain << " boolean value"; if (IsPedanticMatch) OS << "; instead, either compare the pointer to " << (IsObjC ? "nil" : IsCpp ? "nullptr" : "NULL") << " or "; else OS << "; did you mean to "; if (IsComparison) OS << "compare the result of calling " << SuggestedApi; else OS << "call " << SuggestedApi; if (!IsPedanticMatch) OS << "?"; 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 { // Currently this matches CoreFoundation opaque pointer typedefs. auto CSuspiciousNumberObjectExprM = expr(ignoringParenImpCasts( expr(hasType( typedefType(hasDeclaration(anyOf( typedefDecl(hasName("CFNumberRef")), typedefDecl(hasName("CFBooleanRef"))))))) .bind("c_object"))); // Currently this matches XNU kernel number-object pointers. auto CppSuspiciousNumberObjectExprM = expr(ignoringParenImpCasts( expr(hasType(hasCanonicalType( pointerType(pointee(hasCanonicalType( recordType(hasDeclaration( anyOf( cxxRecordDecl(hasName("OSBoolean")), cxxRecordDecl(hasName("OSNumber")) .bind("osnumber")))))))))) .bind("cpp_object"))); // Currently this matches NeXTSTEP number objects. auto ObjCSuspiciousNumberObjectExprM = expr(ignoringParenImpCasts( expr(hasType(hasCanonicalType( objcObjectPointerType(pointee( qualType(hasCanonicalType( qualType(hasDeclaration( objcInterfaceDecl(hasName("NSNumber"))))))))))) .bind("objc_object"))); auto SuspiciousNumberObjectExprM = anyOf( CSuspiciousNumberObjectExprM, CppSuspiciousNumberObjectExprM, ObjCSuspiciousNumberObjectExprM); // Useful for predicates like "Unless we've seen the same object elsewhere". auto AnotherSuspiciousNumberObjectExprM = expr(anyOf( equalsBoundNode("c_object"), equalsBoundNode("objc_object"), equalsBoundNode("cpp_object"))); // The .bind here is in order to compose the error message more accurately. auto ObjCSuspiciousScalarBooleanTypeM = qualType(typedefType(hasDeclaration( typedefDecl(hasName("BOOL"))))).bind("objc_bool_type"); // The .bind here is in order to compose the error message more accurately. auto SuspiciousScalarBooleanTypeM = qualType(anyOf(qualType(booleanType()).bind("cpp_bool_type"), ObjCSuspiciousScalarBooleanTypeM)); // The .bind here is in order to compose the error message more accurately. // Also avoid intptr_t and uintptr_t because they were specifically created // for storing pointers. auto SuspiciousScalarNumberTypeM = qualType(hasCanonicalType(isInteger()), unless(typedefType(hasDeclaration( typedefDecl(matchesName("^::u?intptr_t$")))))) .bind("int_type"); auto SuspiciousScalarTypeM = qualType(anyOf(SuspiciousScalarBooleanTypeM, SuspiciousScalarNumberTypeM)); auto SuspiciousScalarExprM = expr(ignoringParenImpCasts(expr(hasType(SuspiciousScalarTypeM)))); auto ConversionThroughAssignmentM = binaryOperator(allOf(hasOperatorName("="), hasLHS(SuspiciousScalarExprM), hasRHS(SuspiciousNumberObjectExprM))); auto ConversionThroughBranchingM = ifStmt(allOf( hasCondition(SuspiciousNumberObjectExprM), unless(hasConditionVariableStatement(declStmt()) ))).bind("pedantic"); auto ConversionThroughCallM = callExpr(hasAnyArgument(allOf(hasType(SuspiciousScalarTypeM), ignoringParenImpCasts( SuspiciousNumberObjectExprM)))); // 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(allOf(anyOf(hasOperatorName("=="), hasOperatorName("!=")), hasEitherOperand(SuspiciousNumberObjectExprM), hasEitherOperand(SuspiciousScalarExprM .bind("check_if_null")))) .bind("comparison"); auto ConversionThroughComparisonM = binaryOperator(allOf(anyOf(hasOperatorName(">="), hasOperatorName(">"), hasOperatorName("<="), hasOperatorName("<")), hasEitherOperand(SuspiciousNumberObjectExprM), hasEitherOperand(SuspiciousScalarExprM))) .bind("comparison"); auto ConversionThroughConditionalOperatorM = conditionalOperator(allOf( hasCondition(SuspiciousNumberObjectExprM), unless(hasTrueExpression( hasDescendant(AnotherSuspiciousNumberObjectExprM))), unless(hasFalseExpression( hasDescendant(AnotherSuspiciousNumberObjectExprM))))) .bind("pedantic"); auto ConversionThroughExclamationMarkM = unaryOperator(allOf(hasOperatorName("!"), has(expr(SuspiciousNumberObjectExprM)))) .bind("pedantic"); auto ConversionThroughExplicitBooleanCastM = explicitCastExpr(allOf(hasType(SuspiciousScalarBooleanTypeM), has(expr(SuspiciousNumberObjectExprM)))); auto ConversionThroughExplicitNumberCastM = explicitCastExpr(allOf(hasType(SuspiciousScalarNumberTypeM), has(expr(SuspiciousNumberObjectExprM)))); auto ConversionThroughInitializerM = declStmt(hasSingleDecl( varDecl(hasType(SuspiciousScalarTypeM), hasInitializer(SuspiciousNumberObjectExprM)))); auto FinalM = stmt(anyOf(ConversionThroughAssignmentM, ConversionThroughBranchingM, ConversionThroughCallM, ConversionThroughComparisonM, ConversionThroughConditionalOperatorM, ConversionThroughEquivalenceM, ConversionThroughExclamationMarkM, ConversionThroughExplicitBooleanCastM, ConversionThroughExplicitNumberCastM, ConversionThroughInitializerM)).bind("conv"); MatchFinder F; Callback CB(this, BR, AM.getAnalysisDeclContext(D)); F.addMatcher(stmt(forEachDescendant(FinalM)), &CB); F.match(*D->getBody(), AM.getASTContext()); } void ento::registerNumberObjectConversionChecker(CheckerManager &Mgr) { NumberObjectConversionChecker *Chk = Mgr.registerChecker(); Chk->Pedantic = Mgr.getAnalyzerOptions().getCheckerBooleanOption(Chk, "Pedantic"); } bool ento::shouldRegisterNumberObjectConversionChecker(const LangOptions &LO) { return true; }