17330f729Sjoerg //===- NumberObjectConversionChecker.cpp -------------------------*- C++ -*-==//
27330f729Sjoerg //
37330f729Sjoerg // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
47330f729Sjoerg // See https://llvm.org/LICENSE.txt for license information.
57330f729Sjoerg // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
67330f729Sjoerg //
77330f729Sjoerg //===----------------------------------------------------------------------===//
87330f729Sjoerg //
97330f729Sjoerg // This file defines NumberObjectConversionChecker, which checks for a
107330f729Sjoerg // particular common mistake when dealing with numbers represented as objects
117330f729Sjoerg // passed around by pointers. Namely, the language allows to reinterpret the
127330f729Sjoerg // pointer as a number directly, often without throwing any warnings,
137330f729Sjoerg // but in most cases the result of such conversion is clearly unexpected,
147330f729Sjoerg // as pointer value, rather than number value represented by the pointee object,
157330f729Sjoerg // becomes the result of such operation.
167330f729Sjoerg //
177330f729Sjoerg // Currently the checker supports the Objective-C NSNumber class,
187330f729Sjoerg // and the OSBoolean class found in macOS low-level code; the latter
197330f729Sjoerg // can only hold boolean values.
207330f729Sjoerg //
217330f729Sjoerg // This checker has an option "Pedantic" (boolean), which enables detection of
227330f729Sjoerg // more conversion patterns (which are most likely more harmless, and therefore
237330f729Sjoerg // are more likely to produce false positives) - disabled by default,
247330f729Sjoerg // enabled with `-analyzer-config osx.NumberObjectConversion:Pedantic=true'.
257330f729Sjoerg //
267330f729Sjoerg //===----------------------------------------------------------------------===//
277330f729Sjoerg 
287330f729Sjoerg #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
297330f729Sjoerg #include "clang/ASTMatchers/ASTMatchFinder.h"
307330f729Sjoerg #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
317330f729Sjoerg #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
327330f729Sjoerg #include "clang/StaticAnalyzer/Core/Checker.h"
337330f729Sjoerg #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
347330f729Sjoerg #include "clang/Lex/Lexer.h"
357330f729Sjoerg #include "llvm/ADT/APSInt.h"
367330f729Sjoerg 
377330f729Sjoerg using namespace clang;
387330f729Sjoerg using namespace ento;
397330f729Sjoerg using namespace ast_matchers;
407330f729Sjoerg 
417330f729Sjoerg namespace {
427330f729Sjoerg 
437330f729Sjoerg class NumberObjectConversionChecker : public Checker<check::ASTCodeBody> {
447330f729Sjoerg public:
457330f729Sjoerg   bool Pedantic;
467330f729Sjoerg 
477330f729Sjoerg   void checkASTCodeBody(const Decl *D, AnalysisManager &AM,
487330f729Sjoerg                         BugReporter &BR) const;
497330f729Sjoerg };
507330f729Sjoerg 
517330f729Sjoerg class Callback : public MatchFinder::MatchCallback {
527330f729Sjoerg   const NumberObjectConversionChecker *C;
537330f729Sjoerg   BugReporter &BR;
547330f729Sjoerg   AnalysisDeclContext *ADC;
557330f729Sjoerg 
567330f729Sjoerg public:
Callback(const NumberObjectConversionChecker * C,BugReporter & BR,AnalysisDeclContext * ADC)577330f729Sjoerg   Callback(const NumberObjectConversionChecker *C,
587330f729Sjoerg            BugReporter &BR, AnalysisDeclContext *ADC)
597330f729Sjoerg       : C(C), BR(BR), ADC(ADC) {}
60*e038c9c4Sjoerg   void run(const MatchFinder::MatchResult &Result) override;
617330f729Sjoerg };
627330f729Sjoerg } // end of anonymous namespace
637330f729Sjoerg 
run(const MatchFinder::MatchResult & Result)647330f729Sjoerg void Callback::run(const MatchFinder::MatchResult &Result) {
657330f729Sjoerg   bool IsPedanticMatch =
667330f729Sjoerg       (Result.Nodes.getNodeAs<Stmt>("pedantic") != nullptr);
677330f729Sjoerg   if (IsPedanticMatch && !C->Pedantic)
687330f729Sjoerg     return;
697330f729Sjoerg 
707330f729Sjoerg   ASTContext &ACtx = ADC->getASTContext();
717330f729Sjoerg 
727330f729Sjoerg   if (const Expr *CheckIfNull =
737330f729Sjoerg           Result.Nodes.getNodeAs<Expr>("check_if_null")) {
747330f729Sjoerg     // Unless the macro indicates that the intended type is clearly not
757330f729Sjoerg     // a pointer type, we should avoid warning on comparing pointers
767330f729Sjoerg     // to zero literals in non-pedantic mode.
777330f729Sjoerg     // FIXME: Introduce an AST matcher to implement the macro-related logic?
787330f729Sjoerg     bool MacroIndicatesWeShouldSkipTheCheck = false;
797330f729Sjoerg     SourceLocation Loc = CheckIfNull->getBeginLoc();
807330f729Sjoerg     if (Loc.isMacroID()) {
817330f729Sjoerg       StringRef MacroName = Lexer::getImmediateMacroName(
827330f729Sjoerg           Loc, ACtx.getSourceManager(), ACtx.getLangOpts());
837330f729Sjoerg       if (MacroName == "NULL" || MacroName == "nil")
847330f729Sjoerg         return;
857330f729Sjoerg       if (MacroName == "YES" || MacroName == "NO")
867330f729Sjoerg         MacroIndicatesWeShouldSkipTheCheck = true;
877330f729Sjoerg     }
887330f729Sjoerg     if (!MacroIndicatesWeShouldSkipTheCheck) {
897330f729Sjoerg       Expr::EvalResult EVResult;
907330f729Sjoerg       if (CheckIfNull->IgnoreParenCasts()->EvaluateAsInt(
917330f729Sjoerg               EVResult, ACtx, Expr::SE_AllowSideEffects)) {
927330f729Sjoerg         llvm::APSInt Result = EVResult.Val.getInt();
937330f729Sjoerg         if (Result == 0) {
947330f729Sjoerg           if (!C->Pedantic)
957330f729Sjoerg             return;
967330f729Sjoerg           IsPedanticMatch = true;
977330f729Sjoerg         }
987330f729Sjoerg       }
997330f729Sjoerg     }
1007330f729Sjoerg   }
1017330f729Sjoerg 
1027330f729Sjoerg   const Stmt *Conv = Result.Nodes.getNodeAs<Stmt>("conv");
1037330f729Sjoerg   assert(Conv);
1047330f729Sjoerg 
1057330f729Sjoerg   const Expr *ConvertedCObject = Result.Nodes.getNodeAs<Expr>("c_object");
1067330f729Sjoerg   const Expr *ConvertedCppObject = Result.Nodes.getNodeAs<Expr>("cpp_object");
1077330f729Sjoerg   const Expr *ConvertedObjCObject = Result.Nodes.getNodeAs<Expr>("objc_object");
1087330f729Sjoerg   bool IsCpp = (ConvertedCppObject != nullptr);
1097330f729Sjoerg   bool IsObjC = (ConvertedObjCObject != nullptr);
1107330f729Sjoerg   const Expr *Obj = IsObjC ? ConvertedObjCObject
1117330f729Sjoerg                   : IsCpp ? ConvertedCppObject
1127330f729Sjoerg                   : ConvertedCObject;
1137330f729Sjoerg   assert(Obj);
1147330f729Sjoerg 
1157330f729Sjoerg   bool IsComparison =
1167330f729Sjoerg       (Result.Nodes.getNodeAs<Stmt>("comparison") != nullptr);
1177330f729Sjoerg 
1187330f729Sjoerg   bool IsOSNumber =
1197330f729Sjoerg       (Result.Nodes.getNodeAs<Decl>("osnumber") != nullptr);
1207330f729Sjoerg 
1217330f729Sjoerg   bool IsInteger =
1227330f729Sjoerg       (Result.Nodes.getNodeAs<QualType>("int_type") != nullptr);
1237330f729Sjoerg   bool IsObjCBool =
1247330f729Sjoerg       (Result.Nodes.getNodeAs<QualType>("objc_bool_type") != nullptr);
1257330f729Sjoerg   bool IsCppBool =
1267330f729Sjoerg       (Result.Nodes.getNodeAs<QualType>("cpp_bool_type") != nullptr);
1277330f729Sjoerg 
1287330f729Sjoerg   llvm::SmallString<64> Msg;
1297330f729Sjoerg   llvm::raw_svector_ostream OS(Msg);
1307330f729Sjoerg 
1317330f729Sjoerg   // Remove ObjC ARC qualifiers.
1327330f729Sjoerg   QualType ObjT = Obj->getType().getUnqualifiedType();
1337330f729Sjoerg 
1347330f729Sjoerg   // Remove consts from pointers.
1357330f729Sjoerg   if (IsCpp) {
1367330f729Sjoerg     assert(ObjT.getCanonicalType()->isPointerType());
1377330f729Sjoerg     ObjT = ACtx.getPointerType(
1387330f729Sjoerg         ObjT->getPointeeType().getCanonicalType().getUnqualifiedType());
1397330f729Sjoerg   }
1407330f729Sjoerg 
1417330f729Sjoerg   if (IsComparison)
1427330f729Sjoerg     OS << "Comparing ";
1437330f729Sjoerg   else
1447330f729Sjoerg     OS << "Converting ";
1457330f729Sjoerg 
1467330f729Sjoerg   OS << "a pointer value of type '" << ObjT.getAsString() << "' to a ";
1477330f729Sjoerg 
1487330f729Sjoerg   std::string EuphemismForPlain = "primitive";
1497330f729Sjoerg   std::string SuggestedApi = IsObjC ? (IsInteger ? "" : "-boolValue")
1507330f729Sjoerg                            : IsCpp ? (IsOSNumber ? "" : "getValue()")
1517330f729Sjoerg                            : "CFNumberGetValue()";
1527330f729Sjoerg   if (SuggestedApi.empty()) {
1537330f729Sjoerg     // A generic message if we're not sure what API should be called.
1547330f729Sjoerg     // FIXME: Pattern-match the integer type to make a better guess?
1557330f729Sjoerg     SuggestedApi =
1567330f729Sjoerg         "a method on '" + ObjT.getAsString() + "' to get the scalar value";
1577330f729Sjoerg     // "scalar" is not quite correct or common, but some documentation uses it
1587330f729Sjoerg     // when describing object methods we suggest. For consistency, we use
1597330f729Sjoerg     // "scalar" in the whole sentence when we need to use this word in at least
1607330f729Sjoerg     // one place, otherwise we use "primitive".
1617330f729Sjoerg     EuphemismForPlain = "scalar";
1627330f729Sjoerg   }
1637330f729Sjoerg 
1647330f729Sjoerg   if (IsInteger)
1657330f729Sjoerg     OS << EuphemismForPlain << " integer value";
1667330f729Sjoerg   else if (IsObjCBool)
1677330f729Sjoerg     OS << EuphemismForPlain << " BOOL value";
1687330f729Sjoerg   else if (IsCppBool)
1697330f729Sjoerg     OS << EuphemismForPlain << " bool value";
1707330f729Sjoerg   else // Branch condition?
1717330f729Sjoerg     OS << EuphemismForPlain << " boolean value";
1727330f729Sjoerg 
1737330f729Sjoerg 
1747330f729Sjoerg   if (IsPedanticMatch)
1757330f729Sjoerg     OS << "; instead, either compare the pointer to "
1767330f729Sjoerg        << (IsObjC ? "nil" : IsCpp ? "nullptr" : "NULL") << " or ";
1777330f729Sjoerg   else
1787330f729Sjoerg     OS << "; did you mean to ";
1797330f729Sjoerg 
1807330f729Sjoerg   if (IsComparison)
1817330f729Sjoerg     OS << "compare the result of calling " << SuggestedApi;
1827330f729Sjoerg   else
1837330f729Sjoerg     OS << "call " << SuggestedApi;
1847330f729Sjoerg 
1857330f729Sjoerg   if (!IsPedanticMatch)
1867330f729Sjoerg     OS << "?";
1877330f729Sjoerg 
1887330f729Sjoerg   BR.EmitBasicReport(
1897330f729Sjoerg       ADC->getDecl(), C, "Suspicious number object conversion", "Logic error",
1907330f729Sjoerg       OS.str(),
1917330f729Sjoerg       PathDiagnosticLocation::createBegin(Obj, BR.getSourceManager(), ADC),
1927330f729Sjoerg       Conv->getSourceRange());
1937330f729Sjoerg }
1947330f729Sjoerg 
checkASTCodeBody(const Decl * D,AnalysisManager & AM,BugReporter & BR) const1957330f729Sjoerg void NumberObjectConversionChecker::checkASTCodeBody(const Decl *D,
1967330f729Sjoerg                                                      AnalysisManager &AM,
1977330f729Sjoerg                                                      BugReporter &BR) const {
1987330f729Sjoerg   // Currently this matches CoreFoundation opaque pointer typedefs.
1997330f729Sjoerg   auto CSuspiciousNumberObjectExprM =
2007330f729Sjoerg       expr(ignoringParenImpCasts(
2017330f729Sjoerg           expr(hasType(
2027330f729Sjoerg               typedefType(hasDeclaration(anyOf(
2037330f729Sjoerg                   typedefDecl(hasName("CFNumberRef")),
2047330f729Sjoerg                   typedefDecl(hasName("CFBooleanRef")))))))
2057330f729Sjoerg           .bind("c_object")));
2067330f729Sjoerg 
2077330f729Sjoerg   // Currently this matches XNU kernel number-object pointers.
2087330f729Sjoerg   auto CppSuspiciousNumberObjectExprM =
2097330f729Sjoerg       expr(ignoringParenImpCasts(
2107330f729Sjoerg           expr(hasType(hasCanonicalType(
2117330f729Sjoerg               pointerType(pointee(hasCanonicalType(
2127330f729Sjoerg                   recordType(hasDeclaration(
2137330f729Sjoerg                       anyOf(
2147330f729Sjoerg                         cxxRecordDecl(hasName("OSBoolean")),
2157330f729Sjoerg                         cxxRecordDecl(hasName("OSNumber"))
2167330f729Sjoerg                             .bind("osnumber"))))))))))
2177330f729Sjoerg           .bind("cpp_object")));
2187330f729Sjoerg 
2197330f729Sjoerg   // Currently this matches NeXTSTEP number objects.
2207330f729Sjoerg   auto ObjCSuspiciousNumberObjectExprM =
2217330f729Sjoerg       expr(ignoringParenImpCasts(
2227330f729Sjoerg           expr(hasType(hasCanonicalType(
2237330f729Sjoerg               objcObjectPointerType(pointee(
2247330f729Sjoerg                   qualType(hasCanonicalType(
2257330f729Sjoerg                       qualType(hasDeclaration(
2267330f729Sjoerg                           objcInterfaceDecl(hasName("NSNumber")))))))))))
2277330f729Sjoerg           .bind("objc_object")));
2287330f729Sjoerg 
2297330f729Sjoerg   auto SuspiciousNumberObjectExprM = anyOf(
2307330f729Sjoerg       CSuspiciousNumberObjectExprM,
2317330f729Sjoerg       CppSuspiciousNumberObjectExprM,
2327330f729Sjoerg       ObjCSuspiciousNumberObjectExprM);
2337330f729Sjoerg 
2347330f729Sjoerg   // Useful for predicates like "Unless we've seen the same object elsewhere".
2357330f729Sjoerg   auto AnotherSuspiciousNumberObjectExprM =
2367330f729Sjoerg       expr(anyOf(
2377330f729Sjoerg           equalsBoundNode("c_object"),
2387330f729Sjoerg           equalsBoundNode("objc_object"),
2397330f729Sjoerg           equalsBoundNode("cpp_object")));
2407330f729Sjoerg 
2417330f729Sjoerg   // The .bind here is in order to compose the error message more accurately.
2427330f729Sjoerg   auto ObjCSuspiciousScalarBooleanTypeM =
2437330f729Sjoerg       qualType(typedefType(hasDeclaration(
2447330f729Sjoerg                    typedefDecl(hasName("BOOL"))))).bind("objc_bool_type");
2457330f729Sjoerg 
2467330f729Sjoerg   // The .bind here is in order to compose the error message more accurately.
2477330f729Sjoerg   auto SuspiciousScalarBooleanTypeM =
2487330f729Sjoerg       qualType(anyOf(qualType(booleanType()).bind("cpp_bool_type"),
2497330f729Sjoerg                      ObjCSuspiciousScalarBooleanTypeM));
2507330f729Sjoerg 
2517330f729Sjoerg   // The .bind here is in order to compose the error message more accurately.
2527330f729Sjoerg   // Also avoid intptr_t and uintptr_t because they were specifically created
2537330f729Sjoerg   // for storing pointers.
2547330f729Sjoerg   auto SuspiciousScalarNumberTypeM =
2557330f729Sjoerg       qualType(hasCanonicalType(isInteger()),
2567330f729Sjoerg                unless(typedefType(hasDeclaration(
2577330f729Sjoerg                    typedefDecl(matchesName("^::u?intptr_t$"))))))
2587330f729Sjoerg       .bind("int_type");
2597330f729Sjoerg 
2607330f729Sjoerg   auto SuspiciousScalarTypeM =
2617330f729Sjoerg       qualType(anyOf(SuspiciousScalarBooleanTypeM,
2627330f729Sjoerg                      SuspiciousScalarNumberTypeM));
2637330f729Sjoerg 
2647330f729Sjoerg   auto SuspiciousScalarExprM =
2657330f729Sjoerg       expr(ignoringParenImpCasts(expr(hasType(SuspiciousScalarTypeM))));
2667330f729Sjoerg 
2677330f729Sjoerg   auto ConversionThroughAssignmentM =
2687330f729Sjoerg       binaryOperator(allOf(hasOperatorName("="),
2697330f729Sjoerg                            hasLHS(SuspiciousScalarExprM),
2707330f729Sjoerg                            hasRHS(SuspiciousNumberObjectExprM)));
2717330f729Sjoerg 
2727330f729Sjoerg   auto ConversionThroughBranchingM =
2737330f729Sjoerg       ifStmt(allOf(
2747330f729Sjoerg           hasCondition(SuspiciousNumberObjectExprM),
2757330f729Sjoerg           unless(hasConditionVariableStatement(declStmt())
2767330f729Sjoerg       ))).bind("pedantic");
2777330f729Sjoerg 
2787330f729Sjoerg   auto ConversionThroughCallM =
2797330f729Sjoerg       callExpr(hasAnyArgument(allOf(hasType(SuspiciousScalarTypeM),
2807330f729Sjoerg                                     ignoringParenImpCasts(
2817330f729Sjoerg                                         SuspiciousNumberObjectExprM))));
2827330f729Sjoerg 
2837330f729Sjoerg   // We bind "check_if_null" to modify the warning message
2847330f729Sjoerg   // in case it was intended to compare a pointer to 0 with a relatively-ok
2857330f729Sjoerg   // construct "x == 0" or "x != 0".
2867330f729Sjoerg   auto ConversionThroughEquivalenceM =
2877330f729Sjoerg       binaryOperator(allOf(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
2887330f729Sjoerg                            hasEitherOperand(SuspiciousNumberObjectExprM),
2897330f729Sjoerg                            hasEitherOperand(SuspiciousScalarExprM
2907330f729Sjoerg                                             .bind("check_if_null"))))
2917330f729Sjoerg       .bind("comparison");
2927330f729Sjoerg 
2937330f729Sjoerg   auto ConversionThroughComparisonM =
2947330f729Sjoerg       binaryOperator(allOf(anyOf(hasOperatorName(">="), hasOperatorName(">"),
2957330f729Sjoerg                                  hasOperatorName("<="), hasOperatorName("<")),
2967330f729Sjoerg                            hasEitherOperand(SuspiciousNumberObjectExprM),
2977330f729Sjoerg                            hasEitherOperand(SuspiciousScalarExprM)))
2987330f729Sjoerg       .bind("comparison");
2997330f729Sjoerg 
3007330f729Sjoerg   auto ConversionThroughConditionalOperatorM =
3017330f729Sjoerg       conditionalOperator(allOf(
3027330f729Sjoerg           hasCondition(SuspiciousNumberObjectExprM),
3037330f729Sjoerg           unless(hasTrueExpression(
3047330f729Sjoerg               hasDescendant(AnotherSuspiciousNumberObjectExprM))),
3057330f729Sjoerg           unless(hasFalseExpression(
3067330f729Sjoerg               hasDescendant(AnotherSuspiciousNumberObjectExprM)))))
3077330f729Sjoerg       .bind("pedantic");
3087330f729Sjoerg 
3097330f729Sjoerg   auto ConversionThroughExclamationMarkM =
3107330f729Sjoerg       unaryOperator(allOf(hasOperatorName("!"),
3117330f729Sjoerg                           has(expr(SuspiciousNumberObjectExprM))))
3127330f729Sjoerg       .bind("pedantic");
3137330f729Sjoerg 
3147330f729Sjoerg   auto ConversionThroughExplicitBooleanCastM =
3157330f729Sjoerg       explicitCastExpr(allOf(hasType(SuspiciousScalarBooleanTypeM),
3167330f729Sjoerg                              has(expr(SuspiciousNumberObjectExprM))));
3177330f729Sjoerg 
3187330f729Sjoerg   auto ConversionThroughExplicitNumberCastM =
3197330f729Sjoerg       explicitCastExpr(allOf(hasType(SuspiciousScalarNumberTypeM),
3207330f729Sjoerg                              has(expr(SuspiciousNumberObjectExprM))));
3217330f729Sjoerg 
3227330f729Sjoerg   auto ConversionThroughInitializerM =
3237330f729Sjoerg       declStmt(hasSingleDecl(
3247330f729Sjoerg           varDecl(hasType(SuspiciousScalarTypeM),
3257330f729Sjoerg                   hasInitializer(SuspiciousNumberObjectExprM))));
3267330f729Sjoerg 
3277330f729Sjoerg   auto FinalM = stmt(anyOf(ConversionThroughAssignmentM,
3287330f729Sjoerg                            ConversionThroughBranchingM,
3297330f729Sjoerg                            ConversionThroughCallM,
3307330f729Sjoerg                            ConversionThroughComparisonM,
3317330f729Sjoerg                            ConversionThroughConditionalOperatorM,
3327330f729Sjoerg                            ConversionThroughEquivalenceM,
3337330f729Sjoerg                            ConversionThroughExclamationMarkM,
3347330f729Sjoerg                            ConversionThroughExplicitBooleanCastM,
3357330f729Sjoerg                            ConversionThroughExplicitNumberCastM,
3367330f729Sjoerg                            ConversionThroughInitializerM)).bind("conv");
3377330f729Sjoerg 
3387330f729Sjoerg   MatchFinder F;
3397330f729Sjoerg   Callback CB(this, BR, AM.getAnalysisDeclContext(D));
3407330f729Sjoerg 
341*e038c9c4Sjoerg   F.addMatcher(traverse(TK_AsIs, stmt(forEachDescendant(FinalM))), &CB);
3427330f729Sjoerg   F.match(*D->getBody(), AM.getASTContext());
3437330f729Sjoerg }
3447330f729Sjoerg 
registerNumberObjectConversionChecker(CheckerManager & Mgr)3457330f729Sjoerg void ento::registerNumberObjectConversionChecker(CheckerManager &Mgr) {
3467330f729Sjoerg   NumberObjectConversionChecker *Chk =
3477330f729Sjoerg       Mgr.registerChecker<NumberObjectConversionChecker>();
3487330f729Sjoerg   Chk->Pedantic =
3497330f729Sjoerg       Mgr.getAnalyzerOptions().getCheckerBooleanOption(Chk, "Pedantic");
3507330f729Sjoerg }
3517330f729Sjoerg 
shouldRegisterNumberObjectConversionChecker(const CheckerManager & mgr)352*e038c9c4Sjoerg bool ento::shouldRegisterNumberObjectConversionChecker(const CheckerManager &mgr) {
3537330f729Sjoerg   return true;
3547330f729Sjoerg }
355