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

Fix #132059.

Providing incorrect mappings via `-fmodule-file=<name>=<path/to/bmi>`
can crash the compiler when loading a module that imports an
incorrectly mapped module.

The crash occurs during AST body deserialization, when the compiler
attempts to resolve remappings using the `ModuleFile` from the
incorrectly mapped module's BMI file.
The cause is an invalid access into an incorrectly loaded
`ModuleFile`.

This commit fixes the issue by verifying that the mapped BMI files
correspond to the mapped-from modules as soon as the module name is
read from the BMI's control block, and it errors out if there is a
mismatch.

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,46 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// Related to issue #132059
+
+// Precompile the module dependencies correctly 
+// 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 -fmodule-file=A=a.pcm
+
+// Test that providing incorrect mappings via -fmodule-file=<name>=<path/to/bmi>,
+// where the BMI is built for a different module than the one specified and
+// transitively imports the specified module, does not crash the compiler.
+// RUN: not %clang_cc1 -std=c++20 main1.cpp -fmodule-file=A=b.pcm
+
+//--- a.cppm
+export module A;
+
+export int a() {
+  return 41;
+}
+
+//--- b.cppm
+export module B;
+import A;
+
+export int b() {
+  return a() + 1;
+}
+
+//--- main1.cpp
+import A;
+
+int main() {
+  return a();
+}
+
+// Test again for the case where the BMI is first loaded correctly
+// RUN: not %clang_cc1 -std=c++20 main2.cpp-fmodule-file=B=b.pcm -fmodule-file=A=b.pcm
+
+//--- main2.cpp
+import B;
+
+int main() {
+  return b();
+}