[clang][modules] Guard against bad -fmodule-file mappings (llvm#132059)

When using -fmodule-file=<name>=<path/to/bmi> with incorrect inputs,
the compiler crashes in two scenarios:

1. A module is mapped to the right BMI file but one of its
transitively exported module dependencies is also mapped to the same
BMI file.
2. A module is mapped to a wrong BMI file which also transitively
exports that module.

The crash is caused during serialization, when trying to resolve
declaration IDs in the AST body after having imported the wrong
module.

Because the 2nd scenario can only be detected after reading the BMI's
module name, checking for duplicate values while parsing command-line
options is not enough to fix the crash.

This commit fixes the issue by validating module identity after having
read the AST's ControlBlock.

Author: naveen-seth

clang/include/clang/Basic/DiagnosticSerializationKinds.td

@@ -98,6 +98,8 @@ def err_imported_module_relocated : Error<
 def err_module_different_modmap : Error<
     "module '%0' %select{uses|does not use}1 additional module map '%2'"
     "%select{| not}1 used when the module was built">;
+def err_module_mismatch : Error<
+  "tried loading module '%0' from '%1' but found module '%2' instead">, DefaultFatal;
 
 def err_ast_file_macro_def_undef : Error<
     "macro '%0' was %select{defined|undef'd}1 in the AST file '%2' but "

clang/include/clang/Serialization/ASTReader.h

@@ -423,6 +423,9 @@ class ASTReader
     /// configuration.
     ConfigurationMismatch,
 
+    /// The AST file contains a different module than expected.
+    ModuleMismatch,
+
     /// The AST file has errors.
     HadErrors
   };
@@ -1512,11 +1515,13 @@ class ASTReader
                             SourceLocation ImportLoc, ModuleFile *ImportedBy,
                             SmallVectorImpl<ImportedModule> &Loaded,
                             off_t ExpectedSize, time_t ExpectedModTime,
+                            StringRef ExpectedModuleName,
                             ASTFileSignature ExpectedSignature,
                             unsigned ClientLoadCapabilities);
   ASTReadResult ReadControlBlock(ModuleFile &F,
                                  SmallVectorImpl<ImportedModule> &Loaded,
                                  const ModuleFile *ImportedBy,
+                                 StringRef ExpectedModuleName,
                                  unsigned ClientLoadCapabilities);
   static ASTReadResult
   ReadOptionsBlock(llvm::BitstreamCursor &Stream, StringRef Filename,
@@ -1819,6 +1824,18 @@ class ASTReader
                         unsigned ClientLoadCapabilities,
                         ModuleFile **NewLoadedModuleFile = nullptr);
 
+  /// \overload
+  ///
+  /// Calls the above function and checks if the AST file contains the expected
+  /// module. Returns ASTReadResult::Failure on mismatch.
+  ///
+  /// \param ExpectedModuleName The expected name of the new loaded module.
+  ASTReadResult ReadAST(StringRef FileName, ModuleKind Type,
+                        SourceLocation ImportLoc,
+                        unsigned ClientLoadCapabilities,
+                        StringRef ExpectedModuleName,
+                        ModuleFile **NewLoadedModuleFile = nullptr);
+
   /// Make the entities in the given module and any of its (non-explicit)
   /// submodules visible to name lookup.
   ///

clang/lib/Frontend/ASTUnit.cpp

@@ -887,6 +887,7 @@ std::unique_ptr<ASTUnit> ASTUnit::LoadFromASTFile(
   case ASTReader::OutOfDate:
   case ASTReader::VersionMismatch:
   case ASTReader::ConfigurationMismatch:
+  case ASTReader::ModuleMismatch:
   case ASTReader::HadErrors:
     AST->getDiagnostics().Report(diag::err_fe_unable_to_load_pch);
     return nullptr;

clang/lib/Frontend/ChainedIncludesSource.cpp

@@ -81,6 +81,7 @@ createASTReader(CompilerInstance &CI, StringRef pchFile,
   case ASTReader::OutOfDate:
   case ASTReader::VersionMismatch:
   case ASTReader::ConfigurationMismatch:
+  case ASTReader::ModuleMismatch:
   case ASTReader::HadErrors:
     break;
   }

clang/lib/Frontend/CompilerInstance.cpp

@@ -676,6 +676,7 @@ IntrusiveRefCntPtr<ASTReader> CompilerInstance::createPCHExternalASTSource(
   case ASTReader::OutOfDate:
   case ASTReader::VersionMismatch:
   case ASTReader::ConfigurationMismatch:
+  case ASTReader::ModuleMismatch:
   case ASTReader::HadErrors:
     // No suitable PCH file could be found. Return an error.
     break;
@@ -1909,13 +1910,12 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
                           : Source == MS_PrebuiltModulePath
                                 ? 0
                                 : ASTReader::ARR_ConfigurationMismatch;
-  switch (getASTReader()->ReadAST(ModuleFilename,
-                                  Source == MS_PrebuiltModulePath
-                                      ? serialization::MK_PrebuiltModule
-                                      : Source == MS_ModuleBuildPragma
-                                            ? serialization::MK_ExplicitModule
-                                            : serialization::MK_ImplicitModule,
-                                  ImportLoc, ARRFlags)) {
+  switch (getASTReader()->ReadAST(
+      ModuleFilename,
+      Source == MS_PrebuiltModulePath  ? serialization::MK_PrebuiltModule
+      : Source == MS_ModuleBuildPragma ? serialization::MK_ExplicitModule
+                                       : serialization::MK_ImplicitModule,
+      ImportLoc, ARRFlags, ModuleName)) {
   case ASTReader::Success: {
     if (M)
       return M;
@@ -1952,6 +1952,7 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
     // Fall through to error out.
     [[fallthrough]];
   case ASTReader::VersionMismatch:
+  case ASTReader::ModuleMismatch:
   case ASTReader::HadErrors:
     ModuleLoader::HadFatalFailure = true;
     // FIXME: The ASTReader will already have complained, but can we shoehorn

clang/lib/Serialization/ASTReader.cpp

@@ -2926,6 +2926,8 @@ static bool isDiagnosedResult(ASTReader::ASTReadResult ARR, unsigned Caps) {
   case ASTReader::VersionMismatch: return !(Caps & ASTReader::ARR_VersionMismatch);
   case ASTReader::ConfigurationMismatch:
     return !(Caps & ASTReader::ARR_ConfigurationMismatch);
+  case ASTReader::ModuleMismatch:
+    return true;
   case ASTReader::HadErrors: return true;
   case ASTReader::Success: return false;
   }
@@ -3020,11 +3022,10 @@ ASTReader::ASTReadResult ASTReader::ReadOptionsBlock(
   }
 }
 
-ASTReader::ASTReadResult
-ASTReader::ReadControlBlock(ModuleFile &F,
-                            SmallVectorImpl<ImportedModule> &Loaded,
-                            const ModuleFile *ImportedBy,
-                            unsigned ClientLoadCapabilities) {
+ASTReader::ASTReadResult ASTReader::ReadControlBlock(
+    ModuleFile &F, SmallVectorImpl<ImportedModule> &Loaded,
+    const ModuleFile *ImportedBy, StringRef ExpectedModuleName,
+    unsigned ClientLoadCapabilities) {
   BitstreamCursor &Stream = F.Stream;
 
   if (llvm::Error Err = Stream.EnterSubBlock(CONTROL_BLOCK_ID)) {
@@ -3315,7 +3316,7 @@ ASTReader::ReadControlBlock(ModuleFile &F,
 
       // Load the AST file.
       auto Result = ReadASTCore(ImportedFile, ImportedKind, ImportLoc, &F,
-                                Loaded, StoredSize, StoredModTime,
+                                Loaded, StoredSize, StoredModTime, ImportedName,
                                 StoredSignature, Capabilities);
 
       // If we diagnosed a problem, produce a backtrace.
@@ -3338,7 +3339,10 @@ ASTReader::ReadControlBlock(ModuleFile &F,
       case OutOfDate: return OutOfDate;
       case VersionMismatch: return VersionMismatch;
       case ConfigurationMismatch: return ConfigurationMismatch;
-      case HadErrors: return HadErrors;
+      case ModuleMismatch:
+        return ModuleMismatch;
+      case HadErrors:
+        return HadErrors;
       case Success: break;
       }
       break;
@@ -3363,6 +3367,14 @@ ASTReader::ReadControlBlock(ModuleFile &F,
       if (Listener)
         Listener->ReadModuleName(F.ModuleName);
 
+      // Return if the AST unexpectedly contains a different module
+      if (F.Kind == MK_PrebuiltModule && !ExpectedModuleName.empty() &&
+          F.ModuleName != ExpectedModuleName) {
+        Diag(diag::err_module_mismatch)
+            << ExpectedModuleName << F.FileName << F.ModuleName;
+        return ASTReadResult::ModuleMismatch;
+      }
+
       // Validate the AST as soon as we have a name so we can exit early on
       // failure.
       if (ASTReadResult Result = readUnhashedControlBlockOnce())
@@ -4684,6 +4696,15 @@ ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName, ModuleKind Type,
                                             SourceLocation ImportLoc,
                                             unsigned ClientLoadCapabilities,
                                             ModuleFile **NewLoadedModuleFile) {
+  return ReadAST(FileName, Type, ImportLoc, ClientLoadCapabilities, "",
+                 NewLoadedModuleFile);
+}
+
+ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName, ModuleKind Type,
+                                            SourceLocation ImportLoc,
+                                            unsigned ClientLoadCapabilities,
+                                            StringRef ExpectedModuleName,
+                                            ModuleFile **NewLoadedModuleFile) {
   llvm::TimeTraceScope scope("ReadAST", FileName);
 
   llvm::SaveAndRestore SetCurImportLocRAII(CurrentImportLoc, ImportLoc);
@@ -4702,8 +4723,8 @@ ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName, ModuleKind Type,
   SmallVector<ImportedModule, 4> Loaded;
   if (ASTReadResult ReadResult =
           ReadASTCore(FileName, Type, ImportLoc,
-                      /*ImportedBy=*/nullptr, Loaded, 0, 0, ASTFileSignature(),
-                      ClientLoadCapabilities)) {
+                      /*ImportedBy=*/nullptr, Loaded, 0, 0, ExpectedModuleName,
+                      ASTFileSignature(), ClientLoadCapabilities)) {
     ModuleMgr.removeModules(ModuleMgr.begin() + NumModules);
 
     // If we find that any modules are unusable, the global index is going
@@ -4954,25 +4975,26 @@ static unsigned moduleKindForDiagnostic(ModuleKind Kind) {
   llvm_unreachable("unknown module kind");
 }
 
-ASTReader::ASTReadResult
-ASTReader::ReadASTCore(StringRef FileName,
-                       ModuleKind Type,
-                       SourceLocation ImportLoc,
-                       ModuleFile *ImportedBy,
-                       SmallVectorImpl<ImportedModule> &Loaded,
-                       off_t ExpectedSize, time_t ExpectedModTime,
-                       ASTFileSignature ExpectedSignature,
-                       unsigned ClientLoadCapabilities) {
+ASTReader::ASTReadResult ASTReader::ReadASTCore(
+    StringRef FileName, ModuleKind Type, SourceLocation ImportLoc,
+    ModuleFile *ImportedBy, SmallVectorImpl<ImportedModule> &Loaded,
+    off_t ExpectedSize, time_t ExpectedModTime, StringRef ExpectedModuleName,
+    ASTFileSignature ExpectedSignature, unsigned ClientLoadCapabilities) {
   ModuleFile *M;
   std::string ErrorStr;
-  ModuleManager::AddModuleResult AddResult
-    = ModuleMgr.addModule(FileName, Type, ImportLoc, ImportedBy,
-                          getGeneration(), ExpectedSize, ExpectedModTime,
-                          ExpectedSignature, readASTFileSignature,
-                          M, ErrorStr);
+  ModuleManager::AddModuleResult AddResult = ModuleMgr.addModule(
+      FileName, Type, ImportLoc, ImportedBy, getGeneration(), ExpectedSize,
+      ExpectedModTime, ExpectedSignature, readASTFileSignature, M, ErrorStr);
 
   switch (AddResult) {
   case ModuleManager::AlreadyLoaded:
+    // Return if the AST unexpectedly contains a different module
+    if (Type == MK_PrebuiltModule && !ExpectedModuleName.empty() &&
+        M->ModuleName != ExpectedModuleName) {
+      Diag(diag::err_module_mismatch)
+          << ExpectedModuleName << FileName << M->ModuleName;
+      return ASTReadResult::ModuleMismatch;
+    }
     Diag(diag::remark_module_import)
         << M->ModuleName << M->FileName << (ImportedBy ? true : false)
         << (ImportedBy ? StringRef(ImportedBy->ModuleName) : StringRef());
@@ -5053,7 +5075,8 @@ ASTReader::ReadASTCore(StringRef FileName,
     switch (Entry.ID) {
     case CONTROL_BLOCK_ID:
       HaveReadControlBlock = true;
-      switch (ReadControlBlock(F, Loaded, ImportedBy, ClientLoadCapabilities)) {
+      switch (ReadControlBlock(F, Loaded, ImportedBy, ExpectedModuleName,
+                               ClientLoadCapabilities)) {
       case Success:
         // Check that we didn't try to load a non-module AST file as a module.
         //
@@ -5075,6 +5098,8 @@ ASTReader::ReadASTCore(StringRef FileName,
       case OutOfDate: return OutOfDate;
       case VersionMismatch: return VersionMismatch;
       case ConfigurationMismatch: return ConfigurationMismatch;
+      case ModuleMismatch:
+        return ModuleMismatch;
       case HadErrors: return HadErrors;
       }
       break;

clang/test/Modules/fmodule-file-bad-transitive-mapping.cpp

@@ -0,0 +1,52 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface a.cppm -o a.pcm
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface b.cppm -o b.pcm \
+// RUN:   -fmodule-file=A=a.pcm
+
+// This test addresses issue #132059:
+// Bad use of fmodule-file=<name>=<path/to/bmi> previously caused the compiler
+// to crash for the following cases:
+
+//--- a.cppm
+export module A;
+
+export int a() {
+  return 41;
+}
+
+//--- b.cppm
+export module B;
+import A;
+
+export int b() {
+  return a() + 1;
+}
+
+// Test that when -fmodule-file=<name>=<path/to/bmi> mistakenly maps a module to
+// a BMI which depends on the module, the compiler doesn't crash.
+
+// RUN: not %clang_cc1 -std=c++20 main1.cpp-fmodule-file=B=b.pcm \
+// RUN:   -fmodule-file=A=b.pcm
+
+//--- main1.cpp
+import B;
+
+int main() {
+  return b();
+}
+
+// Test that when -fmodule-file=<name>=<path/to/bmi> mistakenly maps a module
+// to a BMI file, and that BMI exposes the parts of the specified module as
+// transitive imports, the compiler doesn't crash.
+
+// RUN: not %clang_cc1 -std=c++20 main2.cpp -fmodule-file=A=b.pcm
+
+//--- main2.cpp
+import A;
+
+int main() {
+  return a();
+}