diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index 707067358fdfe..c1eedb33e74d2 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -3668,6 +3668,51 @@ Here are some examples of situations that we warn about as they *might* be poten RefCountable* uncounted = counted.get(); // warn } +alpha.webkit.UnretainedLocalVarsChecker +""""""""""""""""""""""""""""""""""""""" +The goal of this rule is to make sure that any NS or CF local variable is backed by a RetainPtr with lifetime that is strictly larger than the scope of the unretained local variable. To be on the safe side we require the scope of an unretained variable to be embedded in the scope of Retainptr object that backs it. + +The rules of when to use and not to use RetainPtr are same as alpha.webkit.UncountedCallArgsChecker for ref-counted objects. + +These are examples of cases that we consider safe: + + .. code-block:: cpp + + void foo1() { + RetainPtr retained; + // The scope of unretained is EMBEDDED in the scope of retained. + { + NSObject* unretained = retained.get(); // ok + } + } + + void foo2(RetainPtr retained_param) { + NSObject* unretained = retained_param.get(); // ok + } + + void FooClass::foo_method() { + NSObject* unretained = this; // ok + } + +Here are some examples of situations that we warn about as they *might* be potentially unsafe. The logic is that either we're able to guarantee that a local variable is safe or it's considered unsafe. + + .. code-block:: cpp + + void foo1() { + NSObject* unretained = [[NSObject alloc] init]; // warn + } + + NSObject* global_unretained; + void foo2() { + NSObject* unretained = global_unretained; // warn + } + + void foo3() { + RetainPtr retained; + // The scope of unretained is not EMBEDDED in the scope of retained. + NSObject* unretained = retained.get(); // warn + } + Debug Checkers --------------- diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 9bf491eac1e0e..410f841630660 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1782,4 +1782,8 @@ def UncheckedLocalVarsChecker : Checker<"UncheckedLocalVarsChecker">, HelpText<"Check unchecked local variables.">, Documentation; +def UnretainedLocalVarsChecker : Checker<"UnretainedLocalVarsChecker">, + HelpText<"Check unretained local variables.">, + Documentation; + } // end alpha.webkit diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp index a12853d01819f..dc86c4fcc64b1 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp @@ -24,6 +24,8 @@ bool isSafePtr(clang::CXXRecordDecl *Decl) { bool tryToFindPtrOrigin( const Expr *E, bool StopAtFirstRefCountedObj, + std::function isSafePtr, + std::function isSafePtrType, std::function callback) { while (E) { if (auto *tempExpr = dyn_cast(E)) { @@ -53,9 +55,9 @@ bool tryToFindPtrOrigin( } if (auto *Expr = dyn_cast(E)) { return tryToFindPtrOrigin(Expr->getTrueExpr(), StopAtFirstRefCountedObj, - callback) && + isSafePtr, isSafePtrType, callback) && tryToFindPtrOrigin(Expr->getFalseExpr(), StopAtFirstRefCountedObj, - callback); + isSafePtr, isSafePtrType, callback); } if (auto *cast = dyn_cast(E)) { if (StopAtFirstRefCountedObj) { @@ -92,7 +94,7 @@ bool tryToFindPtrOrigin( } if (auto *callee = call->getDirectCallee()) { - if (isCtorOfRefCounted(callee) || isCtorOfCheckedPtr(callee)) { + if (isCtorOfSafePtr(callee)) { if (StopAtFirstRefCountedObj) return callback(E, true); diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h index b508043d0f37f..e2cc7b976adfc 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h @@ -54,6 +54,8 @@ class Expr; /// Returns false if any of calls to callbacks returned false. Otherwise true. bool tryToFindPtrOrigin( const clang::Expr *E, bool StopAtFirstRefCountedObj, + std::function isSafePtr, + std::function isSafePtrType, std::function callback); /// For \p E referring to a ref-countable/-counted pointer/reference we return diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 8340de9e5a7a9..7899b19854806 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -8,11 +8,13 @@ #include "PtrTypesSemantics.h" #include "ASTUtils.h" +#include "clang/AST/Attr.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/Decl.h" #include "clang/AST/DeclCXX.h" #include "clang/AST/ExprCXX.h" #include "clang/AST/StmtVisitor.h" +#include "clang/Analysis/DomainSpecific/CocoaConventions.h" #include using namespace clang; @@ -117,6 +119,8 @@ bool isRefType(const std::string &Name) { Name == "RefPtr" || Name == "RefPtrAllowingPartiallyDestroyed"; } +bool isRetainPtr(const std::string &Name) { return Name == "RetainPtr"; } + bool isCheckedPtr(const std::string &Name) { return Name == "CheckedPtr" || Name == "CheckedRef"; } @@ -140,8 +144,14 @@ bool isCtorOfCheckedPtr(const clang::FunctionDecl *F) { return isCheckedPtr(safeGetName(F)); } +bool isCtorOfRetainPtr(const clang::FunctionDecl *F) { + const std::string &FunctionName = safeGetName(F); + return FunctionName == "RetainPtr" || FunctionName == "adoptNS" || + FunctionName == "adoptCF"; +} + bool isCtorOfSafePtr(const clang::FunctionDecl *F) { - return isCtorOfRefCounted(F) || isCtorOfCheckedPtr(F); + return isCtorOfRefCounted(F) || isCtorOfCheckedPtr(F) || isCtorOfRetainPtr(F); } template @@ -163,11 +173,15 @@ static bool isPtrOfType(const clang::QualType T, Predicate Pred) { return false; } -bool isSafePtrType(const clang::QualType T) { +bool isRefOrCheckedPtrType(const clang::QualType T) { return isPtrOfType( T, [](auto Name) { return isRefType(Name) || isCheckedPtr(Name); }); } +bool isRetainPtrType(const clang::QualType T) { + return isPtrOfType(T, [](auto Name) { return Name == "RetainPtr"; }); +} + bool isOwnerPtrType(const clang::QualType T) { return isPtrOfType(T, [](auto Name) { return isRefType(Name) || isCheckedPtr(Name) || Name == "unique_ptr" || @@ -195,6 +209,69 @@ std::optional isUnchecked(const QualType T) { return isUnchecked(T->getAsCXXRecordDecl()); } +void RetainTypeChecker::visitTranslationUnitDecl( + const TranslationUnitDecl *TUD) { + IsARCEnabled = TUD->getLangOpts().ObjCAutoRefCount; +} + +void RetainTypeChecker::visitTypedef(const TypedefDecl *TD) { + auto QT = TD->getUnderlyingType(); + if (!QT->isPointerType()) + return; + + auto PointeeQT = QT->getPointeeType(); + const RecordType *RT = PointeeQT->getAs(); + if (!RT) + return; + + for (auto *Redecl : RT->getDecl()->getMostRecentDecl()->redecls()) { + if (Redecl->getAttr()) { + CFPointees.insert(RT); + return; + } + } +} + +bool RetainTypeChecker::isUnretained(const QualType QT) { + if (ento::cocoa::isCocoaObjectRef(QT) && !IsARCEnabled) + return true; + auto CanonicalType = QT.getCanonicalType(); + auto PointeeType = CanonicalType->getPointeeType(); + auto *RT = dyn_cast_or_null(PointeeType.getTypePtrOrNull()); + return RT && CFPointees.contains(RT); +} + +std::optional isUnretained(const QualType T, bool IsARCEnabled) { + if (auto *Subst = dyn_cast(T)) { + if (auto *Decl = Subst->getAssociatedDecl()) { + if (isRetainPtr(safeGetName(Decl))) + return false; + } + } + if ((ento::cocoa::isCocoaObjectRef(T) && !IsARCEnabled) || + ento::coreFoundation::isCFObjectRef(T)) + return true; + + // RetainPtr strips typedef for CF*Ref. Manually check for struct __CF* types. + auto CanonicalType = T.getCanonicalType(); + auto *Type = CanonicalType.getTypePtrOrNull(); + if (!Type) + return false; + auto Pointee = Type->getPointeeType(); + auto *PointeeType = Pointee.getTypePtrOrNull(); + if (!PointeeType) + return false; + auto *Record = PointeeType->getAsStructureType(); + if (!Record) + return false; + auto *Decl = Record->getDecl(); + if (!Decl) + return false; + auto TypeName = Decl->getName(); + return TypeName.starts_with("__CF") || TypeName.starts_with("__CG") || + TypeName.starts_with("__CM"); +} + std::optional isUncounted(const CXXRecordDecl* Class) { // Keep isRefCounted first as it's cheaper. @@ -230,16 +307,20 @@ std::optional isUncheckedPtr(const QualType T) { return false; } -std::optional isUnsafePtr(const QualType T) { +std::optional isUnsafePtr(const QualType T, bool IsArcEnabled) { if (T->isPointerType() || T->isReferenceType()) { if (auto *CXXRD = T->getPointeeCXXRecordDecl()) { auto isUncountedPtr = isUncounted(CXXRD); auto isUncheckedPtr = isUnchecked(CXXRD); - if (isUncountedPtr && isUncheckedPtr) - return *isUncountedPtr || *isUncheckedPtr; + auto isUnretainedPtr = isUnretained(T, IsArcEnabled); + std::optional result; if (isUncountedPtr) - return *isUncountedPtr; - return isUncheckedPtr; + result = *isUncountedPtr; + if (isUncheckedPtr) + result = result ? *result || *isUncheckedPtr : *isUncheckedPtr; + if (isUnretainedPtr) + result = result ? *result || *isUnretainedPtr : *isUnretainedPtr; + return result; } } return false; @@ -248,6 +329,8 @@ std::optional isUnsafePtr(const QualType T) { std::optional isGetterOfSafePtr(const CXXMethodDecl *M) { assert(M); + std::optional RTC; + if (isa(M)) { const CXXRecordDecl *calleeMethodsClass = M->getParent(); auto className = safeGetName(calleeMethodsClass); @@ -263,16 +346,33 @@ std::optional isGetterOfSafePtr(const CXXMethodDecl *M) { method == "impl")) return true; + if (className == "RetainPtr" && method == "get") + return true; + // Ref -> T conversion // FIXME: Currently allowing any Ref -> whatever cast. if (isRefType(className)) { - if (auto *maybeRefToRawOperator = dyn_cast(M)) - return isUnsafePtr(maybeRefToRawOperator->getConversionType()); + if (auto *maybeRefToRawOperator = dyn_cast(M)) { + auto QT = maybeRefToRawOperator->getConversionType(); + auto *T = QT.getTypePtrOrNull(); + return T && (T->isPointerType() || T->isReferenceType()); + } } if (isCheckedPtr(className)) { - if (auto *maybeRefToRawOperator = dyn_cast(M)) - return isUnsafePtr(maybeRefToRawOperator->getConversionType()); + if (auto *maybeRefToRawOperator = dyn_cast(M)) { + auto QT = maybeRefToRawOperator->getConversionType(); + auto *T = QT.getTypePtrOrNull(); + return T && (T->isPointerType() || T->isReferenceType()); + } + } + + if (className == "RetainPtr") { + if (auto *maybeRefToRawOperator = dyn_cast(M)) { + auto QT = maybeRefToRawOperator->getConversionType(); + auto *T = QT.getTypePtrOrNull(); + return T && (T->isPointerType() || T->isReferenceType()); + } } } return false; @@ -297,6 +397,13 @@ bool isCheckedPtr(const CXXRecordDecl *R) { return false; } +bool isRetainPtr(const CXXRecordDecl *R) { + assert(R); + if (auto *TmplR = R->getTemplateInstantiationPattern()) + return safeGetName(TmplR) == "RetainPtr"; + return false; +} + bool isPtrConversion(const FunctionDecl *F) { assert(F); if (isCtorOfRefCounted(F)) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h index 9adfc0f1f5726..fcc1a41dba78b 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -11,6 +11,7 @@ #include "llvm/ADT/APInt.h" #include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/DenseSet.h" #include "llvm/ADT/PointerUnion.h" #include @@ -21,8 +22,11 @@ class CXXRecordDecl; class Decl; class FunctionDecl; class QualType; +class RecordType; class Stmt; +class TranslationUnitDecl; class Type; +class TypedefDecl; // Ref-countability of a type is implicitly defined by Ref and RefPtr // implementation. It can be modeled as: type T having public methods ref() and @@ -51,6 +55,9 @@ bool isRefCounted(const clang::CXXRecordDecl *Class); /// \returns true if \p Class is a CheckedPtr / CheckedRef, false if not. bool isCheckedPtr(const clang::CXXRecordDecl *Class); +/// \returns true if \p Class is a RetainPtr, false if not. +bool isRetainPtr(const clang::CXXRecordDecl *Class); + /// \returns true if \p Class is ref-countable AND not ref-counted, false if /// not, std::nullopt if inconclusive. std::optional isUncounted(const clang::QualType T); @@ -59,6 +66,22 @@ std::optional isUncounted(const clang::QualType T); /// not, std::nullopt if inconclusive. std::optional isUnchecked(const clang::QualType T); +/// An inter-procedural analysis facility that detects CF types with the +/// underlying pointer type. +class RetainTypeChecker { + llvm::DenseSet CFPointees; + bool IsARCEnabled{false}; + +public: + void visitTranslationUnitDecl(const TranslationUnitDecl *); + void visitTypedef(const TypedefDecl *); + bool isUnretained(const QualType); +}; + +/// \returns true if \p Class is NS or CF objects AND not retained, false if +/// not, std::nullopt if inconclusive. +std::optional isUnretained(const clang::QualType T, bool IsARCEnabled); + /// \returns true if \p Class is ref-countable AND not ref-counted, false if /// not, std::nullopt if inconclusive. std::optional isUncounted(const clang::CXXRecordDecl* Class); @@ -77,11 +100,14 @@ std::optional isUncheckedPtr(const clang::QualType T); /// \returns true if \p T is either a raw pointer or reference to an uncounted /// or unchecked class, false if not, std::nullopt if inconclusive. -std::optional isUnsafePtr(const QualType T); +std::optional isUnsafePtr(const QualType T, bool IsArcEnabled); /// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or its /// variant, false if not. -bool isSafePtrType(const clang::QualType T); +bool isRefOrCheckedPtrType(const clang::QualType T); + +/// \returns true if \p T is a RetainPtr, false if not. +bool isRetainPtrType(const clang::QualType T); /// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or /// unique_ptr, false if not. diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp index 56fa72c100ec8..d633fbcbd798b 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp @@ -41,6 +41,8 @@ class RawPtrRefCallArgsChecker virtual std::optional isUnsafeType(QualType) const = 0; virtual std::optional isUnsafePtr(QualType) const = 0; + virtual bool isSafePtr(const CXXRecordDecl *Record) const = 0; + virtual bool isSafePtrType(const QualType type) const = 0; virtual const char *ptrKind() const = 0; void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR, @@ -140,25 +142,28 @@ class RawPtrRefCallArgsChecker } bool isPtrOriginSafe(const Expr *Arg) const { - return tryToFindPtrOrigin(Arg, /*StopAtFirstRefCountedObj=*/true, - [&](const clang::Expr *ArgOrigin, bool IsSafe) { - if (IsSafe) - return true; - if (isa(ArgOrigin)) { - // foo(nullptr) - return true; - } - if (isa(ArgOrigin)) { - // FIXME: Check the value. - // foo(NULL) - return true; - } - if (isASafeCallArg(ArgOrigin)) - return true; - if (EFA.isACallToEnsureFn(ArgOrigin)) - return true; - return false; - }); + return tryToFindPtrOrigin( + Arg, /*StopAtFirstRefCountedObj=*/true, + [&](const clang::CXXRecordDecl *Record) { return isSafePtr(Record); }, + [&](const clang::QualType T) { return isSafePtrType(T); }, + [&](const clang::Expr *ArgOrigin, bool IsSafe) { + if (IsSafe) + return true; + if (isa(ArgOrigin)) { + // foo(nullptr) + return true; + } + if (isa(ArgOrigin)) { + // FIXME: Check the value. + // foo(NULL) + return true; + } + if (isASafeCallArg(ArgOrigin)) + return true; + if (EFA.isACallToEnsureFn(ArgOrigin)) + return true; + return false; + }); } bool shouldSkipCall(const CallExpr *CE) const { @@ -315,6 +320,14 @@ class UncountedCallArgsChecker final : public RawPtrRefCallArgsChecker { return isUncountedPtr(QT); } + bool isSafePtr(const CXXRecordDecl *Record) const final { + return isRefCounted(Record) || isCheckedPtr(Record); + } + + bool isSafePtrType(const QualType type) const final { + return isRefOrCheckedPtrType(type); + } + const char *ptrKind() const final { return "uncounted"; } }; @@ -332,6 +345,14 @@ class UncheckedCallArgsChecker final : public RawPtrRefCallArgsChecker { return isUncheckedPtr(QT); } + bool isSafePtr(const CXXRecordDecl *Record) const final { + return isRefCounted(Record) || isCheckedPtr(Record); + } + + bool isSafePtrType(const QualType type) const final { + return isRefOrCheckedPtrType(type); + } + const char *ptrKind() const final { return "unchecked"; } }; diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp index d586668399502..d413e33a490c5 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp @@ -14,6 +14,7 @@ #include "clang/AST/DeclCXX.h" #include "clang/AST/DynamicRecursiveASTVisitor.h" #include "clang/AST/ParentMapContext.h" +#include "clang/Analysis/DomainSpecific/CocoaConventions.h" #include "clang/Basic/SourceLocation.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" @@ -81,7 +82,7 @@ struct GuardianVisitor : DynamicRecursiveASTVisitor { bool VisitCXXMemberCallExpr(CXXMemberCallExpr *MCE) override { auto MethodName = safeGetName(MCE->getMethodDecl()); if (MethodName == "swap" || MethodName == "leakRef" || - MethodName == "releaseNonNull") { + MethodName == "releaseNonNull" || MethodName == "clear") { auto *ThisArg = MCE->getImplicitObjectArgument()->IgnoreParenCasts(); if (auto *VarRef = dyn_cast(ThisArg)) { if (VarRef->getDecl() == Guardian) @@ -168,11 +169,16 @@ class RawPtrRefLocalVarsChecker mutable BugReporter *BR; EnsureFunctionAnalysis EFA; +protected: + mutable std::optional RTC; + public: RawPtrRefLocalVarsChecker(const char *description) : Bug(this, description, "WebKit coding guidelines") {} virtual std::optional isUnsafePtr(const QualType T) const = 0; + virtual bool isSafePtr(const CXXRecordDecl *) const = 0; + virtual bool isSafePtrType(const QualType) const = 0; virtual const char *ptrKind() const = 0; void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR, @@ -202,6 +208,12 @@ class RawPtrRefLocalVarsChecker return DynamicRecursiveASTVisitor::TraverseDecl(D); } + bool VisitTypedefDecl(TypedefDecl *TD) override { + if (Checker->RTC) + Checker->RTC->visitTypedef(TD); + return true; + } + bool VisitVarDecl(VarDecl *V) override { auto *Init = V->getInit(); if (Init && V->isLocalVarDecl()) @@ -251,6 +263,8 @@ class RawPtrRefLocalVarsChecker }; LocalVisitor visitor(this); + if (RTC) + RTC->visitTranslationUnitDecl(TUD); visitor.TraverseDecl(const_cast(TUD)); } @@ -263,6 +277,10 @@ class RawPtrRefLocalVarsChecker if (IsUncountedPtr && *IsUncountedPtr) { if (tryToFindPtrOrigin( Value, /*StopAtFirstRefCountedObj=*/false, + [&](const clang::CXXRecordDecl *Record) { + return isSafePtr(Record); + }, + [&](const clang::QualType Type) { return isSafePtrType(Type); }, [&](const clang::Expr *InitArgOrigin, bool IsSafe) { if (!InitArgOrigin || IsSafe) return true; @@ -292,8 +310,7 @@ class RawPtrRefLocalVarsChecker MaybeGuardianArgType->getAsCXXRecordDecl(); if (MaybeGuardianArgCXXRecord) { if (MaybeGuardian->isLocalVarDecl() && - (isRefCounted(MaybeGuardianArgCXXRecord) || - isCheckedPtr(MaybeGuardianArgCXXRecord) || + (isSafePtr(MaybeGuardianArgCXXRecord) || isRefcountedStringsHack(MaybeGuardian)) && isGuardedScopeEmbeddedInGuardianScope( V, MaybeGuardian)) @@ -318,6 +335,8 @@ class RawPtrRefLocalVarsChecker bool shouldSkipVarDecl(const VarDecl *V) const { assert(V); + if (isa(V)) + return true; return BR->getSourceManager().isInSystemHeader(V->getLocation()); } @@ -365,6 +384,12 @@ class UncountedLocalVarsChecker final : public RawPtrRefLocalVarsChecker { std::optional isUnsafePtr(const QualType T) const final { return isUncountedPtr(T); } + bool isSafePtr(const CXXRecordDecl *Record) const final { + return isRefCounted(Record) || isCheckedPtr(Record); + } + bool isSafePtrType(const QualType type) const final { + return isRefOrCheckedPtrType(type); + } const char *ptrKind() const final { return "uncounted"; } }; @@ -376,9 +401,34 @@ class UncheckedLocalVarsChecker final : public RawPtrRefLocalVarsChecker { std::optional isUnsafePtr(const QualType T) const final { return isUncheckedPtr(T); } + bool isSafePtr(const CXXRecordDecl *Record) const final { + return isRefCounted(Record) || isCheckedPtr(Record); + } + bool isSafePtrType(const QualType type) const final { + return isRefOrCheckedPtrType(type); + } const char *ptrKind() const final { return "unchecked"; } }; +class UnretainedLocalVarsChecker final : public RawPtrRefLocalVarsChecker { +public: + UnretainedLocalVarsChecker() + : RawPtrRefLocalVarsChecker("Unretained raw pointer or reference not " + "provably backed by a RetainPtr") { + RTC = RetainTypeChecker(); + } + std::optional isUnsafePtr(const QualType T) const final { + return RTC->isUnretained(T); + } + bool isSafePtr(const CXXRecordDecl *Record) const final { + return isRetainPtr(Record); + } + bool isSafePtrType(const QualType type) const final { + return isRetainPtrType(type); + } + const char *ptrKind() const final { return "unretained"; } +}; + } // namespace void ento::registerUncountedLocalVarsChecker(CheckerManager &Mgr) { @@ -396,3 +446,11 @@ void ento::registerUncheckedLocalVarsChecker(CheckerManager &Mgr) { bool ento::shouldRegisterUncheckedLocalVarsChecker(const CheckerManager &) { return true; } + +void ento::registerUnretainedLocalVarsChecker(CheckerManager &Mgr) { + Mgr.registerChecker(); +} + +bool ento::shouldRegisterUnretainedLocalVarsChecker(const CheckerManager &) { + return true; +} diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp index 9527993d0edeb..506442f352288 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp @@ -61,7 +61,8 @@ class UncountedLambdaCapturesChecker } bool shouldCheckThis() { - auto result = !ClsType.isNull() ? isUnsafePtr(ClsType) : std::nullopt; + auto result = + !ClsType.isNull() ? isUnsafePtr(ClsType, false) : std::nullopt; return result && *result; } @@ -305,7 +306,7 @@ class UncountedLambdaCapturesChecker if (ignoreParamVarDecl && isa(CapturedVar)) continue; QualType CapturedVarQualType = CapturedVar->getType(); - auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType()); + auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType(), false); if (IsUncountedPtr && *IsUncountedPtr) reportBug(C, CapturedVar, CapturedVarQualType); } else if (C.capturesThis() && shouldCheckThis) { diff --git a/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h new file mode 100644 index 0000000000000..7bb33bcb6cf44 --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h @@ -0,0 +1,131 @@ +@class NSString; +@class NSArray; +@class NSMutableArray; +#define CF_BRIDGED_TYPE(T) __attribute__((objc_bridge(T))) +typedef CF_BRIDGED_TYPE(id) void * CFTypeRef; +typedef signed long CFIndex; +typedef const struct __CFAllocator * CFAllocatorRef; +typedef const struct CF_BRIDGED_TYPE(NSString) __CFString * CFStringRef; +typedef const struct CF_BRIDGED_TYPE(NSArray) __CFArray * CFArrayRef; +typedef struct CF_BRIDGED_TYPE(NSMutableArray) __CFArray * CFMutableArrayRef; +extern const CFAllocatorRef kCFAllocatorDefault; +CFMutableArrayRef CFArrayCreateMutable(CFAllocatorRef allocator, CFIndex capacity); +extern void CFArrayAppendValue(CFMutableArrayRef theArray, const void *value); +CFArrayRef CFArrayCreate(CFAllocatorRef allocator, const void **values, CFIndex numValues); +CFIndex CFArrayGetCount(CFArrayRef theArray); +extern CFTypeRef CFRetain(CFTypeRef cf); +extern void CFRelease(CFTypeRef cf); + +__attribute__((objc_root_class)) +@interface NSObject ++ (instancetype) alloc; +- (instancetype) init; +- (instancetype)retain; +- (void)release; +@end + +@interface SomeObj : NSObject +- (void)doWork; +- (SomeObj *)other; +- (SomeObj *)next; +- (int)value; +@end + +@interface OtherObj : SomeObj +- (void)doMoreWork:(OtherObj *)other; +@end + +namespace WTF { + +template class RetainPtr; +template RetainPtr adoptNS(T*); +template RetainPtr adoptCF(T); + +template T *downcast(S *t) { return static_cast(t); } + +template struct RemovePointer { + typedef T Type; +}; + +template struct RemovePointer { + typedef T Type; +}; + +template struct RetainPtr { + using ValueType = typename RemovePointer::Type; + using PtrType = ValueType*; + PtrType t; + + RetainPtr() : t(nullptr) { } + + RetainPtr(PtrType t) + : t(t) { + if (t) + CFRetain(t); + } + RetainPtr(RetainPtr&& o) + : RetainPtr(o.t) + { + o.t = nullptr; + } + RetainPtr(const RetainPtr& o) + : RetainPtr(o.t) + { + } + RetainPtr operator=(const RetainPtr& o) + { + if (t) + CFRelease(t); + t = o.t; + if (t) + CFRetain(t); + return *this; + } + ~RetainPtr() { + clear(); + } + void clear() { + if (t) + CFRelease(t); + t = nullptr; + } + void swap(RetainPtr& o) { + PtrType tmp = t; + t = o.t; + o.t = tmp; + } + PtrType get() const { return t; } + PtrType operator->() const { return t; } + T &operator*() const { return *t; } + RetainPtr &operator=(PtrType t) { + RetainPtr o(t); + swap(o); + return *this; + } + operator PtrType() const { return t; } + operator bool() const { return t; } + +private: + template friend RetainPtr adoptNS(U*); + template friend RetainPtr adoptCF(U); + + enum AdoptTag { Adopt }; + RetainPtr(PtrType t, AdoptTag) : t(t) { } +}; + +template +RetainPtr adoptNS(T* t) { + return RetainPtr(t, RetainPtr::Adopt); +} + +template +RetainPtr adoptCF(T t) { + return RetainPtr(t, RetainPtr::Adopt); +} + +} + +using WTF::RetainPtr; +using WTF::adoptNS; +using WTF::adoptCF; +using WTF::downcast; diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-local-vars-arc.mm b/clang/test/Analysis/Checkers/WebKit/unretained-local-vars-arc.mm new file mode 100644 index 0000000000000..92a718f7e3a4c --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/unretained-local-vars-arc.mm @@ -0,0 +1,46 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UnretainedLocalVarsChecker -fobjc-arc -verify %s + +#import "objc-mock-types.h" + +SomeObj *provide(); +void someFunction(); + +namespace raw_ptr { + +void foo() { + SomeObj *bar = [[SomeObj alloc] init]; + [bar doWork]; +} + +void foo2() { + SomeObj *bar = provide(); + [bar doWork]; +} + +void bar() { + CFMutableArrayRef array = CFArrayCreateMutable(kCFAllocatorDefault, 10); + // expected-warning@-1{{Local variable 'array' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + CFArrayAppendValue(array, nullptr); +} + +} // namespace raw_ptr + +@interface AnotherObj : NSObject +- (void)foo:(SomeObj *)obj; +@end + +@implementation AnotherObj +- (void)foo:(SomeObj*)obj { + SomeObj* obj2 = obj; + SomeObj* obj3 = provide(); + obj = nullptr; + [obj2 doWork]; + [obj3 doWork]; +} + +- (void)bar { + CFMutableArrayRef array = CFArrayCreateMutable(kCFAllocatorDefault, 10); + // expected-warning@-1{{Local variable 'array' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + CFArrayAppendValue(array, nullptr); +} +@end diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm b/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm new file mode 100644 index 0000000000000..21ef6a5dca519 --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm @@ -0,0 +1,373 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UnretainedLocalVarsChecker -verify %s + +#import "objc-mock-types.h" + +void someFunction(); + +namespace raw_ptr { +void foo() { + SomeObj *bar; + // FIXME: later on we might warn on uninitialized vars too +} + +void bar(SomeObj *) {} +} // namespace raw_ptr + +namespace pointer { +void foo_ref() { + SomeObj *bar = [[SomeObj alloc] init]; + // expected-warning@-1{{Local variable 'bar' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + [bar doWork]; +} + +bool bar_ref(SomeObj *obj) { + return !!obj; +} + +void cf_ptr() { + CFMutableArrayRef array = CFArrayCreateMutable(kCFAllocatorDefault, 10); + // expected-warning@-1{{Local variable 'array' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + CFArrayAppendValue(array, nullptr); +} +} // namespace pointer + +namespace guardian_scopes { +void foo1() { + RetainPtr foo; + { + SomeObj *bar = foo.get(); + } +} + +void foo2() { + RetainPtr foo; + // missing embedded scope here + SomeObj *bar = foo.get(); + // expected-warning@-1{{Local variable 'bar' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + [bar doWork]; +} + +void foo3() { + RetainPtr foo; + { + { SomeObj *bar = foo.get(); } + } +} + +void foo4() { + { + RetainPtr foo; + { SomeObj *bar = foo.get(); } + } +} + +struct SelfReferencingStruct { + SelfReferencingStruct* ptr; + SomeObj* obj { nullptr }; +}; + +void foo7(SomeObj* obj) { + SelfReferencingStruct bar = { &bar, obj }; + [bar.obj doWork]; +} + +void foo8(SomeObj* obj) { + RetainPtr foo; + + { + SomeObj *bar = foo.get(); + // expected-warning@-1{{Local variable 'bar' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + foo = nullptr; + [bar doWork]; + } + RetainPtr baz; + { + SomeObj *bar = baz.get(); + // expected-warning@-1{{Local variable 'bar' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + baz = obj; + [bar doWork]; + } + + foo = nullptr; + { + SomeObj *bar = foo.get(); + // No warning. It's okay to mutate RefPtr in an outer scope. + [bar doWork]; + } + foo = obj; + { + SomeObj *bar = foo.get(); + // expected-warning@-1{{Local variable 'bar' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + foo.clear(); + [bar doWork]; + } + { + SomeObj *bar = foo.get(); + // expected-warning@-1{{Local variable 'bar' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + foo = obj ? obj : nullptr; + [bar doWork]; + } + { + SomeObj *bar = [foo.get() other] ? foo.get() : nullptr; + // expected-warning@-1{{Local variable 'bar' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + foo = nullptr; + [bar doWork]; + } +} + +void foo9(SomeObj* o) { + RetainPtr guardian(o); + { + SomeObj *bar = guardian.get(); + // expected-warning@-1{{Local variable 'bar' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + guardian = o; // We don't detect that we're setting it to the same value. + [bar doWork]; + } + { + SomeObj *bar = guardian.get(); + // expected-warning@-1{{Local variable 'bar' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + RetainPtr other(bar); // We don't detect other has the same value as guardian. + guardian.swap(other); + [bar doWork]; + } + { + SomeObj *bar = guardian.get(); + // expected-warning@-1{{Local variable 'bar' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + RetainPtr other(static_cast&&>(guardian)); + [bar doWork]; + } + { + SomeObj *bar = guardian.get(); + // expected-warning@-1{{Local variable 'bar' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + guardian.clear(); + [bar doWork]; + } + { + SomeObj *bar = guardian.get(); + // expected-warning@-1{{Local variable 'bar' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + guardian = [o other] ? o : bar; + [bar doWork]; + } +} + +bool trivialFunction(CFMutableArrayRef array) { return !!array; } +void foo10() { + RetainPtr array = adoptCF(CFArrayCreateMutable(kCFAllocatorDefault, 10)); + { + CFMutableArrayRef arrayRef = array.get(); + CFArrayAppendValue(arrayRef, nullptr); + } + { + CFMutableArrayRef arrayRef = array.get(); + // expected-warning@-1{{Local variable 'arrayRef' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + array = nullptr; + CFArrayAppendValue(arrayRef, nullptr); + } + { + CFMutableArrayRef arrayRef = array.get(); + if (trivialFunction(arrayRef)) + arrayRef = nullptr; + } +} + +} // namespace guardian_scopes + +namespace auto_keyword { +class Foo { + SomeObj *provide_obj(); + CFMutableArrayRef provide_cf_array(); + void doWork(CFMutableArrayRef); + + void evil_func() { + SomeObj *bar = provide_obj(); + // expected-warning@-1{{Local variable 'bar' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + auto *baz = provide_obj(); + // expected-warning@-1{{Local variable 'baz' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + auto *baz2 = this->provide_obj(); + // expected-warning@-1{{Local variable 'baz2' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + [[clang::suppress]] auto *baz_suppressed = provide_obj(); // no-warning + } + + void func() { + SomeObj *bar = provide_obj(); + // expected-warning@-1{{Local variable 'bar' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + if (bar) + [bar doWork]; + } + + void bar() { + auto bar = provide_cf_array(); + // expected-warning@-1{{Local variable 'bar' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + doWork(bar); + [[clang::suppress]] auto baz = provide_cf_array(); // no-warning + doWork(baz); + } + +}; +} // namespace auto_keyword + +namespace guardian_casts { +void foo1() { + RetainPtr foo; + { + SomeObj *bar = downcast(foo.get()); + [bar doWork]; + } +} + +void foo2() { + RetainPtr foo; + { + SomeObj *bar = static_cast(downcast(foo.get())); + someFunction(); + } +} +} // namespace guardian_casts + +namespace conditional_op { +SomeObj *provide_obj(); +bool bar(); + +void foo() { + SomeObj *a = bar() ? nullptr : provide_obj(); + // expected-warning@-1{{Local variable 'a' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + RetainPtr b = provide_obj(); + { + SomeObj* c = bar() ? nullptr : b.get(); + [c doWork]; + SomeObj* d = bar() ? b.get() : nullptr; + [d doWork]; + } +} + +} // namespace conditional_op + +namespace local_assignment_basic { + +SomeObj *provide_obj(); + +void foo(SomeObj* a) { + SomeObj* b = a; + // expected-warning@-1{{Local variable 'b' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + if ([b other]) + b = provide_obj(); +} + +void bar(SomeObj* a) { + SomeObj* b; + // expected-warning@-1{{Local variable 'b' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + b = provide_obj(); +} + +void baz() { + RetainPtr a = provide_obj(); + { + SomeObj* b = a.get(); + // expected-warning@-1{{Local variable 'b' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + b = provide_obj(); + } +} + +} // namespace local_assignment_basic + +namespace local_assignment_to_parameter { + +SomeObj *provide_obj(); +void someFunction(); + +void foo(SomeObj* a) { + a = provide_obj(); + // expected-warning@-1{{Assignment to an unretained parameter 'a' is unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + someFunction(); + [a doWork]; +} + +CFMutableArrayRef provide_cf_array(); +void doWork(CFMutableArrayRef); + +void bar(CFMutableArrayRef a) { + a = provide_cf_array(); + // expected-warning@-1{{Assignment to an unretained parameter 'a' is unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + doWork(a); +} + +} // namespace local_assignment_to_parameter + +namespace local_assignment_to_static_local { + +SomeObj *provide_obj(); +void someFunction(); + +void foo() { + static SomeObj* a = nullptr; + // expected-warning@-1{{Static local variable 'a' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + a = provide_obj(); + someFunction(); + [a doWork]; +} + +CFMutableArrayRef provide_cf_array(); +void doWork(CFMutableArrayRef); + +void bar() { + static CFMutableArrayRef a = nullptr; + // expected-warning@-1{{Static local variable 'a' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + a = provide_cf_array(); + doWork(a); +} + +} // namespace local_assignment_to_static_local + +namespace local_assignment_to_global { + +SomeObj *provide_obj(); +void someFunction(); + +SomeObj* g_a = nullptr; +// expected-warning@-1{{Global variable 'local_assignment_to_global::g_a' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + +void foo() { + g_a = provide_obj(); + someFunction(); + [g_a doWork]; +} + +CFMutableArrayRef provide_cf_array(); +void doWork(CFMutableArrayRef); + +CFMutableArrayRef g_b = nullptr; +// expected-warning@-1{{Global variable 'local_assignment_to_global::g_b' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}} + +void bar() { + g_b = provide_cf_array(); + doWork(g_b); +} + +} // namespace local_assignment_to_global + +namespace local_var_for_singleton { + SomeObj *singleton(); + SomeObj *otherSingleton(); + void foo() { + SomeObj* bar = singleton(); + SomeObj* baz = otherSingleton(); + } + + CFMutableArrayRef cfSingleton(); + void bar() { + CFMutableArrayRef cf = cfSingleton(); + } +} + +bool doMoreWorkOpaque(OtherObj*); + +@implementation OtherObj +- (instancetype)init { + self = [super init]; + return self; +} + +- (void)doMoreWork:(OtherObj *)other { + doMoreWorkOpaque(other); +} +@end \ No newline at end of file