cmList: Fix shared state across nested list(TRANSFORM) calls

The TRANSFORM action registry held one live action instance per action for
the whole process, and each instance carried per-call state: a raw Selector
pointer, REPLACE's helper, APPEND/PREPEND's operands.  That was harmless
while every action ran to completion uninterrupted, but commit c7af6e94d8
(list(TRANSFORM): Add PREDICATE selector, 2026-04-08, v4.4.0-rc1) added a
selector that runs a user function once per element, interleaved with the
transform.  User code reentering list(TRANSFORM) with the same action now
rebinds the shared instance mid-flight.  That produces silently wrong
results, and a use-after-free once any element is transformed after the
reentering one.  It needs no unusual code to hit: a predicate calling
find_package(Python) reaches list(TRANSFORM ... REPLACE) inside FindPython's
own module.

Make action objects immutable and per-call.  Operands and the selector become
constructor arguments, Initialize is deleted, and the registry becomes a
constexpr table of metadata with a MakeTransformAction factory.  The
InSelection guard, duplicated in all eight actions, moves into the base class.

This also closes a second hole in the same machinery.  TransformActionApply
overrode only the vector form of Initialize, so transform(APPLY, "f",
selector) dispatched to the empty two-argument virtual in the base, left
Selector null, and dereferenced it.  With no virtual Initialize left to
inherit, an APPLY action without a cmMakefile is no longer constructible, so
the throwing stub that guarded the vector form is no longer needed.

Close a third, from commit 651f82642c (Add APPLY action for list(TRANSFORM),
2026-04-08, v4.4.0-rc1): the cmMakefile overload performs APPLY
unconditionally but validated only the arity of the action passed to it.
APPEND, PREPEND and APPLY all take one argument, so transform(APPEND, "x",
makefile) passed validation and then silently ran APPLY, calling "x" as a
function instead of appending it.  Reject any action but APPLY up front.
That path is unreachable from CMake code, since HandleTransformCommand only
selects the overload for APPLY, and Tests/CMakeLib has no cmMakefile to drive
it with, so it carries no test.

Document the predicate's evaluation order while here.  It runs once per
element, immediately before that element would be transformed.  The manual
did not state the timing, which matters precisely because a predicate that
reenters list(TRANSFORM) observes the outer call mid-flight.

Fixes: #28031
This commit is contained in:
Mickaël Germain
2026-08-11 11:36:08 -04:00
committed by Brad King
parent d657ecc4ea
commit 3151f7824f
5 changed files with 281 additions and 220 deletions
+2
View File
@@ -371,6 +371,8 @@ Modification
variable. The function must set the output variable to a boolean
value. Standard CMake boolean evaluation is used.
If the function does not set the output variable, it is an error.
The function is evaluated for each element in turn, immediately
before that element would be transformed.
Example:
+162 -220
View File
@@ -9,7 +9,6 @@
#include <cstddef>
#include <functional>
#include <iterator>
#include <set>
#include <stdexcept>
#include <utility>
@@ -621,49 +620,36 @@ private:
class TransformAction
{
public:
// Public because an inherited constructor keeps the base's access.
explicit TransformAction(TransformSelector& selector)
: Selector(selector)
{
}
virtual ~TransformAction() = default;
void Initialize(TransformSelector* selector) { this->Selector = selector; }
virtual void Initialize(TransformSelector*, std::string const&) {}
virtual void Initialize(TransformSelector*, std::string const&,
std::string const&)
std::string operator()(std::string const& s)
{
return this->Selector.InSelection(s) ? this->ApplyTo(s) : s;
}
virtual void Initialize(TransformSelector* selector,
std::vector<std::string> const&)
{
this->Initialize(selector);
}
virtual std::string operator()(std::string const& s) = 0;
protected:
TransformSelector* Selector;
virtual std::string ApplyTo(std::string const& s) = 0;
TransformSelector& Selector;
};
class TransformActionAppend : public TransformAction
{
public:
using TransformAction::Initialize;
void Initialize(TransformSelector* selector,
std::string const& append) override
TransformActionAppend(TransformSelector& selector, std::string append)
: TransformAction(selector)
, Append(std::move(append))
{
TransformAction::Initialize(selector);
this->Append = append;
}
void Initialize(TransformSelector* selector,
std::vector<std::string> const& append) override
{
this->Initialize(selector, append.front());
}
std::string operator()(std::string const& s) override
protected:
std::string ApplyTo(std::string const& s) override
{
if (this->Selector->InSelection(s)) {
return cmStrCat(s, this->Append);
}
return s;
return cmStrCat(s, this->Append);
}
private:
@@ -672,27 +658,16 @@ private:
class TransformActionPrepend : public TransformAction
{
public:
using TransformAction::Initialize;
void Initialize(TransformSelector* selector,
std::string const& prepend) override
TransformActionPrepend(TransformSelector& selector, std::string prepend)
: TransformAction(selector)
, Prepend(std::move(prepend))
{
TransformAction::Initialize(selector);
this->Prepend = prepend;
}
void Initialize(TransformSelector* selector,
std::vector<std::string> const& prepend) override
{
this->Initialize(selector, prepend.front());
}
std::string operator()(std::string const& s) override
protected:
std::string ApplyTo(std::string const& s) override
{
if (this->Selector->InSelection(s)) {
return cmStrCat(this->Prepend, s);
}
return s;
return cmStrCat(this->Prepend, s);
}
private:
@@ -701,64 +676,59 @@ private:
class TransformActionToUpper : public TransformAction
{
public:
std::string operator()(std::string const& s) override
{
if (this->Selector->InSelection(s)) {
return cmSystemTools::UpperCase(s);
}
using TransformAction::TransformAction;
return s;
protected:
std::string ApplyTo(std::string const& s) override
{
return cmSystemTools::UpperCase(s);
}
};
class TransformActionToLower : public TransformAction
{
public:
std::string operator()(std::string const& s) override
{
if (this->Selector->InSelection(s)) {
return cmSystemTools::LowerCase(s);
}
using TransformAction::TransformAction;
return s;
protected:
std::string ApplyTo(std::string const& s) override
{
return cmSystemTools::LowerCase(s);
}
};
class TransformActionStrip : public TransformAction
{
public:
std::string operator()(std::string const& s) override
{
if (this->Selector->InSelection(s)) {
return cmTrimWhitespace(s);
}
using TransformAction::TransformAction;
return s;
protected:
std::string ApplyTo(std::string const& s) override
{
return cmTrimWhitespace(s);
}
};
class TransformActionGenexStrip : public TransformAction
{
public:
std::string operator()(std::string const& s) override
{
if (this->Selector->InSelection(s)) {
return cmGeneratorExpression::Preprocess(
s, cmGeneratorExpression::StripAllGeneratorExpressions);
}
using TransformAction::TransformAction;
return s;
protected:
std::string ApplyTo(std::string const& s) override
{
return cmGeneratorExpression::Preprocess(
s, cmGeneratorExpression::StripAllGeneratorExpressions);
}
};
class TransformActionReplace : public TransformAction
{
public:
using TransformAction::Initialize;
void Initialize(TransformSelector* selector, std::string const& regex,
std::string const& replace) override
TransformActionReplace(TransformSelector& selector, std::string const& regex,
std::string const& replace)
: TransformAction(selector)
// Makefile is legitimately null when cmList is used directly from C++;
// cmStringReplaceHelper handles that.
, ReplaceHelper(cm::make_unique<cmStringReplaceHelper>(regex, replace,
selector.Makefile))
{
TransformAction::Initialize(selector);
this->ReplaceHelper = cm::make_unique<cmStringReplaceHelper>(
regex, replace, selector->Makefile);
if (!this->ReplaceHelper->IsRegularExpressionValid()) {
throw transform_error(
cmStrCat("sub-command TRANSFORM, action REPLACE: Failed to compile "
@@ -770,28 +740,18 @@ public:
this->ReplaceHelper->GetError(), '.'));
}
}
void Initialize(TransformSelector* selector,
std::vector<std::string> const& args) override
protected:
std::string ApplyTo(std::string const& s) override
{
this->Initialize(selector, args[0], args[1]);
}
std::string output;
std::string operator()(std::string const& s) override
{
if (this->Selector->InSelection(s)) {
// Scan through the input for all matches.
std::string output;
if (!this->ReplaceHelper->Replace(s, output)) {
throw transform_error(
cmStrCat("sub-command TRANSFORM, action REPLACE: ",
this->ReplaceHelper->GetError(), '.'));
}
return output;
if (!this->ReplaceHelper->Replace(s, output)) {
throw transform_error(cmStrCat("sub-command TRANSFORM, action REPLACE: ",
this->ReplaceHelper->GetError(), '.'));
}
return s;
return output;
}
private:
@@ -801,34 +761,20 @@ private:
class TransformActionApply : public TransformAction
{
public:
using TransformAction::Initialize;
void Initialize(TransformSelector* selector, std::string const& functionName,
cmMakefile& makefile)
TransformActionApply(TransformSelector& selector, std::string functionName,
cmMakefile& makefile)
: TransformAction(selector)
, FunctionName(std::move(functionName))
, Makefile(&makefile)
, OutputVar(OutputVarFor("_cmake_transform_apply_out_", makefile))
{
TransformAction::Initialize(selector);
this->FunctionName = functionName;
this->Makefile = &makefile;
this->OutputVar = OutputVarFor("_cmake_transform_apply_out_", makefile);
RequireFunction(makefile, this->FunctionName,
"sub-command TRANSFORM, action APPLY");
}
void Initialize(TransformSelector* /*selector*/,
std::vector<std::string> const& /*args*/) override
protected:
std::string ApplyTo(std::string const& s) override
{
// This overload must not be used for APPLY - it lacks cmMakefile context.
throw transform_error(
"sub-command TRANSFORM, action APPLY requires cmMakefile context.");
}
std::string operator()(std::string const& s) override
{
if (!this->Selector->InSelection(s)) {
return s;
}
// Unset the output variable before calling
this->Makefile->RemoveDefinition(this->OutputVar);
@@ -861,7 +807,6 @@ public:
// cmValue pointer).
std::string output = *result;
// Clean up
this->Makefile->RemoveDefinition(this->OutputVar);
return output;
@@ -873,68 +818,44 @@ private:
std::string OutputVar;
};
// Descriptor of action
// Arity: number of arguments required for the action
// Transform: Object implementing the action
// Arity: number of arguments required for the action.
//
// Keep this a bare aggregate of literal types: CMake still builds as C++11,
// where a member initializer, a constructor, or a cm::string_view member
// would break the constexpr table below.
struct ActionDescriptor
{
ActionDescriptor(cmList::TransformAction action)
: Action(action)
{
}
ActionDescriptor(cmList::TransformAction action, std::string name,
std::size_t arity,
std::unique_ptr<TransformAction> transform)
: Action(action)
, Name(std::move(name))
, Arity(arity)
, Transform(std::move(transform))
{
}
operator cmList::TransformAction() const { return this->Action; }
cmList::TransformAction Action;
std::string Name;
std::size_t Arity = 0;
std::unique_ptr<TransformAction> Transform;
char const* Name;
std::size_t Arity;
};
// Build a set of supported actions.
using ActionDescriptorSet = std::set<
ActionDescriptor,
std::function<bool(cmList::TransformAction, cmList::TransformAction)>>;
constexpr ActionDescriptor Descriptors[] = {
{ cmList::TransformAction::APPEND, "APPEND", 1 },
{ cmList::TransformAction::PREPEND, "PREPEND", 1 },
{ cmList::TransformAction::TOUPPER, "TOUPPER", 0 },
{ cmList::TransformAction::TOLOWER, "TOLOWER", 0 },
{ cmList::TransformAction::STRIP, "STRIP", 0 },
{ cmList::TransformAction::GENEX_STRIP, "GENEX_STRIP", 0 },
{ cmList::TransformAction::REPLACE, "REPLACE", 2 },
{ cmList::TransformAction::APPLY, "APPLY", 1 },
};
ActionDescriptorSet Descriptors([](cmList::TransformAction x,
cmList::TransformAction y) {
return x < y;
});
ActionDescriptorSet::iterator TransformConfigure(
ActionDescriptor const& TransformConfigure(
cmList::TransformAction action,
std::unique_ptr<cmList::TransformSelector>& selector, std::size_t arity)
{
if (Descriptors.empty()) {
Descriptors.emplace(cmList::TransformAction::APPEND, "APPEND", 1,
cm::make_unique<TransformActionAppend>());
Descriptors.emplace(cmList::TransformAction::PREPEND, "PREPEND", 1,
cm::make_unique<TransformActionPrepend>());
Descriptors.emplace(cmList::TransformAction::TOUPPER, "TOUPPER", 0,
cm::make_unique<TransformActionToUpper>());
Descriptors.emplace(cmList::TransformAction::TOLOWER, "TOLOWER", 0,
cm::make_unique<TransformActionToLower>());
Descriptors.emplace(cmList::TransformAction::STRIP, "STRIP", 0,
cm::make_unique<TransformActionStrip>());
Descriptors.emplace(cmList::TransformAction::GENEX_STRIP, "GENEX_STRIP", 0,
cm::make_unique<TransformActionGenexStrip>());
Descriptors.emplace(cmList::TransformAction::REPLACE, "REPLACE", 2,
cm::make_unique<TransformActionReplace>());
Descriptors.emplace(cmList::TransformAction::APPLY, "APPLY", 1,
cm::make_unique<TransformActionApply>());
// Not indexed by the enum value: this table is in registration order, and
// cmList.h declares TOLOWER before TOUPPER.
ActionDescriptor const* descriptor = nullptr;
for (auto const& candidate : Descriptors) {
if (candidate.Action == action) {
descriptor = &candidate;
break;
}
}
auto descriptor = Descriptors.find(action);
if (descriptor == Descriptors.end()) {
if (!descriptor) {
throw transform_error(cmStrCat(" sub-command TRANSFORM, ",
static_cast<int>(action),
" invalid action."));
@@ -949,7 +870,56 @@ ActionDescriptorSet::iterator TransformConfigure(
selector = cm::make_unique<TransformNoSelector>();
}
return descriptor;
return *descriptor;
}
// Precondition: TransformConfigure has validated the arity, so args is
// indexed unchecked.
std::unique_ptr<TransformAction> MakeTransformAction(
ActionDescriptor const& descriptor, TransformSelector& selector,
std::vector<std::string> const& args)
{
switch (descriptor.Action) {
case cmList::TransformAction::APPEND:
return cm::make_unique<TransformActionAppend>(selector, args[0]);
case cmList::TransformAction::PREPEND:
return cm::make_unique<TransformActionPrepend>(selector, args[0]);
case cmList::TransformAction::TOUPPER:
return cm::make_unique<TransformActionToUpper>(selector);
case cmList::TransformAction::TOLOWER:
return cm::make_unique<TransformActionToLower>(selector);
case cmList::TransformAction::STRIP:
return cm::make_unique<TransformActionStrip>(selector);
case cmList::TransformAction::GENEX_STRIP:
return cm::make_unique<TransformActionGenexStrip>(selector);
case cmList::TransformAction::REPLACE:
return cm::make_unique<TransformActionReplace>(selector, args[0],
args[1]);
case cmList::TransformAction::APPLY:
// APPLY needs a cmMakefile, which this factory does not receive; only
// the cmMakefile overload of cmList::transform can build it.
break;
}
throw transform_error(
"sub-command TRANSFORM, action APPLY requires cmMakefile context.");
}
void TransformValues(cmList::container_type& values,
cmList::TransformAction action,
std::vector<std::string> const& args,
std::unique_ptr<cmList::TransformSelector>& selector)
{
ActionDescriptor const& descriptor =
TransformConfigure(action, selector, args.size());
auto& sel = static_cast<TransformSelector&>(*selector);
std::unique_ptr<TransformAction> transformer =
MakeTransformAction(descriptor, sel, args);
sel.Transform(values, [&transformer](std::string const& s) -> std::string {
return (*transformer)(s);
});
}
}
@@ -1063,15 +1033,7 @@ cmList::TransformSelector::NewPREDICATE(std::string const& functionName,
cmList& cmList::transform(TransformAction action,
std::unique_ptr<TransformSelector> selector)
{
auto descriptor = TransformConfigure(action, selector, 0);
descriptor->Transform->Initialize(
static_cast<::TransformSelector*>(selector.get()));
static_cast<::TransformSelector&>(*selector).Transform(
this->Values, [&descriptor](std::string const& s) -> std::string {
return (*descriptor->Transform)(s);
});
TransformValues(this->Values, action, {}, selector);
return *this;
}
@@ -1079,15 +1041,7 @@ cmList& cmList::transform(TransformAction action,
cmList& cmList::transform(TransformAction action, std::string const& arg,
std::unique_ptr<TransformSelector> selector)
{
auto descriptor = TransformConfigure(action, selector, 1);
descriptor->Transform->Initialize(
static_cast<::TransformSelector*>(selector.get()), arg);
static_cast<::TransformSelector&>(*selector).Transform(
this->Values, [&descriptor](std::string const& s) -> std::string {
return (*descriptor->Transform)(s);
});
TransformValues(this->Values, action, { arg }, selector);
return *this;
}
@@ -1096,15 +1050,7 @@ cmList& cmList::transform(TransformAction action, std::string const& arg1,
std::string const& arg2,
std::unique_ptr<TransformSelector> selector)
{
auto descriptor = TransformConfigure(action, selector, 2);
descriptor->Transform->Initialize(
static_cast<::TransformSelector*>(selector.get()), arg1, arg2);
static_cast<::TransformSelector&>(*selector).Transform(
this->Values, [&descriptor](std::string const& s) -> std::string {
return (*descriptor->Transform)(s);
});
TransformValues(this->Values, action, { arg1, arg2 }, selector);
return *this;
}
@@ -1113,15 +1059,7 @@ cmList& cmList::transform(TransformAction action,
std::vector<std::string> const& args,
std::unique_ptr<TransformSelector> selector)
{
auto descriptor = TransformConfigure(action, selector, args.size());
descriptor->Transform->Initialize(
static_cast<::TransformSelector*>(selector.get()), args);
static_cast<::TransformSelector&>(*selector).Transform(
this->Values, [&descriptor](std::string const& s) -> std::string {
return (*descriptor->Transform)(s);
});
TransformValues(this->Values, action, args, selector);
return *this;
}
@@ -1130,20 +1068,24 @@ cmList& cmList::transform(TransformAction action, std::string const& arg,
cmMakefile& makefile,
std::unique_ptr<TransformSelector> selector)
{
// Validate action and arity via the static registry.
// This overload performs APPLY unconditionally. Without this check the
// other arity-1 actions, APPEND and PREPEND, would pass the arity
// validation below and then silently run APPLY instead.
if (action != TransformAction::APPLY) {
throw transform_error(
"sub-command TRANSFORM: only action APPLY accepts a cmMakefile.");
}
// Validates the arity and defaults the selector.
TransformConfigure(action, selector, 1);
// Create a local instance rather than reusing the singleton from
// Descriptors. A user function invoked by APPLY may itself call
// list(TRANSFORM ... APPLY ...), which would clobber a shared instance.
TransformActionApply applyAction;
applyAction.Initialize(static_cast<::TransformSelector*>(selector.get()),
arg, makefile);
auto& sel = static_cast<::TransformSelector&>(*selector);
TransformActionApply applyAction(sel, arg, makefile);
static_cast<::TransformSelector&>(*selector).Transform(
this->Values, [&applyAction](std::string const& s) -> std::string {
return applyAction(s);
});
sel.Transform(this->Values,
[&applyAction](std::string const& s) -> std::string {
return applyAction(s);
});
return *this;
}
+25
View File
@@ -790,6 +790,31 @@ bool testTransform()
result = false;
}
}
{
// No cmMakefile through this overload, so APPLY must throw.
cmList list({ "AA", "BB" });
try {
list.transform(cmList::TransformAction::APPLY,
std::vector<std::string>{ "someFunction" });
#ifndef __clang_analyzer__ /* clang-analyzer cannot see throw skips this */
result = false;
#endif
} catch (cmList::transform_error const&) {
}
}
{
// No cmMakefile through this overload, so APPLY must throw.
cmList list({ "AA", "BB" });
try {
list.transform(cmList::TransformAction::APPLY, "someFunction");
#ifndef __clang_analyzer__ /* clang-analyzer cannot see throw skips this */
result = false;
#endif
} catch (cmList::transform_error const&) {
}
}
checkResult(result);
+1
View File
@@ -106,6 +106,7 @@ run_cmake(TRANSFORM-PREPEND)
run_cmake(TRANSFORM-REPLACE)
run_cmake(TRANSFORM-APPLY)
run_cmake(TRANSFORM-PREDICATE)
run_cmake(TRANSFORM-PREDICATE-Reentrant)
run_cmake(CMP0186)
# argument tests
@@ -0,0 +1,91 @@
# A PREDICATE function that reenters list(TRANSFORM) must not corrupt the
# outer transform. The nested action must match the outer one; a different
# action uses separate state. Each case checks the nested list too, so
# repairing the outer call by breaking the inner one still fails.
# REPLACE, single element
function(pred_replace value out)
set(inner "hello")
list(TRANSFORM inner REPLACE "l" "L")
if(NOT inner STREQUAL "heLLo")
message(FATAL_ERROR "nested REPLACE is \"${inner}\", expected \"heLLo\"")
endif()
set(${out} TRUE PARENT_SCOPE)
endfunction()
set(replace_single "aXa")
list(TRANSFORM replace_single REPLACE "X" "Z" PREDICATE pred_replace)
if(NOT replace_single STREQUAL "aZa")
message(FATAL_ERROR "replace_single is \"${replace_single}\", expected \"aZa\"")
endif()
# REPLACE, only the last element reenters
# Nothing follows the reentering element: wrong value, not undefined behavior.
function(pred_replace_last value out)
if(value STREQUAL "cXc")
set(inner "hello")
list(TRANSFORM inner REPLACE "l" "L")
if(NOT inner STREQUAL "heLLo")
message(FATAL_ERROR "nested REPLACE is \"${inner}\", expected \"heLLo\"")
endif()
endif()
set(${out} TRUE PARENT_SCOPE)
endfunction()
set(replace_last "aXa" "bXb" "cXc")
list(TRANSFORM replace_last REPLACE "X" "Z" PREDICATE pred_replace_last)
if(NOT replace_last STREQUAL "aZa;bZb;cZc")
message(FATAL_ERROR "replace_last is \"${replace_last}\", expected \"aZa;bZb;cZc\"")
endif()
# APPEND
# Not redundant with REPLACE: the operand is a plain member, not a
# heap-allocated helper.
function(pred_append value out)
set(inner "q")
list(TRANSFORM inner APPEND "_NESTED")
if(NOT inner STREQUAL "q_NESTED")
message(FATAL_ERROR "nested APPEND is \"${inner}\", expected \"q_NESTED\"")
endif()
set(${out} TRUE PARENT_SCOPE)
endfunction()
set(append_single "a")
list(TRANSFORM append_single APPEND "_OUTER" PREDICATE pred_append)
if(NOT append_single STREQUAL "a_OUTER")
message(FATAL_ERROR "append_single is \"${append_single}\", expected \"a_OUTER\"")
endif()
# PREPEND
function(pred_prepend value out)
set(inner "q")
list(TRANSFORM inner PREPEND "NESTED_")
if(NOT inner STREQUAL "NESTED_q")
message(FATAL_ERROR "nested PREPEND is \"${inner}\", expected \"NESTED_q\"")
endif()
set(${out} TRUE PARENT_SCOPE)
endfunction()
set(prepend_single "a")
list(TRANSFORM prepend_single PREPEND "OUTER_" PREDICATE pred_prepend)
if(NOT prepend_single STREQUAL "OUTER_a")
message(FATAL_ERROR "prepend_single is \"${prepend_single}\", expected \"OUTER_a\"")
endif()
# TOUPPER, every element reenters
# The only case transforming an element after a reentering one, covering a
# selector that outlives the nested call.
function(pred_toupper value out)
set(inner a b)
list(TRANSFORM inner TOUPPER)
if(NOT inner STREQUAL "A;B")
message(FATAL_ERROR "nested TOUPPER is \"${inner}\", expected \"A;B\"")
endif()
set(${out} TRUE PARENT_SCOPE)
endfunction()
set(toupper_all x y z)
list(TRANSFORM toupper_all TOUPPER PREDICATE pred_toupper)
if(NOT toupper_all STREQUAL "X;Y;Z")
message(FATAL_ERROR "toupper_all is \"${toupper_all}\", expected \"X;Y;Z\"")
endif()