Skip to content

Commit 7063751

Browse files
doug-walkerdoug-walker
authored
Adsk Contrib - Avoid possible cycles in transforms (AcademySoftwareFoundation#2318)
* Fix potential cycle crash Signed-off-by: Doug Walker <doug.walker@autodesk.com> * Clarify purpose of the various guards Signed-off-by: Doug Walker <doug.walker@autodesk.com> --------- Signed-off-by: Doug Walker <doug.walker@autodesk.com>
1 parent 57a94a8 commit 7063751

4 files changed

Lines changed: 216 additions & 2 deletions

File tree

‎src/OpenColorIO/ContextVariableUtils.cpp‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,10 @@ void LoadEnvironment(EnvMap & map, bool update)
127127
static std::string ResolveContextVariablesImpl(const std::string & str, const EnvMap & map,
128128
UsedEnvs & used, int depth)
129129
{
130-
// Guard against infinite recursion from cyclic variable references.
130+
// Guard against infinite recursion at the string substitution level (e.g. $A expands
131+
// to $B which expands to $A). This is independent from the graph-level guards in
132+
// CollectContextVariables() and Transform.cpp's BuildOps(), which protect against cycles
133+
// in the color space / look / view transform reference graph.
131134
if (depth > 32) return str;
132135

133136
// Early exit if no reserved tokens are found.
@@ -176,11 +179,30 @@ std::string ResolveContextVariables(const std::string & str, const EnvMap & map,
176179
return ResolveContextVariablesImpl(str, map, used, 0);
177180
}
178181

179-
bool CollectContextVariables(const Config & config,
182+
bool CollectContextVariables(const Config & config,
180183
const Context & context,
181184
ConstTransformRcPtr transform,
182185
ContextRcPtr & usedContextVars)
183186
{
187+
// Guard against infinite recursion through cycles in the color space / look / view
188+
// transform reference graph (e.g. a ColorSpace whose from_reference is a
189+
// ColorSpaceTransform pointing back to itself). Distinct from the string-level depth
190+
// check in ResolveContextVariablesImpl, which only guards cyclic context-variable
191+
// substitution within a single string; that check fires once per finite resolveStringVar
192+
// call here and never observes this outer graph traversal. A matching guard exists in
193+
// BuildOps() for the op-builder traversal that runs after this one.
194+
static thread_local int depth = 0;
195+
if (depth > 32)
196+
{
197+
throw Exception("Cycle detected while collecting context variables.");
198+
}
199+
struct DepthGuard
200+
{
201+
int & d;
202+
DepthGuard(int & d_) : d(d_) { ++d; }
203+
~DepthGuard() { --d; }
204+
} guard(depth);
205+
184206
if(ConstColorSpaceTransformRcPtr tr = DynamicPtrCast<const ColorSpaceTransform>(transform))
185207
{
186208
if (CollectContextVariables(config, context, *tr, usedContextVars)) return true;

‎src/OpenColorIO/Transform.cpp‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,27 @@ void BuildOps(OpRcPtrVec & ops,
4949
if(!transform)
5050
return;
5151

52+
// Guard against infinite recursion through cycles in the color space / look / view
53+
// transform reference graph (e.g. a ColorSpace whose from_reference is a
54+
// ColorSpaceTransform pointing back to itself). A matching guard in
55+
// CollectContextVariables() catches the same cycles during the earlier context-variable
56+
// scan; this one protects the op-builder traversal. Both are independent from the
57+
// string-level depth check in ResolveContextVariablesImpl, which guards cyclic
58+
// context-variable substitution within a single string. Having the extra guard here
59+
// is necessary for any situations where CollectContextVariables is not called, for
60+
// example, see Config_tests.cpp/cyclic_color_space_linearity_check.
61+
static thread_local int depth = 0;
62+
if (depth > 32)
63+
{
64+
throw Exception("Cycle detected while building ops from transform.");
65+
}
66+
struct DepthGuard
67+
{
68+
int & d;
69+
DepthGuard(int & d_) : d(d_) { ++d; }
70+
~DepthGuard() { --d; }
71+
} guard(depth);
72+
5273
if(ConstAllocationTransformRcPtr allocationTransform = \
5374
DynamicPtrCast<const AllocationTransform>(transform))
5475
{

‎tests/cpu/Config_tests.cpp‎

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10416,3 +10416,153 @@ OCIO_ADD_TEST(Config, interchange_attributes)
1041610416
}
1041710417
}
1041810418

10419+
OCIO_ADD_TEST(Config, cyclic_color_space_transform)
10420+
{
10421+
// Regression test for a stack overflow triggered by a self-referential
10422+
// ColorSpaceTransform (a ColorSpace whose from_reference is a ColorSpaceTransform
10423+
// that points back to itself). getProcessor() must throw a clean exception
10424+
// rather than recursing until the stack guard page is hit.
10425+
10426+
constexpr char CYCLIC_PROFILE[] =
10427+
"ocio_profile_version: 2\n"
10428+
"roles:\n"
10429+
" default: cs0\n"
10430+
"colorspaces:\n"
10431+
" - !<ColorSpace>\n"
10432+
" name: cs0\n"
10433+
" isdata: true\n"
10434+
" - !<ColorSpace>\n"
10435+
" name: cs1\n"
10436+
" from_scene_reference: !<ColorSpaceTransform> {src: cs0, dst: cs1}\n";
10437+
10438+
std::istringstream is;
10439+
is.str(CYCLIC_PROFILE);
10440+
OCIO::ConstConfigRcPtr config;
10441+
OCIO_CHECK_NO_THROW(config = OCIO::Config::CreateFromStream(is));
10442+
10443+
OCIO_CHECK_THROW_WHAT(config->getProcessor("cs0", "cs1"),
10444+
OCIO::Exception,
10445+
"Cycle detected");
10446+
}
10447+
10448+
OCIO_ADD_TEST(Config, cyclic_color_space_two_step)
10449+
{
10450+
// Two-step cycle: cs1 -> cs2 -> cs1. Verify the depth guard catches indirect cycles too.
10451+
10452+
constexpr char CYCLIC_PROFILE[] =
10453+
"ocio_profile_version: 2\n"
10454+
"roles:\n"
10455+
" default: cs0\n"
10456+
"colorspaces:\n"
10457+
" - !<ColorSpace>\n"
10458+
" name: cs0\n"
10459+
" isdata: true\n"
10460+
" - !<ColorSpace>\n"
10461+
" name: cs1\n"
10462+
" from_scene_reference: !<ColorSpaceTransform> {src: cs0, dst: cs2}\n"
10463+
" - !<ColorSpace>\n"
10464+
" name: cs2\n"
10465+
" from_scene_reference: !<ColorSpaceTransform> {src: cs0, dst: cs1}\n";
10466+
10467+
std::istringstream is;
10468+
is.str(CYCLIC_PROFILE);
10469+
OCIO::ConstConfigRcPtr config;
10470+
OCIO_CHECK_NO_THROW(config = OCIO::Config::CreateFromStream(is));
10471+
10472+
OCIO_CHECK_THROW_WHAT(config->getProcessor("cs0", "cs1"),
10473+
OCIO::Exception,
10474+
"Cycle detected");
10475+
}
10476+
10477+
OCIO_ADD_TEST(Config, cyclic_look_transform)
10478+
{
10479+
// Regression test: a Look whose transform is a LookTransform that references the
10480+
// same look must not crash via stack overflow on getProcessor(). Triggered here
10481+
// by a ColorSpace whose from_scene_reference is a LookTransform invoking lookA,
10482+
// whose own transform is another LookTransform invoking lookA.
10483+
10484+
constexpr char CYCLIC_PROFILE[] =
10485+
"ocio_profile_version: 2\n"
10486+
"roles:\n"
10487+
" default: cs0\n"
10488+
"looks:\n"
10489+
" - !<Look>\n"
10490+
" name: lookA\n"
10491+
" process_space: cs0\n"
10492+
" transform: !<LookTransform> {src: cs0, dst: cs0, looks: lookA}\n"
10493+
"colorspaces:\n"
10494+
" - !<ColorSpace>\n"
10495+
" name: cs0\n"
10496+
" - !<ColorSpace>\n"
10497+
" name: cs1\n"
10498+
" from_scene_reference: !<LookTransform> {src: cs0, dst: cs0, looks: lookA}\n";
10499+
10500+
std::istringstream is;
10501+
is.str(CYCLIC_PROFILE);
10502+
OCIO::ConstConfigRcPtr config;
10503+
OCIO_CHECK_NO_THROW(config = OCIO::Config::CreateFromStream(is));
10504+
10505+
OCIO_CHECK_THROW_WHAT(config->getProcessor("cs0", "cs1"),
10506+
OCIO::Exception,
10507+
"Cycle detected");
10508+
}
10509+
10510+
OCIO_ADD_TEST(Config, cyclic_color_space_linearity_check)
10511+
{
10512+
// Regression test for the BuildOps depth guard. Config::isColorSpaceLinear() reaches
10513+
// the op-builder via Config::Impl::getProcessorWithoutCaching(), which does NOT call
10514+
// CollectContextVariables() first. So even with the CollectContextVariables guard,
10515+
// this path will still infinite-recurse on a cyclic color space unless BuildOps
10516+
// independently guards against it.
10517+
10518+
constexpr char CYCLIC_PROFILE[] =
10519+
"ocio_profile_version: 2\n"
10520+
"roles:\n"
10521+
" default: cs0\n"
10522+
"colorspaces:\n"
10523+
" - !<ColorSpace>\n"
10524+
" name: cs0\n"
10525+
" - !<ColorSpace>\n"
10526+
" name: cs1\n"
10527+
" from_scene_reference: !<ColorSpaceTransform> {src: cs0, dst: cs1}\n";
10528+
10529+
std::istringstream is;
10530+
is.str(CYCLIC_PROFILE);
10531+
OCIO::ConstConfigRcPtr config;
10532+
OCIO_CHECK_NO_THROW(config = OCIO::Config::CreateFromStream(is));
10533+
10534+
OCIO_CHECK_THROW_WHAT(config->isColorSpaceLinear("cs1", OCIO::REFERENCE_SPACE_SCENE),
10535+
OCIO::Exception,
10536+
"Cycle detected");
10537+
}
10538+
10539+
OCIO_ADD_TEST(Config, cyclic_display_view_transform)
10540+
{
10541+
// Regression test: a ColorSpace whose from_scene_reference is a DisplayViewTransform
10542+
// that ultimately points back to the same ColorSpace (via the view's color space)
10543+
// must not crash via stack overflow on getProcessor().
10544+
10545+
constexpr char CYCLIC_PROFILE[] =
10546+
"ocio_profile_version: 2\n"
10547+
"roles:\n"
10548+
" default: cs0\n"
10549+
"displays:\n"
10550+
" D1:\n"
10551+
" - !<View> {name: V1, colorspace: cs_loop}\n"
10552+
"colorspaces:\n"
10553+
" - !<ColorSpace>\n"
10554+
" name: cs0\n"
10555+
" - !<ColorSpace>\n"
10556+
" name: cs_loop\n"
10557+
" from_scene_reference: !<DisplayViewTransform> {src: cs0, display: D1, view: V1}\n";
10558+
10559+
std::istringstream is;
10560+
is.str(CYCLIC_PROFILE);
10561+
OCIO::ConstConfigRcPtr config;
10562+
OCIO_CHECK_NO_THROW(config = OCIO::Config::CreateFromStream(is));
10563+
10564+
OCIO_CHECK_THROW_WHAT(config->getProcessor("cs0", "cs_loop"),
10565+
OCIO::Exception,
10566+
"Cycle detected");
10567+
}
10568+

‎tests/python/ConfigTest.py‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1446,6 +1446,27 @@ def test_active__displayview_lists(self):
14461446
config.addActiveView(view="v1")
14471447
self.assertEqual(config.getNumActiveViews(), 1)
14481448

1449+
def test_cyclic_color_space_transform(self):
1450+
"""
1451+
Regression test: a ColorSpace whose from_scene_reference is a
1452+
ColorSpaceTransform pointing back to itself must not crash the
1453+
process via stack overflow on getProcessor().
1454+
"""
1455+
CYCLIC_PROFILE = """ocio_profile_version: 2
1456+
roles:
1457+
default: cs0
1458+
colorspaces:
1459+
- !<ColorSpace>
1460+
name: cs0
1461+
isdata: true
1462+
- !<ColorSpace>
1463+
name: cs1
1464+
from_scene_reference: !<ColorSpaceTransform> {src: cs0, dst: cs1}
1465+
"""
1466+
cfg = OCIO.Config.CreateFromStream(CYCLIC_PROFILE)
1467+
with self.assertRaises(OCIO.Exception):
1468+
cfg.getProcessor("cs0", "cs1")
1469+
14491470

14501471
class ConfigVirtualWithActiveDisplayTest(unittest.TestCase):
14511472
def setUp(self):

0 commit comments

Comments
 (0)