diff options
author | Brad King <brad.king@kitware.com> | 2020-03-18 13:51:46 (GMT) |
---|---|---|
committer | Brad King <brad.king@kitware.com> | 2020-03-19 10:41:39 (GMT) |
commit | 8affe9aa336b873e9c8e40ec5911ffe23c2ef03a (patch) | |
tree | 0d2b79f86c604c23d3ceff99a69a93fcb9b6c38a /Source | |
parent | 1ec72e09471287630cf142d8587a9b8d9abad629 (diff) | |
download | CMake-8affe9aa336b873e9c8e40ec5911ffe23c2ef03a.zip CMake-8affe9aa336b873e9c8e40ec5911ffe23c2ef03a.tar.gz CMake-8affe9aa336b873e9c8e40ec5911ffe23c2ef03a.tar.bz2 |
export: Fix use-after-free on multiple calls overwriting same FILE
CMake 3.16 and below allow multiple `export()` calls with the same output
file even without using `APPEND`. The implementation worked by accident
by leaking memory. Refactoring in commit 5444a8095d (cmGlobalGenerator:
modernize memrory managemenbt, 2019-12-29, v3.17.0-rc1~239^2) cleaned up
that memory leak and converted it to a use-after-free instead.
The problem is caused by using the `cmGlobalGenerator::BuildExportSets`
map to own `cmExportBuildFileGenerator` instances. It can own only
one instance per output FILE name at a time, so repeating use of the
same file now frees the old `cmExportBuildFileGenerator` instance
and leaves the pointer in the `cmMakefile::ExportBuildFileGenerators`
vector dangling. Move ownership of the instances into `cmMakefile`'s
vector since its entries are not replaced on a repeat output FILE.
In future work we should introduce a policy to error out on this case.
For now simply fix the use-after-free to restore CMake <= 3.16 behavior.
Fixes: #20469
Diffstat (limited to 'Source')
-rw-r--r-- | Source/cmExportCommand.cxx | 6 | ||||
-rw-r--r-- | Source/cmGlobalGenerator.cxx | 19 | ||||
-rw-r--r-- | Source/cmGlobalGenerator.h | 10 | ||||
-rw-r--r-- | Source/cmMakefile.cxx | 16 | ||||
-rw-r--r-- | Source/cmMakefile.h | 10 |
5 files changed, 33 insertions, 28 deletions
diff --git a/Source/cmExportCommand.cxx b/Source/cmExportCommand.cxx index b7cc193..e49c174 100644 --- a/Source/cmExportCommand.cxx +++ b/Source/cmExportCommand.cxx @@ -198,7 +198,6 @@ bool cmExportCommand(std::vector<std::string> const& args, } else { ebfg->SetTargets(targets); } - mf.AddExportBuildFileGenerator(ebfg.get()); ebfg->SetExportOld(arguments.ExportOld); // Compute the set of configurations exported. @@ -211,10 +210,11 @@ bool cmExportCommand(std::vector<std::string> const& args, ebfg->AddConfiguration(ct); } if (exportSet != nullptr) { - gg->AddBuildExportExportSet(std::move(ebfg)); + gg->AddBuildExportExportSet(ebfg.get()); } else { - gg->AddBuildExportSet(std::move(ebfg)); + gg->AddBuildExportSet(ebfg.get()); } + mf.AddExportBuildFileGenerator(std::move(ebfg)); return true; } diff --git a/Source/cmGlobalGenerator.cxx b/Source/cmGlobalGenerator.cxx index 0404715..6a2d4c7 100644 --- a/Source/cmGlobalGenerator.cxx +++ b/Source/cmGlobalGenerator.cxx @@ -262,17 +262,16 @@ void cmGlobalGenerator::ResolveLanguageCompiler(const std::string& lang, } } -void cmGlobalGenerator::AddBuildExportSet( - std::unique_ptr<cmExportBuildFileGenerator> gen) +void cmGlobalGenerator::AddBuildExportSet(cmExportBuildFileGenerator* gen) { - this->BuildExportSets[gen->GetMainExportFileName()] = std::move(gen); + this->BuildExportSets[gen->GetMainExportFileName()] = gen; } void cmGlobalGenerator::AddBuildExportExportSet( - std::unique_ptr<cmExportBuildFileGenerator> gen) + cmExportBuildFileGenerator* gen) { - this->BuildExportExportSets[gen->GetMainExportFileName()] = gen.get(); - this->AddBuildExportSet(std::move(gen)); + this->BuildExportExportSets[gen->GetMainExportFileName()] = gen; + this->AddBuildExportSet(gen); } bool cmGlobalGenerator::GenerateImportFile(const std::string& file) @@ -283,7 +282,7 @@ bool cmGlobalGenerator::GenerateImportFile(const std::string& file) if (!this->ConfigureDoneCMP0026AndCMP0024) { for (const auto& m : this->Makefiles) { - m->RemoveExportBuildFileGeneratorCMP0024(it->second.get()); + m->RemoveExportBuildFileGeneratorCMP0024(it->second); } } @@ -1317,7 +1316,7 @@ cmExportBuildFileGenerator* cmGlobalGenerator::GetExportedTargetsFile( const std::string& filename) const { auto const it = this->BuildExportSets.find(filename); - return it == this->BuildExportSets.end() ? nullptr : it->second.get(); + return it == this->BuildExportSets.end() ? nullptr : it->second; } void cmGlobalGenerator::AddCMP0042WarnTarget(const std::string& target) @@ -1353,9 +1352,9 @@ bool cmGlobalGenerator::CheckALLOW_DUPLICATE_CUSTOM_TARGETS() const void cmGlobalGenerator::ComputeBuildFileGenerators() { for (unsigned int i = 0; i < this->LocalGenerators.size(); ++i) { - std::vector<cmExportBuildFileGenerator*> gens = + std::vector<std::unique_ptr<cmExportBuildFileGenerator>> const& gens = this->Makefiles[i]->GetExportBuildFileGenerators(); - for (cmExportBuildFileGenerator* g : gens) { + for (std::unique_ptr<cmExportBuildFileGenerator> const& g : gens) { g->Compute(this->LocalGenerators[i].get()); } } diff --git a/Source/cmGlobalGenerator.h b/Source/cmGlobalGenerator.h index ba997b2..7dc4822 100644 --- a/Source/cmGlobalGenerator.h +++ b/Source/cmGlobalGenerator.h @@ -463,13 +463,12 @@ public: void ProcessEvaluationFiles(); - std::map<std::string, std::unique_ptr<cmExportBuildFileGenerator>>& - GetBuildExportSets() + std::map<std::string, cmExportBuildFileGenerator*>& GetBuildExportSets() { return this->BuildExportSets; } - void AddBuildExportSet(std::unique_ptr<cmExportBuildFileGenerator>); - void AddBuildExportExportSet(std::unique_ptr<cmExportBuildFileGenerator>); + void AddBuildExportSet(cmExportBuildFileGenerator* gen); + void AddBuildExportExportSet(cmExportBuildFileGenerator* gen); bool IsExportedTargetsFile(const std::string& filename) const; bool GenerateImportFile(const std::string& file); cmExportBuildFileGenerator* GetExportedTargetsFile( @@ -580,8 +579,7 @@ protected: std::set<std::string> InstallComponents; // Sets of named target exports cmExportSetMap ExportSets; - std::map<std::string, std::unique_ptr<cmExportBuildFileGenerator>> - BuildExportSets; + std::map<std::string, cmExportBuildFileGenerator*> BuildExportSets; std::map<std::string, cmExportBuildFileGenerator*> BuildExportExportSets; std::map<std::string, std::string> AliasTargets; diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index b2e59bd..18689fa 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -32,6 +32,7 @@ #include "cmCustomCommandLines.h" #include "cmExecutionStatus.h" #include "cmExpandedCommandArgument.h" // IWYU pragma: keep +#include "cmExportBuildFileGenerator.h" #include "cmFileLockPool.h" #include "cmFunctionBlocker.h" #include "cmGeneratedFileStream.h" @@ -780,7 +781,7 @@ cmMakefile::GetEvaluationFiles() const return this->EvaluationFiles; } -std::vector<cmExportBuildFileGenerator*> +std::vector<std::unique_ptr<cmExportBuildFileGenerator>> const& cmMakefile::GetExportBuildFileGenerators() const { return this->ExportBuildFileGenerators; @@ -789,16 +790,21 @@ cmMakefile::GetExportBuildFileGenerators() const void cmMakefile::RemoveExportBuildFileGeneratorCMP0024( cmExportBuildFileGenerator* gen) { - auto it = std::find(this->ExportBuildFileGenerators.begin(), - this->ExportBuildFileGenerators.end(), gen); + auto it = + std::find_if(this->ExportBuildFileGenerators.begin(), + this->ExportBuildFileGenerators.end(), + [gen](std::unique_ptr<cmExportBuildFileGenerator> const& p) { + return p.get() == gen; + }); if (it != this->ExportBuildFileGenerators.end()) { this->ExportBuildFileGenerators.erase(it); } } -void cmMakefile::AddExportBuildFileGenerator(cmExportBuildFileGenerator* gen) +void cmMakefile::AddExportBuildFileGenerator( + std::unique_ptr<cmExportBuildFileGenerator> gen) { - this->ExportBuildFileGenerators.push_back(gen); + this->ExportBuildFileGenerators.emplace_back(std::move(gen)); } namespace { diff --git a/Source/cmMakefile.h b/Source/cmMakefile.h index f1a68c2..d918abe 100644 --- a/Source/cmMakefile.h +++ b/Source/cmMakefile.h @@ -945,10 +945,11 @@ public: const std::vector<std::unique_ptr<cmGeneratorExpressionEvaluationFile>>& GetEvaluationFiles() const; - std::vector<cmExportBuildFileGenerator*> GetExportBuildFileGenerators() - const; + std::vector<std::unique_ptr<cmExportBuildFileGenerator>> const& + GetExportBuildFileGenerators() const; void RemoveExportBuildFileGeneratorCMP0024(cmExportBuildFileGenerator* gen); - void AddExportBuildFileGenerator(cmExportBuildFileGenerator* gen); + void AddExportBuildFileGenerator( + std::unique_ptr<cmExportBuildFileGenerator> gen); // Maintain a stack of package roots to allow nested PACKAGE_ROOT_PATH // searches @@ -1053,7 +1054,8 @@ private: mutable cmsys::RegularExpression cmNamedCurly; std::vector<cmMakefile*> UnConfiguredDirectories; - std::vector<cmExportBuildFileGenerator*> ExportBuildFileGenerators; + std::vector<std::unique_ptr<cmExportBuildFileGenerator>> + ExportBuildFileGenerators; std::vector<std::unique_ptr<cmGeneratorExpressionEvaluationFile>> EvaluationFiles; |