From cb53d9309eb932e40fcdf3609eb050358ede2d17 Mon Sep 17 00:00:00 2001 From: Brad King Date: Mon, 7 Nov 2022 14:18:22 -0500 Subject: block: Fix variable scope protection from modification by subdirectories When `cmStateSnapshot::RaiseScope` raises a variable in to a parent directory scope, it uses `GetBuildsystemDirectoryParent` to find the current top-most scope on the directory's stack. Since commit 3f4e5e8c3d (cmState: Return end snapshot for GetBuildsystemDirectoryParent., 2015-09-01, v3.4.0-rc1~100^2~1), that depends on the `DirectoryEnd` field in the directory's state. However, when variable-only scopes were added by commit 6954c8936f (cmState: Add a VariableScope snapshot type., 2015-08-01, v3.4.0-rc1~179^2~1), we neglected to account for the addition of that field by commit 52dbe654de (cmState: Record the end position of each directory., 2015-08-01, v3.4.0-rc1~251^2~1). Prior to commit 44a2f3f332 (Add new flow-control commands for variables and policies scopes management, 2022-08-05, v3.25.0-rc1~257^2) this problem went unnoticed because there was no way to have a variable scope at the top of a directory's stack while processing a subdirectory. Now the `block()/endblock()` commands enable the behavior, so fix tracking of a variable scope as the top-most scope in a directory. Fixes: #24138 --- Source/cmState.cxx | 1 + Tests/RunCMake/block/Scope-POLICIES.cmake | 10 ++++++++++ Tests/RunCMake/block/Scope-VARIABLES.cmake | 12 +++++++++++- Tests/RunCMake/block/Scope.cmake | 12 +++++++++++- Tests/RunCMake/block/Scope/CMakeLists.txt | 2 ++ 5 files changed, 35 insertions(+), 2 deletions(-) create mode 100644 Tests/RunCMake/block/Scope/CMakeLists.txt diff --git a/Source/cmState.cxx b/Source/cmState.cxx index 3d38e73..869414b 100644 --- a/Source/cmState.cxx +++ b/Source/cmState.cxx @@ -938,6 +938,7 @@ cmStateSnapshot cmState::CreateVariableScopeSnapshot( pos->ScopeParent = originSnapshot.Position; pos->SnapshotType = cmStateEnums::VariableScopeType; pos->Keep = false; + pos->BuildSystemDirectory->DirectoryEnd = pos; pos->PolicyScope = originSnapshot.Position->Policies; assert(originSnapshot.Position->Vars.IsValid()); diff --git a/Tests/RunCMake/block/Scope-POLICIES.cmake b/Tests/RunCMake/block/Scope-POLICIES.cmake index 789b3d9..9536a99 100644 --- a/Tests/RunCMake/block/Scope-POLICIES.cmake +++ b/Tests/RunCMake/block/Scope-POLICIES.cmake @@ -2,6 +2,9 @@ set(VAR1 "OUTER1") set(VAR2 "OUTER2") +set(VARSUB1 "OUTERSUB1") +set(VARSUB2 "OUTERSUB2") + cmake_policy(SET CMP0139 NEW) # create a block with a new scope for policies @@ -9,6 +12,7 @@ block(SCOPE_FOR POLICIES) set(VAR1 "INNER1") unset(VAR2) set(VAR3 "INNER3") + add_subdirectory(Scope) cmake_policy(SET CMP0139 OLD) endblock() @@ -23,6 +27,12 @@ endif() if(NOT DEFINED VAR3 OR NOT VAR3 STREQUAL "INNER3") message(SEND_ERROR "block/endblock: VAR3 has unexpected value: ${VAR3}") endif() +if(NOT DEFINED VARSUB1 OR NOT VARSUB1 STREQUAL "SUBDIR1") + message(SEND_ERROR "block/endblock: VARSUB1 has unexpected value: ${VARSUB1}") +endif() +if(NOT DEFINED VARSUB2 OR NOT VARSUB2 STREQUAL "SUBDIR2") + message(SEND_ERROR "block/endblock: VARSUB2 has unexpected value: ${VARSUB2}") +endif() cmake_policy(GET CMP0139 CMP0139_STATUS) if(NOT CMP0139_STATUS STREQUAL "NEW") diff --git a/Tests/RunCMake/block/Scope-VARIABLES.cmake b/Tests/RunCMake/block/Scope-VARIABLES.cmake index 140e638..ac8da14 100644 --- a/Tests/RunCMake/block/Scope-VARIABLES.cmake +++ b/Tests/RunCMake/block/Scope-VARIABLES.cmake @@ -8,16 +8,20 @@ set(VAR5 "OUTER5") set(VAR6 "CACHE6" CACHE STRING "") set(VAR6 "OUTER6") +set(VARSUB1 "OUTERSUB1") +set(VARSUB2 "OUTERSUB2") + cmake_policy(SET CMP0139 NEW) # create a block with a new scope for variables -block(SCOPE_FOR VARIABLES PROPAGATE VAR3 VAR4 VAR5 VAR6 VAR7) +block(SCOPE_FOR VARIABLES PROPAGATE VAR3 VAR4 VAR5 VAR6 VAR7 VARSUB2) set(VAR1 "INNER1") set(VAR2 "INNER2" PARENT_SCOPE) set(VAR3 "INNER3") unset(VAR4) unset(VAR6) set(VAR7 "INNER7") + add_subdirectory(Scope) cmake_policy(SET CMP0139 OLD) endblock() @@ -45,6 +49,12 @@ endif() if(NOT DEFINED VAR7 OR NOT VAR7 STREQUAL "INNER7") message(SEND_ERROR "block/endblock: VAR7 has unexpected value: ${VAR7}") endif() +if(NOT DEFINED VARSUB1 OR NOT VARSUB1 STREQUAL "OUTERSUB1") + message(SEND_ERROR "block/endblock: VARSUB1 has unexpected value: ${VARSUB1}") +endif() +if(NOT DEFINED VARSUB2 OR NOT VARSUB2 STREQUAL "SUBDIR2") + message(SEND_ERROR "block/endblock: VARSUB2 has unexpected value: ${VARSUB2}") +endif() cmake_policy(GET CMP0139 CMP0139_STATUS) if(NOT CMP0139_STATUS STREQUAL "OLD") diff --git a/Tests/RunCMake/block/Scope.cmake b/Tests/RunCMake/block/Scope.cmake index e1af50a..ef43df6 100644 --- a/Tests/RunCMake/block/Scope.cmake +++ b/Tests/RunCMake/block/Scope.cmake @@ -8,16 +8,20 @@ set(VAR5 "OUTER5") set(VAR6 "CACHE6" CACHE STRING "") set(VAR6 "OUTER6") +set(VARSUB1 "OUTERSUB1") +set(VARSUB2 "OUTERSUB2") + cmake_policy(SET CMP0139 NEW) # create a block with a new scope for variables and policies -block(PROPAGATE VAR3 VAR4 VAR5 VAR6 VAR7) +block(PROPAGATE VAR3 VAR4 VAR5 VAR6 VAR7 VARSUB2) set(VAR1 "INNER1") set(VAR2 "INNER2" PARENT_SCOPE) set(VAR3 "INNER3") unset(VAR4) unset(VAR6) set(VAR7 "INNER7") + add_subdirectory(Scope) cmake_policy(SET CMP0139 OLD) endblock() @@ -45,6 +49,12 @@ endif() if(NOT DEFINED VAR7 OR NOT VAR7 STREQUAL "INNER7") message(SEND_ERROR "block/endblock: VAR6 has unexpected value: ${VAR7}") endif() +if(NOT DEFINED VARSUB1 OR NOT VARSUB1 STREQUAL "OUTERSUB1") + message(SEND_ERROR "block/endblock: VARSUB1 has unexpected value: ${VARSUB1}") +endif() +if(NOT DEFINED VARSUB2 OR NOT VARSUB2 STREQUAL "SUBDIR2") + message(SEND_ERROR "block/endblock: VARSUB2 has unexpected value: ${VARSUB2}") +endif() cmake_policy(GET CMP0139 CMP0139_STATUS) if(NOT CMP0139_STATUS STREQUAL "NEW") diff --git a/Tests/RunCMake/block/Scope/CMakeLists.txt b/Tests/RunCMake/block/Scope/CMakeLists.txt new file mode 100644 index 0000000..afd79e3 --- /dev/null +++ b/Tests/RunCMake/block/Scope/CMakeLists.txt @@ -0,0 +1,2 @@ +set(VARSUB1 "SUBDIR1" PARENT_SCOPE) +set(VARSUB2 "SUBDIR2" PARENT_SCOPE) -- cgit v0.12 From 96ddcbee60ee304efd047bc0e20cf89066877ab5 Mon Sep 17 00:00:00 2001 From: Brad King Date: Mon, 7 Nov 2022 14:33:06 -0500 Subject: cmState: Clarify name of member tracking the active scope in a directory The `DirectoryEnd` member added by commit 52dbe654de (cmState: Record the end position of each directory., 2015-08-01, v3.4.0-rc1~251^2~1) actually tracks the current top-most scope in a directory's stack. This is evidenced by the use case in commit 3f4e5e8c3d (cmState: Return end snapshot for GetBuildsystemDirectoryParent., 2015-09-01, v3.4.0-rc1~100^2~1). Rename the member to `CurrentScope` to clarify this role. --- Source/cmState.cxx | 22 +++++++++++----------- Source/cmStatePrivate.h | 2 +- Source/cmStateSnapshot.cxx | 10 +++++----- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/Source/cmState.cxx b/Source/cmState.cxx index 869414b..e54ccfc 100644 --- a/Source/cmState.cxx +++ b/Source/cmState.cxx @@ -281,7 +281,7 @@ cmStateSnapshot cmState::Reset() it->CompileOptions.clear(); it->LinkOptions.clear(); it->LinkDirectories.clear(); - it->DirectoryEnd = pos; + it->CurrentScope = pos; it->NormalTargetNames.clear(); it->ImportedTargetNames.clear(); it->Properties.Clear(); @@ -819,7 +819,7 @@ cmStateSnapshot cmState::CreateBaseSnapshot() pos->CompileOptionsPosition = 0; pos->LinkOptionsPosition = 0; pos->LinkDirectoriesPosition = 0; - pos->BuildSystemDirectory->DirectoryEnd = pos; + pos->BuildSystemDirectory->CurrentScope = pos; pos->Policies = this->PolicyStack.Root(); pos->PolicyRoot = this->PolicyStack.Root(); pos->PolicyScope = this->PolicyStack.Root(); @@ -846,7 +846,7 @@ cmStateSnapshot cmState::CreateBuildsystemDirectorySnapshot( originSnapshot.Position->BuildSystemDirectory); pos->ExecutionListFile = this->ExecutionListFiles.Push(originSnapshot.Position->ExecutionListFile); - pos->BuildSystemDirectory->DirectoryEnd = pos; + pos->BuildSystemDirectory->CurrentScope = pos; pos->Policies = originSnapshot.Position->Policies; pos->PolicyRoot = originSnapshot.Position->Policies; pos->PolicyScope = originSnapshot.Position->Policies; @@ -876,7 +876,7 @@ cmStateSnapshot cmState::CreateDeferCallSnapshot( pos->ExecutionListFile = this->ExecutionListFiles.Push( originSnapshot.Position->ExecutionListFile, fileName); assert(originSnapshot.Position->Vars.IsValid()); - pos->BuildSystemDirectory->DirectoryEnd = pos; + pos->BuildSystemDirectory->CurrentScope = pos; pos->PolicyScope = originSnapshot.Position->Policies; return { this, pos }; } @@ -891,7 +891,7 @@ cmStateSnapshot cmState::CreateFunctionCallSnapshot( pos->Keep = false; pos->ExecutionListFile = this->ExecutionListFiles.Push( originSnapshot.Position->ExecutionListFile, fileName); - pos->BuildSystemDirectory->DirectoryEnd = pos; + pos->BuildSystemDirectory->CurrentScope = pos; pos->PolicyScope = originSnapshot.Position->Policies; assert(originSnapshot.Position->Vars.IsValid()); cmLinkedTree::iterator origin = originSnapshot.Position->Vars; @@ -910,7 +910,7 @@ cmStateSnapshot cmState::CreateMacroCallSnapshot( pos->ExecutionListFile = this->ExecutionListFiles.Push( originSnapshot.Position->ExecutionListFile, fileName); assert(originSnapshot.Position->Vars.IsValid()); - pos->BuildSystemDirectory->DirectoryEnd = pos; + pos->BuildSystemDirectory->CurrentScope = pos; pos->PolicyScope = originSnapshot.Position->Policies; return { this, pos }; } @@ -925,7 +925,7 @@ cmStateSnapshot cmState::CreateIncludeFileSnapshot( pos->ExecutionListFile = this->ExecutionListFiles.Push( originSnapshot.Position->ExecutionListFile, fileName); assert(originSnapshot.Position->Vars.IsValid()); - pos->BuildSystemDirectory->DirectoryEnd = pos; + pos->BuildSystemDirectory->CurrentScope = pos; pos->PolicyScope = originSnapshot.Position->Policies; return { this, pos }; } @@ -938,7 +938,7 @@ cmStateSnapshot cmState::CreateVariableScopeSnapshot( pos->ScopeParent = originSnapshot.Position; pos->SnapshotType = cmStateEnums::VariableScopeType; pos->Keep = false; - pos->BuildSystemDirectory->DirectoryEnd = pos; + pos->BuildSystemDirectory->CurrentScope = pos; pos->PolicyScope = originSnapshot.Position->Policies; assert(originSnapshot.Position->Vars.IsValid()); @@ -958,7 +958,7 @@ cmStateSnapshot cmState::CreateInlineListFileSnapshot( pos->Keep = true; pos->ExecutionListFile = this->ExecutionListFiles.Push( originSnapshot.Position->ExecutionListFile, fileName); - pos->BuildSystemDirectory->DirectoryEnd = pos; + pos->BuildSystemDirectory->CurrentScope = pos; pos->PolicyScope = originSnapshot.Position->Policies; return { this, pos }; } @@ -970,7 +970,7 @@ cmStateSnapshot cmState::CreatePolicyScopeSnapshot( this->SnapshotData.Push(originSnapshot.Position, *originSnapshot.Position); pos->SnapshotType = cmStateEnums::PolicyScopeType; pos->Keep = false; - pos->BuildSystemDirectory->DirectoryEnd = pos; + pos->BuildSystemDirectory->CurrentScope = pos; pos->PolicyScope = originSnapshot.Position->Policies; return { this, pos }; } @@ -990,7 +990,7 @@ cmStateSnapshot cmState::Pop(cmStateSnapshot const& originSnapshot) prevPos->BuildSystemDirectory->LinkOptions.size(); prevPos->LinkDirectoriesPosition = prevPos->BuildSystemDirectory->LinkDirectories.size(); - prevPos->BuildSystemDirectory->DirectoryEnd = prevPos; + prevPos->BuildSystemDirectory->CurrentScope = prevPos; if (!pos->Keep && this->SnapshotData.IsLast(pos)) { if (pos->Vars != prevPos->Vars) { diff --git a/Source/cmStatePrivate.h b/Source/cmStatePrivate.h index fd46eed..ec14834 100644 --- a/Source/cmStatePrivate.h +++ b/Source/cmStatePrivate.h @@ -62,7 +62,7 @@ struct cmStateDetail::PolicyStackEntry : public cmPolicies::PolicyMap struct cmStateDetail::BuildsystemDirectoryStateType { - cmStateDetail::PositionType DirectoryEnd; + cmStateDetail::PositionType CurrentScope; std::string Location; std::string OutputLocation; diff --git a/Source/cmStateSnapshot.cxx b/Source/cmStateSnapshot.cxx index c51650a..cb5f11f 100644 --- a/Source/cmStateSnapshot.cxx +++ b/Source/cmStateSnapshot.cxx @@ -64,7 +64,7 @@ bool cmStateSnapshot::IsValid() const cmStateSnapshot cmStateSnapshot::GetBuildsystemDirectory() const { - return { this->State, this->Position->BuildSystemDirectory->DirectoryEnd }; + return { this->State, this->Position->BuildSystemDirectory->CurrentScope }; } cmStateSnapshot cmStateSnapshot::GetBuildsystemDirectoryParent() const @@ -76,7 +76,7 @@ cmStateSnapshot cmStateSnapshot::GetBuildsystemDirectoryParent() const cmStateDetail::PositionType parentPos = this->Position->DirectoryParent; if (parentPos != this->State->SnapshotData.Root()) { snapshot = cmStateSnapshot(this->State, - parentPos->BuildSystemDirectory->DirectoryEnd); + parentPos->BuildSystemDirectory->CurrentScope); } return snapshot; @@ -177,9 +177,9 @@ cmPolicies::PolicyStatus cmStateSnapshot::GetPolicy(cmPolicies::PolicyID id, while (true) { assert(dir.IsValid()); cmLinkedTree::iterator leaf = - dir->DirectoryEnd->Policies; + dir->CurrentScope->Policies; cmLinkedTree::iterator root = - dir->DirectoryEnd->PolicyRoot; + dir->CurrentScope->PolicyRoot; for (; leaf != root; ++leaf) { if (parent_scope) { parent_scope = false; @@ -190,7 +190,7 @@ cmPolicies::PolicyStatus cmStateSnapshot::GetPolicy(cmPolicies::PolicyID id, return status; } } - cmStateDetail::PositionType e = dir->DirectoryEnd; + cmStateDetail::PositionType e = dir->CurrentScope; cmStateDetail::PositionType p = e->DirectoryParent; if (p == this->State->SnapshotData.Root()) { break; -- cgit v0.12