Validate clang-tidy check names.

[alexfh]

Summary

ClangTidy and its frontends rely on check names being restricted to
certain rules, e.g. having a comma in a check name would break -checks=
parameter parsing. So we need to validate check names early.

[djasper]

clang-tidy/ClangTidy.cpp

294

Should we really validate the analyzer check names here? And if so, should we assert?

I'd be more comfortable moving at least the assert to somewhere in the static analyzer itself, so that people working on it know what valid names are and don't have to discover that by breaking an assert build of clang-tidy.

Or, alternatively, people can't add more analyzer checks by linking additional libraries to clang-tidy, right? Should we just have a test that calls "clang-tidy -list-checks" and verifies all the names?

323

It almost feels like it is not worth implementing and testing this function. I'd be fine with simply defining the string as a constant and then calling llvm::Regex().match() at the two call sites. Specifically, if it is located here, people might think that they have to do something with this function when writing a check.

clang-tidy/ClangTidy.h

89

Maybe: .., if \p Name can be used as a check name.

[alexfh]

Use a test instead of runtime checks.

[djasper]

I think this is fine for the analyzer checks. However, as people can link in their checks, it might make sense to do runtime checks for those. WDYT?

[alexfh]

In D4982#7, @djasper wrote:

I think this is fine for the analyzer checks. However, as people can link in their checks, it might make sense to do runtime checks for those. WDYT?

Well, if someone links their checks in, they should still run clang-tidy tests.

[alexfh]

PIng

[alexfh]

Daily pIng ;)

[djasper]

But they won't run clang-tidy checks, but I can see reasons for keeping the implementation simpler. Either way, this is a strict improvement, so looks good.

[alexfh]

This has already landed unintentionally in r217155.

clang-tidy/ClangTidy.h

public:
   /// \brief Add a diagnostic with the check's name.
   DiagnosticBuilder diag(SourceLocation Loc, StringRef Description,
                          DiagnosticIDs::Level Level = DiagnosticIDs::Warning);
 
   /// \brief Sets the check name. Intended to be used by the clang-tidy
   /// framework. Can be called only once.
   void setName(StringRef Name);
 
+  /// \brief Returns \c true, if the specified string can be used for a check
+  /// name.
+  ///
+  /// Only alphanumeric characters, '-', '_' and '.' are allowed in check names.
+  static bool isValidCheckName(StringRef Name);
+
 private:
   void run(const ast_matchers::MatchFinder::MatchResult &Result) override;
   ClangTidyContext *Context;
   std::string CheckName;
 };
 
 class ClangTidyCheckFactories;
 

clang-tidy/ClangTidy.cpp

 #include "clang/Lex/Preprocessor.h"
 #include "clang/Rewrite/Frontend/FixItRewriter.h"
 #include "clang/Rewrite/Frontend/FrontendActions.h"
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
+#include "llvm/Support/Regex.h"
 #include "llvm/Support/Signals.h"
 #include <algorithm>
 #include <utility>
 
 using namespace clang::ast_matchers;
 using namespace clang::driver;
 using namespace clang::tooling;
 using namespace llvm;
if (AnalyzerChecksEnabled) {
     // thus not applicable to clang-tidy in general.
     //
     // Always add all core checkers if any other static analyzer checks are
     // enabled. This is currently necessary, as other path sensitive checks
     // rely on the core checkers.
     for (StringRef CheckName : StaticAnalyzerChecks) {
       std::string Checker((AnalyzerCheckNamePrefix + CheckName).str());
 
+      if (!ClangTidyCheck::isValidCheckName(Checker)) {
+        assert(false &&
+               "Static analyzer check name contains invalid characters");
+        continue;
+      }
       if (CheckName.startswith("core") ||
           (!CheckName.startswith("debug") && Filter.contains(Checker)))
         List.push_back(std::make_pair(CheckName, true));
     }
   }
   return List;
 }
 
 DiagnosticBuilder ClangTidyCheck::diag(SourceLocation Loc, StringRef Message,
                                        DiagnosticIDs::Level Level) {
   return Context->diag(CheckName, Loc, Message, Level);
 }
 
 void ClangTidyCheck::run(const ast_matchers::MatchFinder::MatchResult &Result) {
   Context->setSourceManager(Result.SourceManager);
   check(Result);
 }
 
 void ClangTidyCheck::setName(StringRef Name) {
   assert(CheckName.empty());
   CheckName = Name.str();
 }
 
+bool ClangTidyCheck::isValidCheckName(StringRef Name) {
+  return llvm::Regex("^[a-zA-Z0-9_.\\-]+$").match(Name);
+}
+
 std::vector<std::string> getCheckNames(const ClangTidyOptions &Options) {
   clang::tidy::ClangTidyContext Context(
       new DefaultOptionsProvider(ClangTidyGlobalOptions(), Options));
   ClangTidyASTConsumerFactory Factory(Context);
   return Factory.getCheckNames(Context.getChecksFilter());
 }
 
 ClangTidyStats runClangTidy(ClangTidyOptionsProvider *OptionsProvider,

clang-tidy/ClangTidyModule.cpp

 void ClangTidyCheckFactories::addCheckFactory(StringRef Name,
                                               CheckFactoryBase *Factory) {
   Factories[Name] = Factory;
 }
 
 void ClangTidyCheckFactories::createChecks(
     GlobList &Filter, std::vector<std::unique_ptr<ClangTidyCheck>> &Checks) {
   for (const auto &Factory : Factories) {
+    if (!ClangTidyCheck::isValidCheckName(Factory.first)) {
+      assert(false && "ClangTidy check name contains invalid characters");
+      continue;
+    }
     if (Filter.contains(Factory.first)) {
       ClangTidyCheck *Check = Factory.second->createCheck();
       Check->setName(Factory.first);
       Checks.emplace_back(Check);
     }
   }
 }
 
 } // namespace tidy
 } // namespace clang

unittests/clang-tidy/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   support
   )
 
 get_filename_component(CLANG_LINT_SOURCE_DIR
   ${CMAKE_CURRENT_SOURCE_DIR}/../../clang-tidy REALPATH)
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
+  ClangTidyTest.cpp
   LLVMModuleTest.cpp
   GoogleModuleTest.cpp
   MiscModuleTest.cpp)
 
 target_link_libraries(ClangTidyTests
   clangAST
   clangASTMatchers
   clangBasic
   clangFrontend
   clangTidy
   clangTidyGoogleModule
   clangTidyLLVMModule
   clangTidyMiscModule
   clangTooling
   )

unittests/clang-tidy/ClangTidyTest.cpp

+#include "ClangTidy.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+TEST(ClangTidyCheck, IsValidCheckName) {
+  EXPECT_TRUE(ClangTidyCheck::isValidCheckName("asdf123-xxx_yyy.YYY.ZZZ"));
+  EXPECT_FALSE(ClangTidyCheck::isValidCheckName(""));
+  EXPECT_FALSE(ClangTidyCheck::isValidCheckName("a b"));
+  EXPECT_FALSE(ClangTidyCheck::isValidCheckName("ab "));
+  EXPECT_FALSE(ClangTidyCheck::isValidCheckName("ab!"));
+}
+
+} // namespace test
+} // namespace tidy
+} // namespace clang