Skip to content

Commit 804617c

Browse files
naveen-sethnaveen-seth
committed
[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.
1 parent e70fe9b commit 804617c

File tree

7 files changed

+130
-31
lines changed

7 files changed

+130
-31
lines changed

‎clang/include/clang/Basic/DiagnosticSerializationKinds.td‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,8 @@ def err_imported_module_relocated : Error<
9898
def err_module_different_modmap : Error<
9999
"module '%0' %select{uses|does not use}1 additional module map '%2'"
100100
"%select{| not}1 used when the module was built">;
101+
def err_module_mismatch : Error<
102+
"tried loading module '%0' from '%1' but found module '%2' instead">, DefaultFatal;
101103

102104
def err_ast_file_macro_def_undef : Error<
103105
"macro '%0' was %select{defined|undef'd}1 in the AST file '%2' but "

‎clang/include/clang/Serialization/ASTReader.h‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,9 @@ class ASTReader
423423
/// configuration.
424424
ConfigurationMismatch,
425425

426+
/// The AST file contains a different module than expected.
427+
ModuleMismatch,
428+
426429
/// The AST file has errors.
427430
HadErrors
428431
};
@@ -1512,11 +1515,13 @@ class ASTReader
15121515
SourceLocation ImportLoc, ModuleFile *ImportedBy,
15131516
SmallVectorImpl<ImportedModule> &Loaded,
15141517
off_t ExpectedSize, time_t ExpectedModTime,
1518+
StringRef ExpectedModuleName,
15151519
ASTFileSignature ExpectedSignature,
15161520
unsigned ClientLoadCapabilities);
15171521
ASTReadResult ReadControlBlock(ModuleFile &F,
15181522
SmallVectorImpl<ImportedModule> &Loaded,
15191523
const ModuleFile *ImportedBy,
1524+
StringRef ExpectedModuleName,
15201525
unsigned ClientLoadCapabilities);
15211526
static ASTReadResult
15221527
ReadOptionsBlock(llvm::BitstreamCursor &Stream, StringRef Filename,
@@ -1819,6 +1824,18 @@ class ASTReader
18191824
unsigned ClientLoadCapabilities,
18201825
ModuleFile **NewLoadedModuleFile = nullptr);
18211826

1827+
/// \overload
1828+
///
1829+
/// Calls the above function and checks if the AST file contains the expected
1830+
/// module. Returns ASTReadResult::Failure on mismatch.
1831+
///
1832+
/// \param ExpectedModuleName The expected name of the new loaded module.
1833+
ASTReadResult ReadAST(StringRef FileName, ModuleKind Type,
1834+
SourceLocation ImportLoc,
1835+
unsigned ClientLoadCapabilities,
1836+
StringRef ExpectedModuleName,
1837+
ModuleFile **NewLoadedModuleFile = nullptr);
1838+
18221839
/// Make the entities in the given module and any of its (non-explicit)
18231840
/// submodules visible to name lookup.
18241841
///

‎clang/lib/Frontend/ASTUnit.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -887,6 +887,7 @@ std::unique_ptr<ASTUnit> ASTUnit::LoadFromASTFile(
887887
case ASTReader::OutOfDate:
888888
case ASTReader::VersionMismatch:
889889
case ASTReader::ConfigurationMismatch:
890+
case ASTReader::ModuleMismatch:
890891
case ASTReader::HadErrors:
891892
AST->getDiagnostics().Report(diag::err_fe_unable_to_load_pch);
892893
return nullptr;

‎clang/lib/Frontend/ChainedIncludesSource.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ createASTReader(CompilerInstance &CI, StringRef pchFile,
8181
case ASTReader::OutOfDate:
8282
case ASTReader::VersionMismatch:
8383
case ASTReader::ConfigurationMismatch:
84+
case ASTReader::ModuleMismatch:
8485
case ASTReader::HadErrors:
8586
break;
8687
}

‎clang/lib/Frontend/CompilerInstance.cpp‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -676,6 +676,7 @@ IntrusiveRefCntPtr<ASTReader> CompilerInstance::createPCHExternalASTSource(
676676
case ASTReader::OutOfDate:
677677
case ASTReader::VersionMismatch:
678678
case ASTReader::ConfigurationMismatch:
679+
case ASTReader::ModuleMismatch:
679680
case ASTReader::HadErrors:
680681
// No suitable PCH file could be found. Return an error.
681682
break;
@@ -1909,13 +1910,12 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
19091910
: Source == MS_PrebuiltModulePath
19101911
? 0
19111912
: ASTReader::ARR_ConfigurationMismatch;
1912-
switch (getASTReader()->ReadAST(ModuleFilename,
1913-
Source == MS_PrebuiltModulePath
1914-
? serialization::MK_PrebuiltModule
1915-
: Source == MS_ModuleBuildPragma
1916-
? serialization::MK_ExplicitModule
1917-
: serialization::MK_ImplicitModule,
1918-
ImportLoc, ARRFlags)) {
1913+
switch (getASTReader()->ReadAST(
1914+
ModuleFilename,
1915+
Source == MS_PrebuiltModulePath ? serialization::MK_PrebuiltModule
1916+
: Source == MS_ModuleBuildPragma ? serialization::MK_ExplicitModule
1917+
: serialization::MK_ImplicitModule,
1918+
ImportLoc, ARRFlags, ModuleName)) {
19191919
case ASTReader::Success: {
19201920
if (M)
19211921
return M;
@@ -1952,6 +1952,7 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
19521952
// Fall through to error out.
19531953
[[fallthrough]];
19541954
case ASTReader::VersionMismatch:
1955+
case ASTReader::ModuleMismatch:
19551956
case ASTReader::HadErrors:
19561957
ModuleLoader::HadFatalFailure = true;
19571958
// FIXME: The ASTReader will already have complained, but can we shoehorn

‎clang/lib/Serialization/ASTReader.cpp‎

Lines changed: 49 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -2926,6 +2926,8 @@ static bool isDiagnosedResult(ASTReader::ASTReadResult ARR, unsigned Caps) {
29262926
case ASTReader::VersionMismatch: return !(Caps & ASTReader::ARR_VersionMismatch);
29272927
case ASTReader::ConfigurationMismatch:
29282928
return !(Caps & ASTReader::ARR_ConfigurationMismatch);
2929+
case ASTReader::ModuleMismatch:
2930+
return true;
29292931
case ASTReader::HadErrors: return true;
29302932
case ASTReader::Success: return false;
29312933
}
@@ -3020,11 +3022,10 @@ ASTReader::ASTReadResult ASTReader::ReadOptionsBlock(
30203022
}
30213023
}
30223024

3023-
ASTReader::ASTReadResult
3024-
ASTReader::ReadControlBlock(ModuleFile &F,
3025-
SmallVectorImpl<ImportedModule> &Loaded,
3026-
const ModuleFile *ImportedBy,
3027-
unsigned ClientLoadCapabilities) {
3025+
ASTReader::ASTReadResult ASTReader::ReadControlBlock(
3026+
ModuleFile &F, SmallVectorImpl<ImportedModule> &Loaded,
3027+
const ModuleFile *ImportedBy, StringRef ExpectedModuleName,
3028+
unsigned ClientLoadCapabilities) {
30283029
BitstreamCursor &Stream = F.Stream;
30293030

30303031
if (llvm::Error Err = Stream.EnterSubBlock(CONTROL_BLOCK_ID)) {
@@ -3315,7 +3316,7 @@ ASTReader::ReadControlBlock(ModuleFile &F,
33153316

33163317
// Load the AST file.
33173318
auto Result = ReadASTCore(ImportedFile, ImportedKind, ImportLoc, &F,
3318-
Loaded, StoredSize, StoredModTime,
3319+
Loaded, StoredSize, StoredModTime, ImportedName,
33193320
StoredSignature, Capabilities);
33203321

33213322
// If we diagnosed a problem, produce a backtrace.
@@ -3338,7 +3339,10 @@ ASTReader::ReadControlBlock(ModuleFile &F,
33383339
case OutOfDate: return OutOfDate;
33393340
case VersionMismatch: return VersionMismatch;
33403341
case ConfigurationMismatch: return ConfigurationMismatch;
3341-
case HadErrors: return HadErrors;
3342+
case ModuleMismatch:
3343+
return ModuleMismatch;
3344+
case HadErrors:
3345+
return HadErrors;
33423346
case Success: break;
33433347
}
33443348
break;
@@ -3363,6 +3367,14 @@ ASTReader::ReadControlBlock(ModuleFile &F,
33633367
if (Listener)
33643368
Listener->ReadModuleName(F.ModuleName);
33653369

3370+
// Return if the AST unexpectedly contains a different module
3371+
if (F.Kind == MK_PrebuiltModule && !ExpectedModuleName.empty() &&
3372+
F.ModuleName != ExpectedModuleName) {
3373+
Diag(diag::err_module_mismatch)
3374+
<< ExpectedModuleName << F.FileName << F.ModuleName;
3375+
return ASTReadResult::ModuleMismatch;
3376+
}
3377+
33663378
// Validate the AST as soon as we have a name so we can exit early on
33673379
// failure.
33683380
if (ASTReadResult Result = readUnhashedControlBlockOnce())
@@ -4684,6 +4696,15 @@ ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName, ModuleKind Type,
46844696
SourceLocation ImportLoc,
46854697
unsigned ClientLoadCapabilities,
46864698
ModuleFile **NewLoadedModuleFile) {
4699+
return ReadAST(FileName, Type, ImportLoc, ClientLoadCapabilities, "",
4700+
NewLoadedModuleFile);
4701+
}
4702+
4703+
ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName, ModuleKind Type,
4704+
SourceLocation ImportLoc,
4705+
unsigned ClientLoadCapabilities,
4706+
StringRef ExpectedModuleName,
4707+
ModuleFile **NewLoadedModuleFile) {
46874708
llvm::TimeTraceScope scope("ReadAST", FileName);
46884709

46894710
llvm::SaveAndRestore SetCurImportLocRAII(CurrentImportLoc, ImportLoc);
@@ -4702,8 +4723,8 @@ ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName, ModuleKind Type,
47024723
SmallVector<ImportedModule, 4> Loaded;
47034724
if (ASTReadResult ReadResult =
47044725
ReadASTCore(FileName, Type, ImportLoc,
4705-
/*ImportedBy=*/nullptr, Loaded, 0, 0, ASTFileSignature(),
4706-
ClientLoadCapabilities)) {
4726+
/*ImportedBy=*/nullptr, Loaded, 0, 0, ExpectedModuleName,
4727+
ASTFileSignature(), ClientLoadCapabilities)) {
47074728
ModuleMgr.removeModules(ModuleMgr.begin() + NumModules);
47084729

47094730
// If we find that any modules are unusable, the global index is going
@@ -4954,25 +4975,26 @@ static unsigned moduleKindForDiagnostic(ModuleKind Kind) {
49544975
llvm_unreachable("unknown module kind");
49554976
}
49564977

4957-
ASTReader::ASTReadResult
4958-
ASTReader::ReadASTCore(StringRef FileName,
4959-
ModuleKind Type,
4960-
SourceLocation ImportLoc,
4961-
ModuleFile *ImportedBy,
4962-
SmallVectorImpl<ImportedModule> &Loaded,
4963-
off_t ExpectedSize, time_t ExpectedModTime,
4964-
ASTFileSignature ExpectedSignature,
4965-
unsigned ClientLoadCapabilities) {
4978+
ASTReader::ASTReadResult ASTReader::ReadASTCore(
4979+
StringRef FileName, ModuleKind Type, SourceLocation ImportLoc,
4980+
ModuleFile *ImportedBy, SmallVectorImpl<ImportedModule> &Loaded,
4981+
off_t ExpectedSize, time_t ExpectedModTime, StringRef ExpectedModuleName,
4982+
ASTFileSignature ExpectedSignature, unsigned ClientLoadCapabilities) {
49664983
ModuleFile *M;
49674984
std::string ErrorStr;
4968-
ModuleManager::AddModuleResult AddResult
4969-
= ModuleMgr.addModule(FileName, Type, ImportLoc, ImportedBy,
4970-
getGeneration(), ExpectedSize, ExpectedModTime,
4971-
ExpectedSignature, readASTFileSignature,
4972-
M, ErrorStr);
4985+
ModuleManager::AddModuleResult AddResult = ModuleMgr.addModule(
4986+
FileName, Type, ImportLoc, ImportedBy, getGeneration(), ExpectedSize,
4987+
ExpectedModTime, ExpectedSignature, readASTFileSignature, M, ErrorStr);
49734988

49744989
switch (AddResult) {
49754990
case ModuleManager::AlreadyLoaded:
4991+
// Return if the AST unexpectedly contains a different module
4992+
if (Type == MK_PrebuiltModule && !ExpectedModuleName.empty() &&
4993+
M->ModuleName != ExpectedModuleName) {
4994+
Diag(diag::err_module_mismatch)
4995+
<< ExpectedModuleName << FileName << M->ModuleName;
4996+
return ASTReadResult::ModuleMismatch;
4997+
}
49764998
Diag(diag::remark_module_import)
49774999
<< M->ModuleName << M->FileName << (ImportedBy ? true : false)
49785000
<< (ImportedBy ? StringRef(ImportedBy->ModuleName) : StringRef());
@@ -5053,7 +5075,8 @@ ASTReader::ReadASTCore(StringRef FileName,
50535075
switch (Entry.ID) {
50545076
case CONTROL_BLOCK_ID:
50555077
HaveReadControlBlock = true;
5056-
switch (ReadControlBlock(F, Loaded, ImportedBy, ClientLoadCapabilities)) {
5078+
switch (ReadControlBlock(F, Loaded, ImportedBy, ExpectedModuleName,
5079+
ClientLoadCapabilities)) {
50575080
case Success:
50585081
// Check that we didn't try to load a non-module AST file as a module.
50595082
//
@@ -5075,6 +5098,8 @@ ASTReader::ReadASTCore(StringRef FileName,
50755098
case OutOfDate: return OutOfDate;
50765099
case VersionMismatch: return VersionMismatch;
50775100
case ConfigurationMismatch: return ConfigurationMismatch;
5101+
case ModuleMismatch:
5102+
return ModuleMismatch;
50785103
case HadErrors: return HadErrors;
50795104
}
50805105
break;

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
// RUN: rm -rf %t
2+
// RUN: split-file %s %t
3+
// RUN: cd %t
4+
5+
// RUN: %clang_cc1 -std=c++20 -emit-module-interface a.cppm -o a.pcm
6+
// RUN: %clang_cc1 -std=c++20 -emit-module-interface b.cppm -o b.pcm \
7+
// RUN: -fmodule-file=A=a.pcm
8+
9+
// This test addresses issue #132059:
10+
// Bad use of fmodule-file=<name>=<path/to/bmi> previously caused the compiler
11+
// to crash for the following cases:
12+
13+
//--- a.cppm
14+
export module A;
15+
16+
export int a() {
17+
return 41;
18+
}
19+
20+
//--- b.cppm
21+
export module B;
22+
import A;
23+
24+
export int b() {
25+
return a() + 1;
26+
}
27+
28+
// Test that when -fmodule-file=<name>=<path/to/bmi> mistakenly maps a module to
29+
// a BMI which depends on the module, the compiler doesn't crash.
30+
31+
// RUN: not %clang_cc1 -std=c++20 main1.cpp-fmodule-file=B=b.pcm \
32+
// RUN: -fmodule-file=A=b.pcm
33+
34+
//--- main1.cpp
35+
import B;
36+
37+
int main() {
38+
return b();
39+
}
40+
41+
// Test that when -fmodule-file=<name>=<path/to/bmi> mistakenly maps a module
42+
// to a BMI file, and that BMI exposes the parts of the specified module as
43+
// transitive imports, the compiler doesn't crash.
44+
45+
// RUN: not %clang_cc1 -std=c++20 main2.cpp -fmodule-file=A=b.pcm
46+
47+
//--- main2.cpp
48+
import A;
49+
50+
int main() {
51+
return a();
52+
}

0 commit comments

Comments
 (0)