[libc++] Clang-tidy operator& hijacker. #128366

mordante · 2025-02-22T17:44:27Z

Guards against introducing new places where operator& depends on a template type.

llvmbot · 2025-02-22T17:44:59Z

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

Guards against introducing new places where operator& depends on a template type.

Full diff: https://github.com/llvm/llvm-project/pull/128366.diff

4 Files Affected:

(modified) libcxx/test/tools/clang_tidy_checks/CMakeLists.txt (+1)
(modified) libcxx/test/tools/clang_tidy_checks/libcpp_module.cpp (+3)
(added) libcxx/test/tools/clang_tidy_checks/robust_against_operator_ampersand.cpp (+44)
(added) libcxx/test/tools/clang_tidy_checks/robust_against_operator_ampersand.hpp (+18)

diff --git a/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt b/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
index 0f8f0e8864d0f..e8e62c3f4ba40 100644
--- a/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
+++ b/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
@@ -96,6 +96,7 @@ set(SOURCES
     proper_version_checks.cpp
     qualify_declval.cpp
     robust_against_adl.cpp
+    robust_against_operator_ampersand.cpp
     uglify_attributes.cpp
 
     libcpp_module.cpp
diff --git a/libcxx/test/tools/clang_tidy_checks/libcpp_module.cpp b/libcxx/test/tools/clang_tidy_checks/libcpp_module.cpp
index bc7c8ce7ec443..a52e25f2cf08f 100644
--- a/libcxx/test/tools/clang_tidy_checks/libcpp_module.cpp
+++ b/libcxx/test/tools/clang_tidy_checks/libcpp_module.cpp
@@ -17,6 +17,7 @@
 #include "proper_version_checks.hpp"
 #include "qualify_declval.hpp"
 #include "robust_against_adl.hpp"
+#include "robust_against_operator_ampersand.hpp"
 #include "uglify_attributes.hpp"
 
 namespace {
@@ -30,6 +31,8 @@ class LibcxxTestModule : public clang::tidy::ClangTidyModule {
     check_factories.registerCheck<libcpp::nodebug_on_aliases>("libcpp-nodebug-on-aliases");
     check_factories.registerCheck<libcpp::proper_version_checks>("libcpp-cpp-version-check");
     check_factories.registerCheck<libcpp::robust_against_adl_check>("libcpp-robust-against-adl");
+    check_factories.registerCheck<libcpp::robust_against_operator_ampersand>(
+        "libcpp-robust-against-operator-ampersand");
     check_factories.registerCheck<libcpp::uglify_attributes>("libcpp-uglify-attributes");
     check_factories.registerCheck<libcpp::qualify_declval>("libcpp-qualify-declval");
   }
diff --git a/libcxx/test/tools/clang_tidy_checks/robust_against_operator_ampersand.cpp b/libcxx/test/tools/clang_tidy_checks/robust_against_operator_ampersand.cpp
new file mode 100644
index 0000000000000..8361e0c3eee88
--- /dev/null
+++ b/libcxx/test/tools/clang_tidy_checks/robust_against_operator_ampersand.cpp
@@ -0,0 +1,44 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang-tidy/ClangTidyCheck.h"
+#include "clang-tidy/ClangTidyModuleRegistry.h"
+#include "clang/Tooling/FixIt.h"
+
+#include "robust_against_operator_ampersand.hpp"
+
+// This clang-tidy check ensures that we don't use operator& on dependant
+// types. If the type is user supplied it may call the type's operator&.
+// Instead use std::addressof.
+
+namespace libcpp {
+robust_against_operator_ampersand::robust_against_operator_ampersand(
+    llvm::StringRef name, clang::tidy::ClangTidyContext* context)
+    : clang::tidy::ClangTidyCheck(name, context) {}
+
+void robust_against_operator_ampersand::registerMatchers(clang::ast_matchers::MatchFinder* finder) {
+  using namespace clang::ast_matchers;
+  finder->addMatcher(
+      cxxOperatorCallExpr(allOf(hasOperatorName("&"), argumentCountIs(1), isTypeDependent()),
+                          unless(hasUnaryOperand(dependentScopeDeclRefExpr())))
+          .bind("match"),
+      this);
+}
+
+void robust_against_operator_ampersand::check(const clang::ast_matchers::MatchFinder::MatchResult& result) {
+  if (const auto* call = result.Nodes.getNodeAs< clang::CXXOperatorCallExpr >("match"); call != nullptr) {
+    diag(call->getBeginLoc(), "Guard against user provided operator& for dependent types.")
+        << clang::FixItHint::CreateReplacement(
+               call->getSourceRange(),
+               (llvm::Twine(
+                    "std::addressof(" + clang::tooling::fixit::getText(*call->getArg(0), *result.Context) + ")"))
+                   .str());
+  }
+}
+
+} // namespace libcpp
diff --git a/libcxx/test/tools/clang_tidy_checks/robust_against_operator_ampersand.hpp b/libcxx/test/tools/clang_tidy_checks/robust_against_operator_ampersand.hpp
new file mode 100644
index 0000000000000..5cdc0baca5c23
--- /dev/null
+++ b/libcxx/test/tools/clang_tidy_checks/robust_against_operator_ampersand.hpp
@@ -0,0 +1,18 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang-tidy/ClangTidyCheck.h"
+
+namespace libcpp {
+class robust_against_operator_ampersand : public clang::tidy::ClangTidyCheck {
+public:
+  robust_against_operator_ampersand(llvm::StringRef, clang::tidy::ClangTidyContext*);
+  void registerMatchers(clang::ast_matchers::MatchFinder*) override;
+  void check(const clang::ast_matchers::MatchFinder::MatchResult&) override;
+};
+} // namespace libcpp

libcxx/test/tools/clang_tidy_checks/robust_against_operator_ampersand.hpp

github-actions · 2025-03-03T18:08:13Z

✅ With the latest revision this PR passed the C/C++ code formatter.

denzor200 · 2025-03-12T15:17:06Z

This check should be in regular Clang Tidy, using of std::addressof actual not only for libbcpp, any user-writted code might follow the guidline to use std::addressof instead of operator& for a generic type. For example, the Boost library: https://github.com/boostorg/pfr/blob/f09e6aeae9d050897fff72b93d5f5e866cc5e11a/include/boost/pfr/detail/core_name20_static.hpp#L196

Please, look at the issue about it: #121172

philnik777

@denzor200 It's a lot easier for us to add a libc++-speicific clang-tidy check than a general one. libc++ checks have significantly lower quality requirements, since they only have to work for libc++ (making "seems to work fine" good enough). If anybody wants to make this a general check they're welcome, but it's certainly more effort.

philnik777 · 2025-03-12T17:43:50Z

libcxx/test/tools/clang_tidy_checks/robust_against_operator_ampersand.cpp

+// This clang-tidy check ensures that we don't use operator& on dependant
+// types. If the type is user supplied it may call the type's operator&.
+// Instead use std::addressof.


This should go into the coding guidelines instead.

I disagree. The code should documented what the intention is.

As for the coding guidelines; it already has this information documented.

(Note testing for operator, in the same part of the documentation is on my todo list.)

I agree with @mordante here, this is documented explicitly in the coding guidelines already:

Function overloading also applies to operators. Using &user_object may call a user-defined operator&. Use std::addressof instead. Similarly, to avoid invoking a user-defined operator,, make sure to cast the result to void when using the , or avoid it in the first place. For example:

IMO this comment doesn't provide absolutely necessary information, but it also doesn't hurt. I'd keep it.

The guidelines should still document that this check exists. IMO this comment isn't useful, since it will get out of sync with the guidelines. I'd be much happier with a reference to the guidelines than documenting the same thing in different places.

I would suggest the following, then:

Suggested change

// This clang-tidy check ensures that we don't use operator& on dependant

// types. If the type is user supplied it may call the type's operator&.

// Instead use std::addressof.

// This clang-tidy check ensures that we don't use operator& on dependant

// types, as documented in our coding guidelines.

I am mostly neutral on this issue. It's true that we're duplicating a bit of information, but we're really not duplicating a lot of it. I'd let @mordante decide here to avoid blocking this whole patch on such a minor point.

I don't think it's useful still, but I wouldn't block it on this. However, I would block on not mentioning the check in the documentation as we do with all other checks.

mordante · 2025-03-12T19:03:04Z

This check should be in regular Clang Tidy, using of std::addressof actual not only for libbcpp, any user-writted code might follow the guidline to use std::addressof instead of operator& for a generic type. For example, the Boost library: https://github.com/boostorg/pfr/blob/f09e6aeae9d050897fff72b93d5f5e866cc5e11a/include/boost/pfr/detail/core_name20_static.hpp#L196

Please, look at the issue about it: #121172

As @philnik777 mentioned we like to add checks to our own plugin. Another benefit of our plugin is that the check is available in clang-tidy 19, 20, and HEAD and not just in HEAD.

Still I'm open to adding a similar check to the regular Clang Tidy. Then we can remove our own check once all libc++ supported clang-tidy versions have this check.

Guards against introducing new places where operator& depends on a template type.

github-actions · 2025-03-18T17:24:11Z

⚠️ Python code formatter, darker found issues in your code. ⚠️

You can test this locally with the following command:

darker --check --diff -r HEAD~1...HEAD libcxx/test/libcxx/clang_tidy.gen.py

View the diff from darker here.

--- clang_tidy.gen.py	2025-04-05 15:46:32.000000 +0000
+++ clang_tidy.gen.py	2025-04-05 15:48:28.428352 +0000
@@ -17,11 +17,12 @@
 import sys
 sys.path.append(sys.argv[1])
 from libcxx.header_information import lit_header_restrictions, lit_header_undeprecations, public_headers
 
 for header in public_headers:
-  print(f"""\
+    print(
+        f"""\
 //--- {header}.sh.cpp
 
 // REQUIRES: has-clang-tidy
 
 // The frozen headers should not be updated to the latest libc++ style, so don't test.
@@ -35,6 +36,7 @@
 
 // TODO: run clang-tidy with modules enabled once they are supported
 // RUN: %{{clang-tidy}} %s --warnings-as-errors=* -header-filter=.* --config-file=%{{libcxx-dir}}/.clang-tidy --load=%{{test-tools-dir}}/clang_tidy_checks/libcxx-tidy.plugin -- -Wweak-vtables %{{compile_flags}} -fno-modules
 
 #include <{header}>
-""")
+"""
+    )

ldionne

LGTM

ldionne · 2025-04-01T16:49:01Z

libcxx/test/tools/clang_tidy_checks/robust_against_operator_ampersand.cpp

+// This clang-tidy check ensures that we don't use operator& on dependant
+// types. If the type is user supplied it may call the type's operator&.
+// Instead use std::addressof.


I would suggest the following, then:

Suggested change

// This clang-tidy check ensures that we don't use operator& on dependant

// types. If the type is user supplied it may call the type's operator&.

// Instead use std::addressof.

// This clang-tidy check ensures that we don't use operator& on dependant

// types, as documented in our coding guidelines.

I am mostly neutral on this issue. It's true that we're duplicating a bit of information, but we're really not duplicating a lot of it. I'd let @mordante decide here to avoid blocking this whole patch on such a minor point.

philnik777 · 2025-04-07T19:44:03Z

@mordante Can you please update the coding guidelines?

mordante requested a review from a team as a code owner February 22, 2025 17:44

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Feb 22, 2025

mordante force-pushed the users/mordante/operator_hijacking_clang-tidy branch from 5a96aad to ab09f9d Compare February 22, 2025 18:32

ldionne reviewed Feb 25, 2025

View reviewed changes

libcxx/test/tools/clang_tidy_checks/robust_against_operator_ampersand.hpp Outdated Show resolved Hide resolved

mordante force-pushed the users/mordante/operator_hijacking_not_exploitable branch from 05c33c8 to c24720b Compare February 26, 2025 16:36

Base automatically changed from users/mordante/operator_hijacking_not_exploitable to main February 27, 2025 16:47

mordante force-pushed the users/mordante/operator_hijacking_clang-tidy branch from ab09f9d to c40bf19 Compare March 3, 2025 18:04

mordante changed the base branch from main to users/mordante/operator_hijacking_fixes March 3, 2025 18:05

mordante mentioned this pull request Mar 3, 2025

[NFC][libc++] Guard against operator& hijacking. #129453

Merged

philnik777 requested changes Mar 12, 2025

View reviewed changes

mordante force-pushed the users/mordante/operator_hijacking_fixes branch from 687de63 to fd0c93a Compare March 18, 2025 17:08

Base automatically changed from users/mordante/operator_hijacking_fixes to main March 18, 2025 17:08

mordante added 2 commits March 18, 2025 18:10

[libc++] Clang-tidy operator& hijacker.

3f89222

Guards against introducing new places where operator& depends on a template type.

Addresses review comments.

bc68ee1

mordante force-pushed the users/mordante/operator_hijacking_clang-tidy branch from c40bf19 to bc68ee1 Compare March 18, 2025 17:20

ldionne approved these changes Apr 1, 2025

View reviewed changes

mordante added 6 commits April 1, 2025 19:57

Merge branch 'main' into users/mordante/operator_hijacking_clang-tidy

710398f

Merge branch 'main' into users/mordante/operator_hijacking_clang-tidy

8251c5d

Try to fix CI and address review comment.

b8432da

Another attempt at the CI.

6681cc6

Another attempt at the CI.

f938d3a

Another attempt at the CI.

66c72c5

mordante merged commit a406fb8 into main Apr 7, 2025
79 of 80 checks passed

mordante deleted the users/mordante/operator_hijacking_clang-tidy branch April 7, 2025 16:20

[libc++] Clang-tidy operator& hijacker. #128366

[libc++] Clang-tidy operator& hijacker. #128366

Uh oh!

Conversation

mordante commented Feb 22, 2025

Uh oh!

llvmbot commented Feb 22, 2025

Uh oh!

Uh oh!

github-actions bot commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

denzor200 commented Mar 12, 2025

Uh oh!

philnik777 left a comment

Choose a reason for hiding this comment

Uh oh!

philnik777 Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

mordante Mar 14, 2025

Choose a reason for hiding this comment

Uh oh!

ldionne Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

philnik777 Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

ldionne Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

philnik777 Apr 5, 2025

Choose a reason for hiding this comment

Uh oh!

mordante commented Mar 12, 2025

Uh oh!

github-actions bot commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

ldionne Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

philnik777 commented Apr 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

github-actions bot commented Mar 3, 2025 •

edited

Loading

github-actions bot commented Mar 18, 2025 •

edited

Loading