Enforce safe usage of DiagnosticsEngine::getCustomDiagID()

Replace the last incorrect uses and templatize the function to require a
compile-time constant string preventing further misuse.

The diagnostic formatter expects well-formed input and has undefined behaviour
with arbitrary input or crafted user strings in source files. Accepting user
input would also have caused unbounded generation of new diagnostic IDs which
can be problematic in long-running sessions or language bindings.

This completes the work to fix several incorrect callers that passed user
input or raw messages to the diagnostics engine where a constant format string
was expected.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@200132 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Basic/Diagnostic.h b/include/clang/Basic/Diagnostic.h
index ef30c474bc2929718e801fa6a3d968381a8f5523..bd5f7b06f46b47ac67ef3e9a1133c3ee22e244e1 100644 (file)
--- a/include/clang/Basic/Diagnostic.h
+++ b/include/clang/Basic/Diagnostic.h
@@ -587,15 +587,18 @@ public:
     this->NumWarnings = NumWarnings;
   }
 
-  /// \brief Return an ID for a diagnostic with the specified message and level.
+  /// \brief Return an ID for a diagnostic with the specified format string and
+  /// level.
   ///
   /// If this is the first request for this diagnostic, it is registered and
   /// created, otherwise the existing ID is returned.
   ///
   /// \param Message A fixed diagnostic format string that will be hashed and
   /// mapped to a unique DiagID.
-  unsigned getCustomDiagID(Level L, StringRef Message) {
-    return Diags->getCustomDiagID((DiagnosticIDs::Level)L, Message);
+  template <unsigned N>
+  unsigned getCustomDiagID(Level L, const char (&FormatString)[N]) {
+    return Diags->getCustomDiagID((DiagnosticIDs::Level)L,
+                                  StringRef(FormatString, N));
   }
 
   /// \brief Converts a diagnostic argument (as an intptr_t) into the string

diff --git a/include/clang/Basic/DiagnosticIDs.h b/include/clang/Basic/DiagnosticIDs.h
index 2be4acf2faf9131f4cee47ec557918f84b19394a..0152f25fa669c4e6e5c0741e36c18deaee5f364b 100644 (file)
--- a/include/clang/Basic/DiagnosticIDs.h
+++ b/include/clang/Basic/DiagnosticIDs.h
@@ -124,11 +124,16 @@ public:
   DiagnosticIDs();
   ~DiagnosticIDs();
 
-  /// \brief Return an ID for a diagnostic with the specified message and level.
+  /// \brief Return an ID for a diagnostic with the specified format string and
+  /// level.
   ///
   /// If this is the first request for this diagnostic, it is registered and
   /// created, otherwise the existing ID is returned.
-  unsigned getCustomDiagID(Level L, StringRef Message);
+
+  // FIXME: Replace this function with a create-only facilty like
+  // createCustomDiagIDFromFormatString() to enforce safe usage. At the time of
+  // writing, nearly all callers of this function were invalid.
+  unsigned getCustomDiagID(Level L, StringRef FormatString);
 
   //===--------------------------------------------------------------------===//
   // Diagnostic classification and reporting interfaces.

diff --git a/lib/ARCMigrate/FileRemapper.cpp b/lib/ARCMigrate/FileRemapper.cpp
index e406d47c0258447eaaa965ac235214f3bc8e4ff2..88e54ff89dfc56d1465c4b4d6dee2ef8e0822516 100644 (file)
--- a/lib/ARCMigrate/FileRemapper.cpp
+++ b/lib/ARCMigrate/FileRemapper.cpp
@@ -271,9 +271,7 @@ void FileRemapper::resetTarget(Target &targ) {
 }
 
 bool FileRemapper::report(const Twine &err, DiagnosticsEngine &Diag) {
-  SmallString<128> buf;
-  unsigned ID = Diag.getDiagnosticIDs()->getCustomDiagID(DiagnosticIDs::Error,
-                                                         err.toStringRef(buf));
-  Diag.Report(ID);
+  Diag.Report(Diag.getCustomDiagID(DiagnosticsEngine::Error, "%0"))
+      << err.str();
   return true;
 }

diff --git a/lib/ARCMigrate/ObjCMT.cpp b/lib/ARCMigrate/ObjCMT.cpp
index 39370b9823433d29dd1813c8430a4a797101fa92..19352c010714823627b008903d113f16462e128d 100644 (file)
--- a/lib/ARCMigrate/ObjCMT.cpp
+++ b/lib/ARCMigrate/ObjCMT.cpp
@@ -1845,11 +1845,9 @@ void ObjCMigrateASTConsumer::HandleTranslationUnit(ASTContext &Ctx) {
    std::string Error;
    llvm::raw_fd_ostream OS(MigrateDir.c_str(), Error, llvm::sys::fs::F_Binary);
     if (!Error.empty()) {
-      // FIXME: It's not safe to pass arbitrary user-generated strings into
-      // getCustomDiagID(). Use a constant diagnostic ID instead.
-      unsigned ID = Ctx.getDiagnostics().getDiagnosticIDs()->
-          getCustomDiagID(DiagnosticIDs::Error, Error);
-      Ctx.getDiagnostics().Report(ID);
+      DiagnosticsEngine &Diags = Ctx.getDiagnostics();
+      Diags.Report(Diags.getCustomDiagID(DiagnosticsEngine::Error, "%0"))
+          << Error;
       return;
     }
 
@@ -2062,12 +2060,8 @@ private:
 }
 
 static bool reportDiag(const Twine &Err, DiagnosticsEngine &Diag) {
-  SmallString<128> Buf;
-  // FIXME: It's not safe to pass arbitrary user-generated strings into
-  // getCustomDiagID(). Use a constant diagnostic ID instead.
-  unsigned ID = Diag.getDiagnosticIDs()->getCustomDiagID(DiagnosticIDs::Error,
-                                                         Err.toStringRef(Buf));
-  Diag.Report(ID);
+  Diag.Report(Diag.getCustomDiagID(DiagnosticsEngine::Error, "%0"))
+      << Err.str();
   return true;
 }
 

diff --git a/lib/CodeGen/CodeGenModule.cpp b/lib/CodeGen/CodeGenModule.cpp
index 95dd52c4d589f5ce4504d7624bfd5d7b41740e06..2ee232284fdc447c230fd685801386113713026a 100644 (file)
--- a/lib/CodeGen/CodeGenModule.cpp
+++ b/lib/CodeGen/CodeGenModule.cpp
@@ -339,9 +339,9 @@ void CodeGenModule::DecorateInstruction(llvm::Instruction *Inst,
     Inst->setMetadata(llvm::LLVMContext::MD_tbaa, TBAAInfo);
 }
 
-void CodeGenModule::Error(SourceLocation loc, StringRef error) {
-  unsigned diagID = getDiags().getCustomDiagID(DiagnosticsEngine::Error, error);
-  getDiags().Report(Context.getFullLoc(loc), diagID);
+void CodeGenModule::Error(SourceLocation loc, StringRef message) {
+  unsigned diagID = getDiags().getCustomDiagID(DiagnosticsEngine::Error, "%0");
+  getDiags().Report(Context.getFullLoc(loc), diagID) << message;
 }
 
 /// ErrorUnsupported - Print out an error that codegen doesn't support the

diff --git a/lib/CodeGen/CodeGenPGO.cpp b/lib/CodeGen/CodeGenPGO.cpp
index fb42a8a22245c8cff27aded7710f11b126d2fce0..a1782d32b445e9053492e270b01781db9788c30f 100644 (file)
--- a/lib/CodeGen/CodeGenPGO.cpp
+++ b/lib/CodeGen/CodeGenPGO.cpp
@@ -24,8 +24,8 @@ using namespace CodeGen;
 
 static void ReportBadPGOData(CodeGenModule &CGM, const char *Message) {
   DiagnosticsEngine &Diags = CGM.getDiags();
-  unsigned DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Error, Message);
-  Diags.Report(DiagID);
+  unsigned diagID = Diags.getCustomDiagID(DiagnosticsEngine::Error, "%0");
+  Diags.Report(diagID) << Message;
 }
 
 PGOProfileData::PGOProfileData(CodeGenModule &CGM, std::string Path)

diff --git a/lib/Lex/PTHLexer.cpp b/lib/Lex/PTHLexer.cpp
index e2629a3b2c498cbc894a313a5f1b284b35adce51..e174222ece388b7ce5ee639e911324033f060b08 100644 (file)
--- a/lib/Lex/PTHLexer.cpp
+++ b/lib/Lex/PTHLexer.cpp
@@ -419,7 +419,7 @@ PTHManager::~PTHManager() {
 }
 
 static void InvalidPTH(DiagnosticsEngine &Diags, const char *Msg) {
-  Diags.Report(Diags.getCustomDiagID(DiagnosticsEngine::Error, Msg));
+  Diags.Report(Diags.getCustomDiagID(DiagnosticsEngine::Error, "%0")) << Msg;
 }
 
 PTHManager *PTHManager::Create(const std::string &file,