Check and warn if public destructors are found on types that derive from...

Check and warn if public destructors are found on types that derive from base::RefCounted or base::RefCountedThreadSafe.

Having public destructors is dangerous, as it allows the ref-counted object to be stack allocated and have references that attempt to outlive the stack, or to allow callers to explicitly delete it while references are still held. Both patterns result in use-after-free, hence their prohibition.

In doing so, update the Chromium plugin to be able to scan both headers and implementation files, and enable the public destructor check for both types with an optional flag ( -Xclang -plugin-arg-find-bad-constructs -Xclang skip-refcounted-dtors )

BUG=123295
TEST=none

Review URL: https://chromiumcodereview.appspot.com/10005022

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@132500 0039d316-1c4b-4281-b951-d872f2087c98

tools/clang/plugins/ChromeClassTester.h

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -21,8 +21,6 @@ class ChromeClassTester : public clang::ASTConsumer {
   explicit ChromeClassTester(clang::CompilerInstance& instance);
   virtual ~ChromeClassTester();
 
-  void BuildBannedLists();
-
   // ASTConsumer:
   virtual void HandleTagDeclDefinition(clang::TagDecl* tag);
 
@@ -34,29 +32,38 @@ class ChromeClassTester : public clang::ASTConsumer {
   // printing.
   void emitWarning(clang::SourceLocation loc, const char* error);
 
-  // Utility method for subclasses to check if testing details are in this
-  // class. Some tests won't care if a class has a ::testing member and others
-  // will.
-  bool InTestingNamespace(const clang::Decl* record);
-
   // Utility method for subclasses to check if this class is in a banned
   // namespace.
   bool InBannedNamespace(const clang::Decl* record);
 
+  // Utility method for subclasses to determine the namespace of the
+  // specified record, if any. Unnamed namespaces will be identified as
+  // "<anonymous namespace>".
+  std::string GetNamespace(const clang::Decl* record);
+
+  // Utility method for subclasses to check if this class is within an
+  // implementation (.cc, .cpp, .mm) file.
+  bool InImplementationFile(clang::SourceLocation location);
+
  private:
+  void BuildBannedLists();
+
   // Filtered versions of tags that are only called with things defined in
   // chrome header files.
-  virtual void CheckChromeClass(const clang::SourceLocation& record_location,
+  virtual void CheckChromeClass(clang::SourceLocation record_location,
                                 clang::CXXRecordDecl* record) = 0;
 
   // Utility methods used for filtering out non-chrome classes (and ones we
   // deliberately ignore) in HandleTagDeclDefinition().
-  std::string GetNamespace(const clang::Decl* record);
   std::string GetNamespaceImpl(const clang::DeclContext* context,
-                               std::string candidate);
+                               const std::string& candidate);
   bool InBannedDirectory(clang::SourceLocation loc);
   bool IsIgnoredType(const std::string& base_name);
 
+  // Attempts to determine the filename for the given SourceLocation.
+  // Returns false if the filename could not be determined.
+  bool GetFilename(clang::SourceLocation loc, std::string* filename);
+
   clang::CompilerInstance& instance_;
   clang::DiagnosticsEngine& diagnostic_;
 

tools/clang/plugins/FindBadConstructs.cpp

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -10,14 +10,13 @@
 // - Missing "virtual" keywords on methods that should be virtual.
 // - Non-annotated overriding virtual methods.
 // - Virtual methods with nonempty implementations in their headers.
-//
-// Things that are still TODO:
-// - Deriving from base::RefCounted and friends should mandate non-public
-//   destructors.
+// - Classes that derive from base::RefCounted / base::RefCountedThreadSafe
+//   should have protected or private destructors.
 
 #include "clang/Frontend/FrontendPluginRegistry.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/AST.h"
+#include "clang/AST/CXXInheritance.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Frontend/CompilerInstance.h"
@@ -36,20 +35,109 @@ bool TypeHasNonTrivialDtor(const Type* type) {
   return false;
 }
 
+// Returns the underlying Type for |type| by expanding typedefs and removing
+// any namespace qualifiers.
+const Type* UnwrapType(const Type* type) {
+  if (const ElaboratedType* elaborated = dyn_cast<ElaboratedType>(type))
+    return UnwrapType(elaborated->getNamedType().getTypePtr());
+  if (const TypedefType* typedefed = dyn_cast<TypedefType>(type))
+    return UnwrapType(typedefed->desugar().getTypePtr());
+  return type;
+}
+
 // Searches for constructs that we know we don't want in the Chromium code base.
 class FindBadConstructsConsumer : public ChromeClassTester {
  public:
-  FindBadConstructsConsumer(CompilerInstance& instance)
-      : ChromeClassTester(instance) {}
+  FindBadConstructsConsumer(CompilerInstance& instance,
+                            bool check_refcounted_dtors)
+      : ChromeClassTester(instance),
+        check_refcounted_dtors_(check_refcounted_dtors) {
+  }
 
-  virtual void CheckChromeClass(const SourceLocation& record_location,
+  virtual void CheckChromeClass(SourceLocation record_location,
                                 CXXRecordDecl* record) {
-    CheckCtorDtorWeight(record_location, record);
-    CheckVirtualMethods(record_location, record);
+    bool implementation_file = InImplementationFile(record_location);
+
+    if (!implementation_file) {
+      // Only check for "heavy" constructors/destructors in header files;
+      // within implementation files, there is no performance cost.
+      CheckCtorDtorWeight(record_location, record);
+
+      // Check that all virtual methods are marked accordingly with both
+      // virtual and OVERRIDE.
+      CheckVirtualMethods(record_location, record);
+    }
+
+    if (check_refcounted_dtors_)
+      CheckRefCountedDtors(record_location, record);
+  }
+
+ private:
+  bool check_refcounted_dtors_;
+
+  // Returns true if |base| specifies one of the Chromium reference counted
+  // classes (base::RefCounted / base::RefCountedThreadSafe). |user_data| is
+  // ignored.
+  static bool IsRefCountedCallback(const CXXBaseSpecifier* base,
+                                   CXXBasePath& path,
+                                   void* user_data) {
+    FindBadConstructsConsumer* self =
+        static_cast<FindBadConstructsConsumer*>(user_data);
+
+    const TemplateSpecializationType* base_type =
+        dyn_cast<TemplateSpecializationType>(
+            UnwrapType(base->getType().getTypePtr()));
+    if (!base_type) {
+      // Base-most definition is not a template, so this cannot derive from
+      // base::RefCounted. However, it may still be possible to use with a
+      // scoped_refptr<> and support ref-counting, so this is not a perfect
+      // guarantee of safety.
+      return false;
+    }
+
+    TemplateName name = base_type->getTemplateName();
+    if (TemplateDecl* decl = name.getAsTemplateDecl()) {
+      std::string base_name = decl->getNameAsString();
+
+      // Check for both base::RefCounted and base::RefCountedThreadSafe.
+      if (base_name.compare(0, 10, "RefCounted") == 0 &&
+          self->GetNamespace(decl) == "base") {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  // Prints errors if the destructor of a RefCounted class is public.
+  void CheckRefCountedDtors(SourceLocation record_location,
+                            CXXRecordDecl* record) {
+    // Skip anonymous structs.
+    if (record->getIdentifier() == NULL)
+      return;
+
+    CXXBasePaths paths;
+    if (!record->lookupInBases(
+            &FindBadConstructsConsumer::IsRefCountedCallback, this, paths)) {
+      return;  // Class does not derive from a ref-counted base class.
+    }
+
+    if (!record->hasUserDeclaredDestructor()) {
+      emitWarning(
+          record_location,
+          "Classes that are ref-counted should have explicit "
+          "destructors that are protected or private.");
+    } else if (CXXDestructorDecl* dtor = record->getDestructor()) {
+      if (dtor->getAccess() == AS_public) {
+        emitWarning(
+            dtor->getInnerLocStart(),
+            "Classes that are ref-counted should not have "
+            "public destructors.");
+      }
+    }
   }
 
   // Prints errors if the constructor/destructor weight is too heavy.
-  void CheckCtorDtorWeight(const SourceLocation& record_location,
+  void CheckCtorDtorWeight(SourceLocation record_location,
                            CXXRecordDecl* record) {
     // We don't handle anonymous structs. If this record doesn't have a
     // name, it's of the form:
@@ -161,6 +249,10 @@ class FindBadConstructsConsumer : public ChromeClassTester {
     }
   }
 
+  bool InTestingNamespace(const Decl* record) {
+    return GetNamespace(record).find("testing") != std::string::npos;
+  }
+
   bool IsMethodInBannedNamespace(const CXXMethodDecl* method) {
     if (InBannedNamespace(method))
       return true;
@@ -196,7 +288,7 @@ class FindBadConstructsConsumer : public ChromeClassTester {
   // trick to get around that. If a class has member variables whose types are
   // in the "testing" namespace (which is how gmock works behind the scenes),
   // there's a really high chance we won't care about these errors
-  void CheckVirtualMethods(const SourceLocation& record_location,
+  void CheckVirtualMethods(SourceLocation record_location,
                            CXXRecordDecl* record) {
     for (CXXRecordDecl::field_iterator it = record->field_begin();
          it != record->field_end(); ++it) {
@@ -283,16 +375,32 @@ class FindBadConstructsConsumer : public ChromeClassTester {
 };
 
 class FindBadConstructsAction : public PluginASTAction {
+ public:
+  FindBadConstructsAction() : check_refcounted_dtors_(true) {}
+
  protected:
   ASTConsumer* CreateASTConsumer(CompilerInstance &CI, llvm::StringRef ref) {
-    return new FindBadConstructsConsumer(CI);
+    return new FindBadConstructsConsumer(CI, check_refcounted_dtors_);
   }
 
   bool ParseArgs(const CompilerInstance &CI,
                  const std::vector<std::string>& args) {
-    // We don't take any additional arguments here.
-    return true;
+    bool parsed = true;
+
+    for (size_t i = 0; i < args.size() && parsed; ++i) {
+      if (args[i] == "skip-refcounted-dtors") {
+        check_refcounted_dtors_ = false;
+      } else {
+        parsed = false;
+        llvm::errs() << "Unknown argument: " << args[i] << "\n";
+      }
+    }
+
+    return parsed;
   }
+
+ private:
+  bool check_refcounted_dtors_;
 };
 
 }  // namespace

tools/clang/plugins/tests/base_refcounted.cpp

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base_refcounted.h"
+
+#include <cstddef>
+
+namespace {
+
+// Unsafe; should error.
+class AnonymousDerivedProtectedToPublicInImpl
+    : public ProtectedRefCountedDtorInHeader {
+ public:
+  AnonymousDerivedProtectedToPublicInImpl() {}
+  ~AnonymousDerivedProtectedToPublicInImpl() {}
+};
+
+}  // namespace
+
+// Unsafe; should error.
+class PublicRefCountedDtorInImpl
+    : public base::RefCounted<PublicRefCountedDtorInImpl> {
+ public:
+  PublicRefCountedDtorInImpl() {}
+  ~PublicRefCountedDtorInImpl() {}
+
+ private:
+  friend class base::RefCounted<PublicRefCountedDtorInImpl>;
+};
+
+class Foo {
+ public:
+  class BarInterface {
+   protected:
+    virtual ~BarInterface() {}
+  };
+
+  typedef base::RefCounted<BarInterface> RefCountedBar;
+  typedef RefCountedBar AnotherTypedef;
+};
+
+class Baz {
+ public:
+  typedef typename Foo::AnotherTypedef MyLocalTypedef;
+};
+
+// Unsafe; should error.
+class UnsafeTypedefChainInImpl : public Baz::MyLocalTypedef {
+ public:
+  UnsafeTypedefChainInImpl() {}
+  ~UnsafeTypedefChainInImpl() {}
+};
+
+int main() {
+  PublicRefCountedDtorInHeader bad;
+  PublicRefCountedDtorInImpl also_bad;
+
+  ProtectedRefCountedDtorInHeader* protected_ok = NULL;
+  PrivateRefCountedDtorInHeader* private_ok = NULL;
+
+  DerivedProtectedToPublicInHeader still_bad;
+  PublicRefCountedThreadSafeDtorInHeader another_bad_variation;
+  AnonymousDerivedProtectedToPublicInImpl and_this_is_bad_too;
+  ImplicitDerivedProtectedToPublicInHeader bad_yet_again;
+  UnsafeTypedefChainInImpl and_again_this_is_bad;
+
+  WebKitPublicDtorInHeader ignored;
+  WebKitDerivedPublicDtorInHeader still_ignored;
+
+  return 0;
+}

tools/clang/plugins/tests/base_refcounted.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_REFCOUNTED_H_
+#define BASE_REFCOUNTED_H_
+
+namespace base {
+
+template <typename T>
+class RefCounted {
+ public:
+  RefCounted() {}
+  ~RefCounted() {}
+};
+
+template <typename T>
+class RefCountedThreadSafe {
+ public:
+  RefCountedThreadSafe() {}
+  ~RefCountedThreadSafe() {}
+};
+
+}  // namespace base
+
+// Ignore classes whose inheritance tree ends in WebKit's RefCounted base
+// class. Though prone to error, this pattern is very prevalent in WebKit
+// code, so do not issue any warnings.
+namespace WebKit {
+
+template <typename T>
+class RefCounted {
+ public:
+  RefCounted() {}
+  ~RefCounted() {}
+};
+
+}  // namespace WebKit
+
+// Unsafe; should error.
+class PublicRefCountedDtorInHeader
+    : public base::RefCounted<PublicRefCountedDtorInHeader> {
+ public:
+  PublicRefCountedDtorInHeader() {}
+  ~PublicRefCountedDtorInHeader() {}
+
+ private:
+  friend class base::RefCounted<PublicRefCountedDtorInHeader>;
+};
+
+// Unsafe; should error.
+class PublicRefCountedThreadSafeDtorInHeader
+    : public base::RefCountedThreadSafe<
+          PublicRefCountedThreadSafeDtorInHeader> {
+ public:
+  PublicRefCountedThreadSafeDtorInHeader() {}
+  ~PublicRefCountedThreadSafeDtorInHeader() {}
+
+ private:
+  friend class base::RefCountedThreadSafe<
+      PublicRefCountedThreadSafeDtorInHeader>;
+};
+
+// Safe; should not have errors.
+class ProtectedRefCountedDtorInHeader
+    : public base::RefCounted<ProtectedRefCountedDtorInHeader> {
+ public:
+  ProtectedRefCountedDtorInHeader() {}
+
+ protected:
+  ~ProtectedRefCountedDtorInHeader() {}
+
+ private:
+  friend class base::RefCounted<ProtectedRefCountedDtorInHeader>;
+};
+
+// Safe; should not have errors.
+class PrivateRefCountedDtorInHeader
+    : public base::RefCounted<PrivateRefCountedDtorInHeader> {
+ public:
+  PrivateRefCountedDtorInHeader() {}
+
+ private:
+  ~PrivateRefCountedDtorInHeader() {}
+  friend class base::RefCounted<PrivateRefCountedDtorInHeader>;
+};
+
+// Unsafe; A grandchild class ends up exposing their parent and grandparent's
+// destructors.
+class DerivedProtectedToPublicInHeader
+    : public ProtectedRefCountedDtorInHeader {
+ public:
+  DerivedProtectedToPublicInHeader() {}
+  ~DerivedProtectedToPublicInHeader() {}
+};
+
+// Unsafe; A grandchild ends up implicitly exposing their parent and
+// grantparent's destructors.
+class ImplicitDerivedProtectedToPublicInHeader
+    : public ProtectedRefCountedDtorInHeader {
+ public:
+  ImplicitDerivedProtectedToPublicInHeader() {}
+};
+
+// Unsafe-but-ignored; should not have errors.
+class WebKitPublicDtorInHeader
+    : public WebKit::RefCounted<WebKitPublicDtorInHeader> {
+ public:
+  WebKitPublicDtorInHeader() {}
+  ~WebKitPublicDtorInHeader() {}
+};
+
+// Unsafe-but-ignored; should not have errors.
+class WebKitDerivedPublicDtorInHeader
+    : public WebKitPublicDtorInHeader {
+ public:
+  WebKitDerivedPublicDtorInHeader() {}
+  ~WebKitDerivedPublicDtorInHeader() {}
+};
+
+#endif  // BASE_REFCOUNTED_H_

tools/clang/plugins/tests/base_refcounted.txt

+In file included from base_refcounted.cpp:5:
+./base_refcounted.h:45:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
+  ~PublicRefCountedDtorInHeader() {}
+  ^
+./base_refcounted.h:57:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
+  ~PublicRefCountedThreadSafeDtorInHeader() {}
+  ^
+./base_refcounted.h:94:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
+  ~DerivedProtectedToPublicInHeader() {}
+  ^
+./base_refcounted.h:99:1: warning: [chromium-style] Classes that are ref-counted should have explicit destructors that are protected or private.
+class ImplicitDerivedProtectedToPublicInHeader
+^
+base_refcounted.cpp:16:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
+  ~AnonymousDerivedProtectedToPublicInImpl() {}
+  ^
+base_refcounted.cpp:26:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
+  ~PublicRefCountedDtorInImpl() {}
+  ^
+base_refcounted.cpp:52:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
+  ~UnsafeTypedefChainInImpl() {}
+  ^
+7 warnings generated.

tools/clang/plugins/tests/inline_ctor.h

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef INCLINE_CTOR_H_
-#define INCLINE_CTOR_H_
+#ifndef INLINE_CTOR_H_
+#define INLINE_CTOR_H_
 
 #include <string>
 #include <vector>
@@ -18,4 +18,4 @@ class InlineCtorsArentOKInHeader {
   std::vector<std::string> two_;
 };
 
-#endif  // INCLINE_CTOR_H_
+#endif  // INLINE_CTOR_H_

tools/clang/scripts/plugin_flags.sh

 #!/bin/bash
-# Copyright (c) 2011 The Chromium Authors. All rights reserved.
+# Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
@@ -17,4 +17,5 @@ else
 fi
 
 echo -Xclang -load -Xclang $CLANG_LIB_PATH/libFindBadConstructs.$LIBSUFFIX \
-  -Xclang -add-plugin -Xclang find-bad-constructs
+  -Xclang -add-plugin -Xclang find-bad-constructs \
+  -Xclang -plugin-arg-find-bad-constructs -Xclang skip-refcounted-dtors