Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clangd] Fix use-after-free issues in TidyProvider.cpp #114808

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

hokein
Copy link
Collaborator

@hokein hokein commented Nov 4, 2024

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Nov 4, 2024

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clangd

Author: Haojian Wu (hokein)

Changes

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

1 Files Affected:

  • (modified) clang-tools-extra/clangd/TidyProvider.cpp (+12-7)
diff --git a/clang-tools-extra/clangd/TidyProvider.cpp b/clang-tools-extra/clangd/TidyProvider.cpp
index a87238e0c0938c..f35cf9f0ab999d 100644
--- a/clang-tools-extra/clangd/TidyProvider.cpp
+++ b/clang-tools-extra/clangd/TidyProvider.cpp
@@ -46,7 +46,7 @@ class DotClangTidyCache : private FileCache {
         [this](std::optional<llvm::StringRef> Data) {
           Value.reset();
           if (Data && !Data->empty()) {
-            tidy::DiagCallback Diagnostics = [](const llvm::SMDiagnostic &D) {
+            auto Diagnostics = [](const llvm::SMDiagnostic &D) {
               switch (D.getKind()) {
               case llvm::SourceMgr::DK_Error:
                 elog("tidy-config error at {0}:{1}:{2}: {3}", D.getFilename(),
@@ -159,12 +159,15 @@ TidyProviderRef provideEnvironment() {
     return Ret;
   }();
 
-  if (User)
-    return
-        [](tidy::ClangTidyOptions &Opts, llvm::StringRef) { Opts.User = User; };
+  if (User) {
+    static const auto Provider = [](tidy::ClangTidyOptions &Opts, llvm::StringRef) { Opts.User = User; };
+    return Provider;
+  }
   // FIXME: Once function_ref and unique_function operator= operators handle
   // null values, this can return null.
-  return [](tidy::ClangTidyOptions &, llvm::StringRef) {};
+  static const auto EmptyProvider = [](tidy::ClangTidyOptions &,
+                                       llvm::StringRef) {};
+  return EmptyProvider;
 }
 
 TidyProviderRef provideDefaultChecks() {
@@ -178,10 +181,11 @@ TidyProviderRef provideDefaultChecks() {
       "bugprone-suspicious-missing-comma", "bugprone-unused-raii",
       "bugprone-unused-return-value", "misc-unused-using-decls",
       "misc-unused-alias-decls", "misc-definitions-in-headers");
-  return [](tidy::ClangTidyOptions &Opts, llvm::StringRef) {
+  static auto Provider =  [](tidy::ClangTidyOptions &Opts, llvm::StringRef) {
     if (!Opts.Checks || Opts.Checks->empty())
       Opts.Checks = DefaultChecks;
   };
+  return Provider;
 }
 
 TidyProvider addTidyChecks(llvm::StringRef Checks,
@@ -252,7 +256,7 @@ TidyProvider disableUnusableChecks(llvm::ArrayRef<std::string> ExtraBadChecks) {
 }
 
 TidyProviderRef provideClangdConfig() {
-  return [](tidy::ClangTidyOptions &Opts, llvm::StringRef) {
+  static const auto Provider = [](tidy::ClangTidyOptions &Opts, llvm::StringRef) {
     const auto &CurTidyConfig = Config::current().Diagnostics.ClangTidy;
     if (!CurTidyConfig.Checks.empty())
       mergeCheckList(Opts.Checks, CurTidyConfig.Checks);
@@ -262,6 +266,7 @@ TidyProviderRef provideClangdConfig() {
                                          tidy::ClangTidyOptions::ClangTidyValue(
                                              CheckOption.getValue(), 10000U));
   };
+  return Provider;
 }
 
 TidyProvider provideClangTidyFiles(ThreadsafeFS &TFS) {

Copy link

github-actions bot commented Nov 4, 2024

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

Copy link
Member

@kadircet kadircet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we instead change the function return types to be TidyProvider ? unless there's something we get by making these static lambdas (which isn't obvious to me).

@hokein hokein force-pushed the clangd-use-after-free branch from 38c41b1 to 0d9ff0b Compare November 4, 2024 18:34
@hokein
Copy link
Collaborator Author

hokein commented Nov 4, 2024

can we instead change the function return types to be TidyProvider ? unless there's something we get by making these static lambdas (which isn't obvious to me).

Sure, Done.

@hokein hokein merged commit 8a1eba4 into llvm:main Nov 5, 2024
8 checks passed
@hokein hokein deleted the clangd-use-after-free branch November 5, 2024 11:01
PhilippRados pushed a commit to PhilippRados/llvm-project that referenced this pull request Nov 6, 2024
hokein added a commit that referenced this pull request Nov 7, 2024
This helps catch dangling llvm::function_ref references, see #114950,
#114949, #114808, #114789
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants