Support ptr to ptr and union in webkit member checker

clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp

@@ -38,9 +38,7 @@ class RawPtrRefMemberChecker
   RawPtrRefMemberChecker(const char *description)
       : Bug(this, description, "WebKit coding guidelines") {}
 
-  virtual std::optional<bool>
-  isPtrCompatible(const clang::QualType,
-                  const clang::CXXRecordDecl *R) const = 0;
+  virtual std::optional<bool> isUnsafePtr(QualType) const = 0;
   virtual const char *typeName() const = 0;
   virtual const char *invariant() const = 0;
 
@@ -87,26 +85,44 @@ class RawPtrRefMemberChecker
     if (shouldSkipDecl(RD))
       return;
 
-    for (auto *Member : RD->fields()) {
-      auto QT = Member->getType();
-      const Type *MemberType = QT.getTypePtrOrNull();
-      if (!MemberType)
-        continue;
-
-      if (auto *MemberCXXRD = MemberType->getPointeeCXXRecordDecl()) {
-        std::optional<bool> IsCompatible = isPtrCompatible(QT, MemberCXXRD);
-        if (IsCompatible && *IsCompatible)
-          reportBug(Member, MemberType, MemberCXXRD, RD);
-      } else {
-        std::optional<bool> IsCompatible = isPtrCompatible(QT, nullptr);
-        auto *PointeeType = MemberType->getPointeeType().getTypePtrOrNull();
-        if (IsCompatible && *IsCompatible) {
-          auto *Desugared = PointeeType->getUnqualifiedDesugaredType();
-          if (auto *ObjCType = dyn_cast_or_null<ObjCInterfaceType>(Desugared))
-            reportBug(Member, MemberType, ObjCType->getDecl(), RD);
-        }
-      }
+    for (auto *Member : RD->fields())
+      visitMember(Member, RD);
+  }
+
+  void visitMember(const FieldDecl *Member, const RecordDecl *RD) const {
+    auto QT = Member->getType();
+    const Type *MemberType = QT.getTypePtrOrNull();
+
+    while (MemberType) {
+      auto IsUnsafePtr = isUnsafePtr(QT);
+      if (IsUnsafePtr && *IsUnsafePtr)
+        break;
+      if (!MemberType->isPointerType())
+        return;
+      QT = MemberType->getPointeeType();
+      MemberType = QT.getTypePtrOrNull();
     }
+
+    if (!MemberType)
+      return;
+
+    if (auto *MemberCXXRD = MemberType->getPointeeCXXRecordDecl())
+      reportBug(Member, MemberType, MemberCXXRD, RD);
+    else if (auto *ObjCDecl = getObjCDecl(MemberType))
+      reportBug(Member, MemberType, ObjCDecl, RD);
+  }
+
+  ObjCInterfaceDecl *getObjCDecl(const Type *TypePtr) const {
+    auto *PointeeType = TypePtr->getPointeeType().getTypePtrOrNull();
+    if (!PointeeType)
+      return nullptr;
+    auto *Desugared = PointeeType->getUnqualifiedDesugaredType();
+    if (!Desugared)
+      return nullptr;
+    auto *ObjCType = dyn_cast<ObjCInterfaceType>(Desugared);
+    if (!ObjCType)
+      return nullptr;
+    return ObjCType->getDecl();
   }
 
   void visitObjCDecl(const ObjCContainerDecl *CD) const {
@@ -138,19 +154,15 @@ class RawPtrRefMemberChecker
     const Type *IvarType = QT.getTypePtrOrNull();
     if (!IvarType)
       return;
-    if (auto *IvarCXXRD = IvarType->getPointeeCXXRecordDecl()) {
-      std::optional<bool> IsCompatible = isPtrCompatible(QT, IvarCXXRD);
-      if (IsCompatible && *IsCompatible)
-        reportBug(Ivar, IvarType, IvarCXXRD, CD);
-    } else {
-      std::optional<bool> IsCompatible = isPtrCompatible(QT, nullptr);
-      auto *PointeeType = IvarType->getPointeeType().getTypePtrOrNull();
-      if (IsCompatible && *IsCompatible) {
-        auto *Desugared = PointeeType->getUnqualifiedDesugaredType();
-        if (auto *ObjCType = dyn_cast_or_null<ObjCInterfaceType>(Desugared))
-          reportBug(Ivar, IvarType, ObjCType->getDecl(), CD);
-      }
-    }
+
+    auto IsUnsafePtr = isUnsafePtr(QT);
+    if (!IsUnsafePtr || !*IsUnsafePtr)
+      return;
+
+    if (auto *MemberCXXRD = IvarType->getPointeeCXXRecordDecl())
+      reportBug(Ivar, IvarType, MemberCXXRD, CD);
+    else if (auto *ObjCDecl = getObjCDecl(IvarType))
+      reportBug(Ivar, IvarType, ObjCDecl, CD);
   }
 
   void visitObjCPropertyDecl(const ObjCContainerDecl *CD,
@@ -161,19 +173,15 @@ class RawPtrRefMemberChecker
     const Type *PropType = QT.getTypePtrOrNull();
     if (!PropType)
       return;
-    if (auto *PropCXXRD = PropType->getPointeeCXXRecordDecl()) {
-      std::optional<bool> IsCompatible = isPtrCompatible(QT, PropCXXRD);
-      if (IsCompatible && *IsCompatible)
-        reportBug(PD, PropType, PropCXXRD, CD);
-    } else {
-      std::optional<bool> IsCompatible = isPtrCompatible(QT, nullptr);
-      auto *PointeeType = PropType->getPointeeType().getTypePtrOrNull();
-      if (IsCompatible && *IsCompatible) {
-        auto *Desugared = PointeeType->getUnqualifiedDesugaredType();
-        if (auto *ObjCType = dyn_cast_or_null<ObjCInterfaceType>(Desugared))
-          reportBug(PD, PropType, ObjCType->getDecl(), CD);
-      }
-    }
+
+    auto IsUnsafePtr = isUnsafePtr(QT);
+    if (!IsUnsafePtr || !*IsUnsafePtr)
+      return;
+
+    if (auto *MemberCXXRD = PropType->getPointeeCXXRecordDecl())
+      reportBug(PD, PropType, MemberCXXRD, CD);
+    else if (auto *ObjCDecl = getObjCDecl(PropType))
+      reportBug(PD, PropType, ObjCDecl, CD);
   }
 
   bool shouldSkipDecl(const RecordDecl *RD) const {
@@ -194,7 +202,8 @@ class RawPtrRefMemberChecker
 
     const auto Kind = RD->getTagKind();
     // FIMXE: Should we check union members too?
-    if (Kind != TagTypeKind::Struct && Kind != TagTypeKind::Class)
+    if (Kind != TagTypeKind::Struct && Kind != TagTypeKind::Class &&
+        Kind != TagTypeKind::Union)
       return true;
 
     // Ignore CXXRecords that come from system headers.
@@ -231,7 +240,10 @@ class RawPtrRefMemberChecker
     printQuotedName(Os, Member);
     Os << " in ";
     printQuotedQualifiedName(Os, ClassCXXRD);
-    Os << " is a ";
+    if (Member->getType().getTypePtrOrNull() == MemberType)
+      Os << " is a ";
+    else
+      Os << " contains a ";
     if (printPointer(Os, MemberType) == PrintDeclKind::Pointer) {
       auto Typedef = MemberType->getAs<TypedefType>();
       assert(Typedef);
@@ -263,10 +275,8 @@ class NoUncountedMemberChecker final : public RawPtrRefMemberChecker {
       : RawPtrRefMemberChecker("Member variable is a raw-pointer/reference to "
                                "reference-countable type") {}
 
-  std::optional<bool>
-  isPtrCompatible(const clang::QualType,
-                  const clang::CXXRecordDecl *R) const final {
-    return R ? isRefCountable(R) : std::nullopt;
+  std::optional<bool> isUnsafePtr(QualType QT) const final {
+    return isUncountedPtr(QT.getCanonicalType());
   }
 
   const char *typeName() const final { return "ref-countable type"; }
@@ -282,10 +292,8 @@ class NoUncheckedPtrMemberChecker final : public RawPtrRefMemberChecker {
       : RawPtrRefMemberChecker("Member variable is a raw-pointer/reference to "
                                "checked-pointer capable type") {}
 
-  std::optional<bool>
-  isPtrCompatible(const clang::QualType,
-                  const clang::CXXRecordDecl *R) const final {
-    return R ? isCheckedPtrCapable(R) : std::nullopt;
+  std::optional<bool> isUnsafePtr(QualType QT) const final {
+    return isUncheckedPtr(QT.getCanonicalType());
   }
 
   const char *typeName() const final { return "CheckedPtr capable type"; }
@@ -304,9 +312,7 @@ class NoUnretainedMemberChecker final : public RawPtrRefMemberChecker {
     RTC = RetainTypeChecker();
   }
 
-  std::optional<bool>
-  isPtrCompatible(const clang::QualType QT,
-                  const clang::CXXRecordDecl *) const final {
+  std::optional<bool> isUnsafePtr(QualType QT) const final {
     return RTC->isUnretained(QT);
   }
 

clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp

@@ -34,22 +34,24 @@ namespace members {
 
 } // namespace members
 
-namespace ignore_unions {
+namespace unions {
 
   union Foo {
     CheckedObj* a;
+    // expected-warning@-1{{Member variable 'a' in 'unions::Foo' is a raw pointer to CheckedPtr capable type 'CheckedObj'}}
     CheckedPtr<CheckedObj> c;
     CheckedRef<CheckedObj> d;
   };
 
   template<class T>
   union FooTmpl {
     T* a;
+    // expected-warning@-1{{Member variable 'a' in 'unions::FooTmpl<CheckedObj>' is a raw pointer to CheckedPtr capable type 'CheckedObj'}}
   };
 
   void forceTmplToInstantiate(FooTmpl<CheckedObj>) { }
 
-} // namespace ignore_unions
+} // namespace unions
 
 namespace checked_ptr_ref_ptr_capable {
 
@@ -59,3 +61,23 @@ namespace checked_ptr_ref_ptr_capable {
   }
 
 } // checked_ptr_ref_ptr_capable
+
+namespace ptr_to_ptr_to_checked_ptr_capable {
+
+  struct List {
+    CheckedObj** elements;
+    // expected-warning@-1{{Member variable 'elements' in 'ptr_to_ptr_to_checked_ptr_capable::List' contains a raw pointer to CheckedPtr capable type 'CheckedObj'}}
+  };
+
+  template <typename T>
+  struct TemplateList {
+    T** elements;
+    // expected-warning@-1{{Member variable 'elements' in 'ptr_to_ptr_to_checked_ptr_capable::TemplateList<CheckedObj>' contains a raw pointer to CheckedPtr capable type 'CheckedObj'}}
+  };
+  TemplateList<CheckedObj> list;
+
+  struct SafeList {
+    CheckedPtr<CheckedObj>* elements;
+  };
+
+} // namespace ptr_to_ptr_to_checked_ptr_capable

clang/test/Analysis/Checkers/WebKit/uncounted-members.cpp

@@ -36,20 +36,22 @@ namespace members {
   };
 } // members
 
-namespace ignore_unions {
+namespace unions {
   union Foo {
     RefCountable* a;
+    // expected-warning@-1{{Member variable 'a' in 'unions::Foo' is a raw pointer to ref-countable type 'RefCountable'}}
     RefPtr<RefCountable> b;
     Ref<RefCountable> c;
   };
 
   template<class T>
-  union RefPtr {
+  union FooTmpl {
     T* a;
+    // expected-warning@-1{{Member variable 'a' in 'unions::FooTmpl<RefCountable>' is a raw pointer to ref-countable type 'RefCountable'}}
   };
 
-  void forceTmplToInstantiate(RefPtr<RefCountable>) {}
-} // ignore_unions
+  void forceTmplToInstantiate(FooTmpl<RefCountable>) {}
+} // unions
 
 namespace ignore_system_header {
 
@@ -77,3 +79,23 @@ namespace checked_ptr_ref_ptr_capable {
   }
 
 } // checked_ptr_ref_ptr_capable
+
+namespace ptr_to_ptr_to_ref_counted {
+
+  struct List {
+    RefCountable** elements;
+    // expected-warning@-1{{Member variable 'elements' in 'ptr_to_ptr_to_ref_counted::List' contains a raw pointer to ref-countable type 'RefCountable'}}
+  };
+
+  template <typename T>
+  struct TemplateList {
+    T** elements;
+    // expected-warning@-1{{Member variable 'elements' in 'ptr_to_ptr_to_ref_counted::TemplateList<RefCountable>' contains a raw pointer to ref-countable type 'RefCountable'}}
+  };
+  TemplateList<RefCountable> list;
+
+  struct SafeList {
+    RefPtr<RefCountable>* elements;
+  };
+
+} // namespace ptr_to_ptr_to_ref_counted

clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm

@@ -21,6 +21,12 @@
     CFMutableArrayRef e = nullptr;
 // expected-warning@-1{{Member variable 'e' in 'members::Foo' is a retainable type 'CFMutableArrayRef'}}
   };
+
+  union FooUnion {
+    SomeObj* a;
+    CFMutableArrayRef b;
+    // expected-warning@-1{{Member variable 'b' in 'members::FooUnion' is a retainable type 'CFMutableArrayRef'}}
+  };
 
   template<class T, class S>
   struct FooTmpl {
@@ -37,3 +43,24 @@ void forceTmplToInstantiate(FooTmpl<SomeObj, CFMutableArrayRef>) {}
   };
 
 }
+
+namespace ptr_to_ptr_to_retained {
+
+  struct List {
+    CFMutableArrayRef* elements2;
+    // expected-warning@-1{{Member variable 'elements2' in 'ptr_to_ptr_to_retained::List' contains a retainable type 'CFMutableArrayRef'}}
+  };
+
+  template <typename T, typename S>
+  struct TemplateList {
+    S* elements2;
+    // expected-warning@-1{{Member variable 'elements2' in 'ptr_to_ptr_to_retained::TemplateList<SomeObj, __CFArray *>' contains a raw pointer to retainable type '__CFArray'}}
+  };
+  TemplateList<SomeObj, CFMutableArrayRef> list;
+
+  struct SafeList {
+    RetainPtr<SomeObj>* elements1;
+    RetainPtr<CFMutableArrayRef>* elements2;
+  };
+
+} // namespace ptr_to_ptr_to_retained

clang/test/Analysis/Checkers/WebKit/unretained-members.mm

@@ -47,21 +47,49 @@ void forceTmplToInstantiate(FooTmpl<SomeObj, CFMutableArrayRef>) {}
 
 }
 
-namespace ignore_unions {
+namespace unions {
   union Foo {
     SomeObj* a;
+    // expected-warning@-1{{Member variable 'a' in 'unions::Foo' is a raw pointer to retainable type 'SomeObj'}}
     RetainPtr<SomeObj> b;
     CFMutableArrayRef c;
+    // expected-warning@-1{{Member variable 'c' in 'unions::Foo' is a retainable type 'CFMutableArrayRef'}}
   };
 
   template<class T>
-  union RefPtr {
+  union FooTempl {
     T* a;
+    // expected-warning@-1{{Member variable 'a' in 'unions::FooTempl<SomeObj>' is a raw pointer to retainable type 'SomeObj'}}
   };
 
-  void forceTmplToInstantiate(RefPtr<SomeObj>) {}
+  void forceTmplToInstantiate(FooTempl<SomeObj>) {}
 }
 
+namespace ptr_to_ptr_to_retained {
+
+  struct List {
+    SomeObj** elements1;
+    // expected-warning@-1{{Member variable 'elements1' in 'ptr_to_ptr_to_retained::List' contains a raw pointer to retainable type 'SomeObj'}}
+    CFMutableArrayRef* elements2;
+    // expected-warning@-1{{Member variable 'elements2' in 'ptr_to_ptr_to_retained::List' contains a retainable type 'CFMutableArrayRef'}}
+  };
+
+  template <typename T, typename S>
+  struct TemplateList {
+    T** elements1;
+    // expected-warning@-1{{Member variable 'elements1' in 'ptr_to_ptr_to_retained::TemplateList<SomeObj, __CFArray *>' contains a raw pointer to retainable type 'SomeObj'}}
+    S* elements2;
+    // expected-warning@-1{{Member variable 'elements2' in 'ptr_to_ptr_to_retained::TemplateList<SomeObj, __CFArray *>' contains a raw pointer to retainable type '__CFArray'}}
+  };
+  TemplateList<SomeObj, CFMutableArrayRef> list;
+
+  struct SafeList {
+    RetainPtr<SomeObj>* elements1;
+    RetainPtr<CFMutableArrayRef>* elements2;
+  };
+
+} // namespace ptr_to_ptr_to_retained
+
 @interface AnotherObject : NSObject {
   NSString *ns_string;
   // expected-warning@-1{{Instance variable 'ns_string' in 'AnotherObject' is a raw pointer to retainable type 'NSString'; member variables must be a RetainPtr}}