https://github.com/naveen-seth updated
https://github.com/llvm/llvm-project/pull/133462
>From 804617c3083935d10f98b10e86f965e7508eb587 Mon Sep 17 00:00:00 2001
From: naveen-seth <naveen.ha...@outlook.com>
Date: Fri, 28 Mar 2025 06:59:06 +0100
Subject: [PATCH] [clang][modules] Guard against bad -fmodule-file mappings
(#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.
---
.../Basic/DiagnosticSerializationKinds.td | 2 +
clang/include/clang/Serialization/ASTReader.h | 17 +++++
clang/lib/Frontend/ASTUnit.cpp | 1 +
clang/lib/Frontend/ChainedIncludesSource.cpp | 1 +
clang/lib/Frontend/CompilerInstance.cpp | 15 ++--
clang/lib/Serialization/ASTReader.cpp | 73 +++++++++++++------
.../fmodule-file-bad-transitive-mapping.cpp | 52 +++++++++++++
7 files changed, 130 insertions(+), 31 deletions(-)
create mode 100644 clang/test/Modules/fmodule-file-bad-transitive-mapping.cpp
diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
index 3914d3930bec7..16cc946e3f3d9 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/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 "
diff --git a/clang/include/clang/Serialization/ASTReader.h
b/clang/include/clang/Serialization/ASTReader.h
index 2779b3d1cf2ea..57f2a08e09359 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/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.
///
diff --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp
index 0a5f1cfd1a264..7500be81ea976 100644
--- a/clang/lib/Frontend/ASTUnit.cpp
+++ b/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;
diff --git a/clang/lib/Frontend/ChainedIncludesSource.cpp
b/clang/lib/Frontend/ChainedIncludesSource.cpp
index a7096e27796a0..ee0363249124b 100644
--- a/clang/lib/Frontend/ChainedIncludesSource.cpp
+++ b/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;
}
diff --git a/clang/lib/Frontend/CompilerInstance.cpp
b/clang/lib/Frontend/CompilerInstance.cpp
index 91093d3ccb84c..95b3ae34f36fe 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/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
diff --git a/clang/lib/Serialization/ASTReader.cpp
b/clang/lib/Serialization/ASTReader.cpp
index 0cd2cedb48dd9..afae2f3a0e217 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/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;
diff --git a/clang/test/Modules/fmodule-file-bad-transitive-mapping.cpp
b/clang/test/Modules/fmodule-file-bad-transitive-mapping.cpp
new file mode 100644
index 0000000000000..9bfe3c786605b
--- /dev/null
+++ b/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();
+}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
