[tidy] Fix possible use-after-free in IdentifierNamingCheck

[kadircet]

Summary

CheckName retrieived during construction is a reference to keys stored
inside ClangTidyCheckFactories, which isn't guranteed to outlive the check.

[kadircet]

see https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidy.cpp#L638 for such a pattern, clangd also initializes checks with a similar approach.

[hokein]

The fix looks good.

We also have a CheckName field in the base class ClangTidyCheck, however that's field is private, we can't not access, we could consider make it protected (I think that's probably out of the scope of this patch).

[njames93]

In D150254#4331640, @kadircet wrote:

see https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidy.cpp#L638 for such a pattern, clangd also initializes checks with a similar approach.

In this example the factory outlives the check so there is no possible use after free.

In regard to this change, I think that this member can actually be removed entirely as we can get the name of the check using the getID virtual function.

[njames93]

In D150254#4331734, @hokein wrote:

The fix looks good.

We also have a CheckName field in the base class ClangTidyCheck, however that's field is private, we can't not access, we could consider make it protected (I think that's probably out of the scope of this patch).

See previous comment

[kadircet]

• Expose getID to tidy-checks and use it instead of storing checkname

[kadircet]

In D150254#4331738, @njames93 wrote:

In D150254#4331640, @kadircet wrote:

see https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidy.cpp#L638 for such a pattern, clangd also initializes checks with a similar approach.

In this example the factory outlives the check so there is no possible use after free.

Sorry you're right, I thought checks created in https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidy.cpp#L660 would become part of the return value, but instead they're dropped on the floor.
but nevertheless, clangd has this pattern in https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/ParsedAST.cpp#L479. I am happy to change the pattern in clangd as well, but I don't think there's anything requiring users to keep the "factory" around in the contract.

In regard to this change, I think that this member can actually be removed entirely as we can get the name of the check using the getID virtual function.

made it protected and used it instead.

clang-tools-extra/clang-tidy/ClangTidyCheck.h

private:
 
     std::string NamePrefix;
     const ClangTidyOptions::OptionMap &CheckOptions;
     ClangTidyContext *Context;
   };
 
 private:
   void run(const ast_matchers::MatchFinder::MatchResult &Result) override;
-  StringRef getID() const override { return CheckName; }
   std::string CheckName;
   ClangTidyContext *Context;
 
 protected:
   OptionsView Options;
   /// Returns the main file name of the current translation unit.
   StringRef getCurrentMainFile() const { return Context->getCurrentFile(); }
   /// Returns the language options from the context.
   const LangOptions &getLangOpts() const { return Context->getLangOpts(); }
   /// Returns true when the check is run in a use case when only 1 fix will be
   /// applied at a time.
   bool areDiagsSelfContained() const {
     return Context->areDiagsSelfContained();
   }
+  StringRef getID() const override { return CheckName; }
 };
 
 /// Read a named option from the ``Context`` and parse it as a bool.
 ///
 /// Reads the option with the check-local name \p LocalName from the
 /// ``CheckOptions``. If the corresponding key is not present, return
 /// ``std::nullopt``.
 ///

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h

 //===--- IdentifierNamingCheck.h - clang-tidy -------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_IDENTIFIERNAMINGCHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_IDENTIFIERNAMINGCHECK_H
 
 #include "../utils/RenamerClangTidyCheck.h"
 #include <optional>
+#include <string>
 namespace clang::tidy {
 namespace readability {
 
 enum StyleKind : int;
 
 /// Checks for identifiers naming style mismatch.
 ///
 /// This check will try to enforce coding guidelines on the identifiers naming.
private:
 
   const FileStyle &getStyleForFile(StringRef FileName) const;
 
   /// Stores the style options as a vector, indexed by the specified \ref
   /// StyleKind, for a given directory.
   mutable llvm::StringMap<FileStyle> NamingStylesCache;
   FileStyle *MainFileStyle;
   ClangTidyContext *Context;
-  const StringRef CheckName;
   const bool GetConfigPerFile;
   const bool IgnoreFailedSplit;
   HungarianNotation HungarianNotation;
 };
 
 } // namespace readability
 template <>
 struct OptionEnumMapping<readability::IdentifierNamingCheck::CaseType> {
   static llvm::ArrayRef<
       std::pair<readability::IdentifierNamingCheck::CaseType, StringRef>>
   getEnumMapping();
 };
 } // namespace clang::tidy
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_IDENTIFIERNAMINGCHECK_H

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

 //===--- IdentifierNamingCheck.cpp - clang-tidy ---------------------------===//
 //
 // 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 "IdentifierNamingCheck.h"
 
 #include "../GlobList.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMapInfo.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/YAMLParser.h"
 #include <optional>
 
if (!QT.isNull() && QT->isArrayType())
       TypeName.append("[]");
   }
 
   return TypeName;
 }
 
 IdentifierNamingCheck::IdentifierNamingCheck(StringRef Name,
                                              ClangTidyContext *Context)
-    : RenamerClangTidyCheck(Name, Context), Context(Context), CheckName(Name),
+    : RenamerClangTidyCheck(Name, Context), Context(Context),
       GetConfigPerFile(Options.get("GetConfigPerFile", true)),
       IgnoreFailedSplit(Options.get("IgnoreFailedSplit", false)) {
 
   auto IterAndInserted = NamingStylesCache.try_emplace(
       llvm::sys::path::parent_path(Context->getCurrentFile()),
       getFileStyleFromOptions(Options));
   assert(IterAndInserted.second && "Couldn't insert Style");
   // Holding a reference to the data in the vector is safe as it should never

 IdentifierNamingCheck::getStyleForFile(StringRef FileName) const {
   if (!GetConfigPerFile)
     return *MainFileStyle;
   StringRef Parent = llvm::sys::path::parent_path(FileName);
   auto Iter = NamingStylesCache.find(Parent);
   if (Iter != NamingStylesCache.end())
     return Iter->getValue();
 
+  llvm::StringRef CheckName = getID();
   ClangTidyOptions Options = Context->getOptionsForFile(FileName);
   if (Options.Checks && GlobList(*Options.Checks).contains(CheckName)) {
     auto It = NamingStylesCache.try_emplace(
         Parent,
         getFileStyleFromOptions({CheckName, Options.CheckOptions, Context}));
     assert(It.second);
     return It.first->getValue();
   }
   // Default construction gives an empty style.
   auto It = NamingStylesCache.try_emplace(Parent);
   assert(It.second);
   return It.first->getValue();
 }
 
 } // namespace readability
 } // namespace clang::tidy