Reland: Add check-bad-raw-ptr-cast to the clang plugin
change in reland: fix build error
Original description:
check-bad-raw-ptr-cast checks for raw_ptr<T>* being cast to another type.
This prevents issues like this: https://chromium-review.googlesource.com/c/chromium/src/+/2587397/9/base/memory/checked_ptr.md#230
As discussed in https://chat.google.com/room/AAAAdj-LwK0/QOr9OiIVv5M
Change-Id: I2da17fc4dbcb080f334871dc85be6d581e867ee5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3961908
Commit-Queue: Keishi Hattori <keishi@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Thomas Lukaszewicz <tluk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1066236}
diff --git a/tools/clang/plugins/BlinkDataMemberTypeChecker.cpp b/tools/clang/plugins/BlinkDataMemberTypeChecker.cpp
index f85b782c..e563619 100644
--- a/tools/clang/plugins/BlinkDataMemberTypeChecker.cpp
+++ b/tools/clang/plugins/BlinkDataMemberTypeChecker.cpp
@@ -51,7 +51,7 @@
void BlinkDataMemberTypeChecker::CheckClass(SourceLocation location,
const CXXRecordDecl* record) {
- std::string filename = GetFilename(instance_, location);
+ std::string filename = GetFilename(instance_.getSourceManager(), location);
if (!included_filenames_regex_.match(filename))
return;
if (excluded_filenames_regex_.match(filename))
@@ -147,7 +147,8 @@
// Similarly, stop finding the root underlying type if the intermediate
// type is defined in a file that should not be checked, e.g. in a file
// under third_party/blink/public/common.
- std::string filename = GetFilename(instance_, decl->getLocation());
+ std::string filename =
+ GetFilename(instance_.getSourceManager(), decl->getLocation());
if (!included_filenames_regex_.match(filename))
return;
}
diff --git a/tools/clang/plugins/CMakeLists.txt b/tools/clang/plugins/CMakeLists.txt
index 0c0c4aa..d07457f8 100644
--- a/tools/clang/plugins/CMakeLists.txt
+++ b/tools/clang/plugins/CMakeLists.txt
@@ -3,6 +3,7 @@
ChromeClassTester.cpp
FindBadConstructsAction.cpp
FindBadConstructsConsumer.cpp
+ FindBadRawPtrPatterns.cpp
CheckIPCVisitor.cpp
CheckLayoutObjectMethodsVisitor.cpp
Util.cpp
diff --git a/tools/clang/plugins/ChromeClassTester.cpp b/tools/clang/plugins/ChromeClassTester.cpp
index a76d59ff..603e2c8b 100644
--- a/tools/clang/plugins/ChromeClassTester.cpp
+++ b/tools/clang/plugins/ChromeClassTester.cpp
@@ -71,7 +71,7 @@
if (instance().getSourceManager().isInSystemHeader(loc))
return LocationType::kThirdParty;
- std::string filename = GetFilename(instance(), loc);
+ std::string filename = GetFilename(instance().getSourceManager(), loc);
if (filename.empty()) {
// If the filename cannot be determined, simply treat this as a banned
// location, instead of going through the full lookup process.
@@ -139,7 +139,7 @@
// If |record_location| is a macro, check the whole chain of expansions.
const SourceManager& source_manager = instance_.getSourceManager();
while (true) {
- filename = GetFilename(instance(), record_location);
+ filename = GetFilename(instance().getSourceManager(), record_location);
if (ends_with(filename, ".cc") || ends_with(filename, ".cpp") ||
ends_with(filename, ".mm")) {
return true;
diff --git a/tools/clang/plugins/FindBadConstructsAction.cpp b/tools/clang/plugins/FindBadConstructsAction.cpp
index 698b4a8..c6ea3bc 100644
--- a/tools/clang/plugins/FindBadConstructsAction.cpp
+++ b/tools/clang/plugins/FindBadConstructsAction.cpp
@@ -55,6 +55,8 @@
options_.check_layout_object_methods = true;
} else if (args[i] == "raw-ref-template-as-trivial-member") {
options_.raw_ref_template_as_trivial_member = true;
+ } else if (args[i] == "check-bad-raw-ptr-cast") {
+ options_.check_bad_raw_ptr_cast = true;
} else {
parsed = false;
llvm::errs() << "Unknown clang plugin argument: " << args[i] << "\n";
diff --git a/tools/clang/plugins/FindBadConstructsConsumer.cpp b/tools/clang/plugins/FindBadConstructsConsumer.cpp
index ac25fd7..3386c131 100644
--- a/tools/clang/plugins/FindBadConstructsConsumer.cpp
+++ b/tools/clang/plugins/FindBadConstructsConsumer.cpp
@@ -4,6 +4,7 @@
#include "FindBadConstructsConsumer.h"
+#include "FindBadRawPtrPatterns.h"
#include "Util.h"
#include "clang/AST/Attr.h"
#include "clang/Frontend/CompilerInstance.h"
@@ -230,6 +231,7 @@
RecursiveASTVisitor::TraverseDecl(context.getTranslationUnitDecl());
if (ipc_visitor_)
ipc_visitor_->set_context(nullptr);
+ FindBadRawPtrPatterns(options_, context, instance());
}
bool FindBadConstructsConsumer::TraverseDecl(Decl* decl) {
diff --git a/tools/clang/plugins/FindBadRawPtrPatterns.cpp b/tools/clang/plugins/FindBadRawPtrPatterns.cpp
new file mode 100644
index 0000000..7cbf997
--- /dev/null
+++ b/tools/clang/plugins/FindBadRawPtrPatterns.cpp
@@ -0,0 +1,105 @@
+// Copyright 2022 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "FindBadRawPtrPatterns.h"
+
+#include "Util.h"
+#include "clang/AST/AST.h"
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/Attr.h"
+#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/TypeLoc.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/Support/LineIterator.h"
+
+using namespace clang;
+using namespace clang::ast_matchers;
+
+namespace chrome_checker {
+
+const char kBadCastSignature[] =
+ "[chromium-raw-ptr-cast] Casting raw_ptr<T>* to another type is not "
+ "allowed as it may cause BRP ref count mismatch and bypass security "
+ "checks.";
+
+class BadCastMatcher : public MatchFinder::MatchCallback {
+ public:
+ explicit BadCastMatcher(clang::CompilerInstance& compiler)
+ : compiler_(compiler) {
+ error_bad_raw_ptr_cast_signature_ =
+ compiler_.getDiagnostics().getCustomDiagID(
+ clang::DiagnosticsEngine::Error, kBadCastSignature);
+ }
+
+ void Register(MatchFinder& match_finder) {
+ // TODO(keishi): Also find casts to and from classes that contain raw_ptr.
+ auto cast_matcher =
+ castExpr(
+ allOf(hasSourceExpression(hasType(pointerType(pointee(
+ hasUnqualifiedDesugaredType(recordType(hasDeclaration(
+ cxxRecordDecl(classTemplateSpecializationDecl(
+ hasName("base::raw_ptr")))))))))),
+ hasCastKind(CK_BitCast)))
+ .bind("castExpr");
+ match_finder.addMatcher(cast_matcher, this);
+ }
+
+ void run(const MatchFinder::MatchResult& result) override {
+ const clang::CastExpr* cast_expr =
+ result.Nodes.getNodeAs<clang::CastExpr>("castExpr");
+ assert(cast_expr && "matcher should bind 'castExpr'");
+
+ const clang::SourceManager& source_manager = *result.SourceManager;
+ clang::SourceLocation loc = cast_expr->getSourceRange().getBegin();
+ std::string file_path = GetFilename(source_manager, loc);
+
+ // Using raw_ptr<T> in a stdlib collection will cause a cast.
+ // e.g.
+ // https://source.chromium.org/chromium/chromium/src/+/main:components/feed/core/v2/xsurface_datastore.h;drc=a0ff03edcace35ec020edd235f4d9e9735fc9690;l=107
+ if (file_path.find("buildtools/third_party/libc++") != std::string::npos)
+ return;
+ // CHECK(raw_ptr<T>) will cause a cast.
+ // e.g.
+ // https://source.chromium.org/chromium/chromium/src/+/main:base/task/sequence_manager/thread_controller_with_message_pump_impl.cc;drc=c49b7434a9d4a61c49fc0123e904a6c5e7162731;l=121
+ if (file_path.find("base/check_op.h") != std::string::npos)
+ return;
+ // raw_ptr<T>* is cast to ui::metadata::PropertyKey
+ // https://source.chromium.org/chromium/chromium/src/+/main:ui/views/view.cc;drc=a0ff03edcace35ec020edd235f4d9e9735fc9690;l=2417
+ if (file_path.find("ui/views/controls/table/table_view.cc") !=
+ std::string::npos)
+ return;
+ // XdgActivation::activation_queue_ is a base::queue<raw_ptr> which causes a
+ // cast in VectorBuffer and circular_deque.
+ if (file_path.find("base/containers/vector_buffer.h") != std::string::npos)
+ return;
+ if (file_path.find("base/containers/circular_deque.h") != std::string::npos)
+ return;
+
+ compiler_.getDiagnostics().Report(cast_expr->getEndLoc(),
+ error_bad_raw_ptr_cast_signature_);
+ }
+
+ private:
+ clang::CompilerInstance& compiler_;
+ unsigned error_bad_raw_ptr_cast_signature_;
+};
+
+void FindBadRawPtrPatterns(Options options,
+ clang::ASTContext& ast_context,
+ clang::CompilerInstance& compiler) {
+ if (!options.check_bad_raw_ptr_cast)
+ return;
+ MatchFinder match_finder;
+
+ BadCastMatcher bad_cast_matcher(compiler);
+ bad_cast_matcher.Register(match_finder);
+
+ match_finder.matchAST(ast_context);
+}
+
+} // namespace chrome_checker
diff --git a/tools/clang/plugins/FindBadRawPtrPatterns.h b/tools/clang/plugins/FindBadRawPtrPatterns.h
new file mode 100644
index 0000000..6d10e8e
--- /dev/null
+++ b/tools/clang/plugins/FindBadRawPtrPatterns.h
@@ -0,0 +1,20 @@
+// Copyright 2022 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef TOOLS_CLANG_PLUGINS_FINDBADRAWPTRPATTERNS_H_
+#define TOOLS_CLANG_PLUGINS_FINDBADRAWPTRPATTERNS_H_
+
+#include "Options.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Frontend/CompilerInstance.h"
+
+namespace chrome_checker {
+
+void FindBadRawPtrPatterns(Options options,
+ clang::ASTContext& ast_context,
+ clang::CompilerInstance& compiler);
+
+} // namespace chrome_checker
+
+#endif // TOOLS_CLANG_PLUGINS_FINDBADRAWPTRPATTERNS_H_
diff --git a/tools/clang/plugins/Options.h b/tools/clang/plugins/Options.h
index e06a971..e016193 100644
--- a/tools/clang/plugins/Options.h
+++ b/tools/clang/plugins/Options.h
@@ -13,6 +13,7 @@
bool check_ipc = false;
bool check_layout_object_methods = false;
bool raw_ref_template_as_trivial_member = false;
+ bool check_bad_raw_ptr_cast = false;
};
} // namespace chrome_checker
diff --git a/tools/clang/plugins/Util.cpp b/tools/clang/plugins/Util.cpp
index 73641113..5b6ebb41 100644
--- a/tools/clang/plugins/Util.cpp
+++ b/tools/clang/plugins/Util.cpp
@@ -37,9 +37,8 @@
return GetNamespaceImpl(record->getDeclContext(), std::string());
}
-std::string GetFilename(clang::CompilerInstance& instance,
+std::string GetFilename(const clang::SourceManager& source_manager,
clang::SourceLocation location) {
- const clang::SourceManager& source_manager = instance.getSourceManager();
clang::SourceLocation spelling_location =
source_manager.getSpellingLoc(location);
clang::PresumedLoc ploc = source_manager.getPresumedLoc(spelling_location);
diff --git a/tools/clang/plugins/Util.h b/tools/clang/plugins/Util.h
index 02f7edd..01f05b6 100644
--- a/tools/clang/plugins/Util.h
+++ b/tools/clang/plugins/Util.h
@@ -9,7 +9,7 @@
#include "clang/AST/DeclBase.h"
#include "clang/Basic/SourceLocation.h"
-#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Basic/SourceManager.h"
// Utility method for subclasses to determine the namespace of the
// specified record, if any. Unnamed namespaces will be identified as
@@ -18,7 +18,7 @@
// Attempts to determine the filename for the given SourceLocation.
// Returns an empty string if the filename could not be determined.
-std::string GetFilename(clang::CompilerInstance& instance,
+std::string GetFilename(const clang::SourceManager& instance,
clang::SourceLocation location);
#endif // TOOLS_CLANG_PLUGINS_UTIL_H_
diff --git a/tools/clang/plugins/tests/bad_raw_ptr_cast.cpp b/tools/clang/plugins/tests/bad_raw_ptr_cast.cpp
new file mode 100644
index 0000000..1ebfa3d
--- /dev/null
+++ b/tools/clang/plugins/tests/bad_raw_ptr_cast.cpp
@@ -0,0 +1,17 @@
+// Copyright 2022 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "bad_raw_ptr_cast.h"
+
+#include <cstring>
+
+void BadRawPtr::Check() {
+ // Explicit cast from raw_ptr<T>* is not allowed.
+ void* bar = reinterpret_cast<void*>(&foo_);
+ bar = static_cast<void*>(&foo_);
+ bar = (void*)&foo_;
+ // Implicit cast from raw_ptr<T>* is not allowed.
+ bar = &foo_;
+ memcpy(&foo_, &foo_, 1);
+}
diff --git a/tools/clang/plugins/tests/bad_raw_ptr_cast.flags b/tools/clang/plugins/tests/bad_raw_ptr_cast.flags
new file mode 100644
index 0000000..f56a9561
--- /dev/null
+++ b/tools/clang/plugins/tests/bad_raw_ptr_cast.flags
@@ -0,0 +1 @@
+-Xclang -plugin-arg-find-bad-constructs -Xclang check-bad-raw-ptr-cast
diff --git a/tools/clang/plugins/tests/bad_raw_ptr_cast.h b/tools/clang/plugins/tests/bad_raw_ptr_cast.h
new file mode 100644
index 0000000..a6b8931
--- /dev/null
+++ b/tools/clang/plugins/tests/bad_raw_ptr_cast.h
@@ -0,0 +1,18 @@
+// Copyright 2022 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef TOOLS_CLANG_PLUGINS_TESTS_BAD_RAW_PTR_CAST_H_
+#define TOOLS_CLANG_PLUGINS_TESTS_BAD_RAW_PTR_CAST_H_
+
+#include "base/memory/raw_ptr.h"
+
+class BadRawPtr {
+ public:
+ void Check();
+
+ private:
+ raw_ptr<int> foo_;
+};
+
+#endif // TOOLS_CLANG_PLUGINS_TESTS_BAD_RAW_PTR_CAST_H_
\ No newline at end of file
diff --git a/tools/clang/plugins/tests/bad_raw_ptr_cast.txt b/tools/clang/plugins/tests/bad_raw_ptr_cast.txt
new file mode 100644
index 0000000..a1a11ba
--- /dev/null
+++ b/tools/clang/plugins/tests/bad_raw_ptr_cast.txt
@@ -0,0 +1,19 @@
+bad_raw_ptr_cast.cpp:11:44: error: [chromium-raw-ptr-cast] Casting raw_ptr<T>* to another type is not allowed as it may cause BRP ref count mismatch and bypass security checks.
+ void* bar = reinterpret_cast<void*>(&foo_);
+ ^
+bad_raw_ptr_cast.cpp:12:29: error: [chromium-raw-ptr-cast] Casting raw_ptr<T>* to another type is not allowed as it may cause BRP ref count mismatch and bypass security checks.
+ bar = static_cast<void*>(&foo_);
+ ^
+bad_raw_ptr_cast.cpp:13:17: error: [chromium-raw-ptr-cast] Casting raw_ptr<T>* to another type is not allowed as it may cause BRP ref count mismatch and bypass security checks.
+ bar = (void*)&foo_;
+ ^
+bad_raw_ptr_cast.cpp:15:10: error: [chromium-raw-ptr-cast] Casting raw_ptr<T>* to another type is not allowed as it may cause BRP ref count mismatch and bypass security checks.
+ bar = &foo_;
+ ^
+bad_raw_ptr_cast.cpp:16:11: error: [chromium-raw-ptr-cast] Casting raw_ptr<T>* to another type is not allowed as it may cause BRP ref count mismatch and bypass security checks.
+ memcpy(&foo_, &foo_, 1);
+ ^
+bad_raw_ptr_cast.cpp:16:18: error: [chromium-raw-ptr-cast] Casting raw_ptr<T>* to another type is not allowed as it may cause BRP ref count mismatch and bypass security checks.
+ memcpy(&foo_, &foo_, 1);
+ ^
+6 errors generated.
diff --git a/ui/views/examples/table_example.cc b/ui/views/examples/table_example.cc
index c1aced2..c6225120 100644
--- a/ui/views/examples/table_example.cc
+++ b/ui/views/examples/table_example.cc
@@ -65,21 +65,20 @@
MaximumFlexSizeRule::kUnbounded)
.WithWeight(1);
- const auto make_checkbox = [](const std::u16string& label, int id,
- raw_ptr<TableView>* table,
- raw_ptr<Checkbox>* checkbox,
- FlexSpecification full_flex) {
- return Builder<Checkbox>()
- .CopyAddressTo(checkbox)
- .SetText(label)
- .SetCallback(base::BindRepeating(
- [](int id, raw_ptr<TableView>* table, raw_ptr<Checkbox>* checkbox) {
- (*table)->SetColumnVisibility(id, (*checkbox)->GetChecked());
- },
- id, table, checkbox))
- .SetChecked(true)
- .SetProperty(kFlexBehaviorKey, full_flex);
- };
+ const auto make_checkbox =
+ [](const std::u16string& label, int id, raw_ptr<TableView>* table,
+ raw_ptr<Checkbox>* checkbox, FlexSpecification full_flex) {
+ return Builder<Checkbox>()
+ .CopyAddressTo(checkbox)
+ .SetText(label)
+ .SetCallback(base::BindRepeating(
+ [](int id, TableView* table, Checkbox* checkbox) {
+ table->SetColumnVisibility(id, checkbox->GetChecked());
+ },
+ id, *table, *checkbox))
+ .SetChecked(true)
+ .SetProperty(kFlexBehaviorKey, full_flex);
+ };
// Make table
Builder<View>(container)
