From 5696dc70d27854425471f22cf672d6fa4165844a Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 20 Oct 2023 11:42:59 +0100 Subject: [PATCH] Filter : Sanitise context used to evaluate `Filter.enabled` Without this, any input connections were evaluated once per location rather than just once, leading to much unnecessary pressure on the hash cache. It also provided a loophole via which you could modify the filter per location in a way that meant that ancestor and descendant matches would be inaccurate. --- Changes.md | 1 + include/GafferScene/Filter.h | 2 ++ python/GafferSceneTest/FilterTest.py | 23 ++++++++++++++++++++ src/GafferScene/Filter.cpp | 32 ++++++++++++++++++++++++++-- 4 files changed, 56 insertions(+), 2 deletions(-) diff --git a/Changes.md b/Changes.md index f4b84d8b3a9..a8441cb5b9e 100644 --- a/Changes.md +++ b/Changes.md @@ -16,6 +16,7 @@ Improvements Fixes ----- +- Filter : Fixed bug which allowed the `scene:path` context variable to "leak" upstream via the `Filter.enabled` plug. This caused unnecessary evaluations of the input, and also provided a loophole via which the filter result could be made inconsistent with respect to descendant and ancestor matches. - Windows : - Fixed a bug preventing anything except strings from being copied and pasted. - Fixed likely cause of crash when resizing Spreadsheet column width (#5296). diff --git a/include/GafferScene/Filter.h b/include/GafferScene/Filter.h index 72fcab98993..6698d3f8580 100644 --- a/include/GafferScene/Filter.h +++ b/include/GafferScene/Filter.h @@ -113,6 +113,8 @@ class GAFFERSCENE_API Filter : public Gaffer::ComputeNode private : + bool enabled( const Gaffer::Context *context ) const; + friend class FilterPlug; static size_t g_firstPlugIndex; diff --git a/python/GafferSceneTest/FilterTest.py b/python/GafferSceneTest/FilterTest.py index ddbc4bdc7b0..c5bb51bff29 100644 --- a/python/GafferSceneTest/FilterTest.py +++ b/python/GafferSceneTest/FilterTest.py @@ -52,5 +52,28 @@ def testInputScene( self ) : GafferScene.Filter.setInputScene( c, p["out"] ) self.assertEqual( GafferScene.Filter.getInputScene( c ), p["out"] ) + def testEnabledPlugContextSanitisation( self ) : + + # Make a graph where `pathFilter.enabled` reads from the + # scene globals. + plane = GafferScene.Plane() + + optionQuery = GafferScene.OptionQuery() + optionQuery["scene"].setInput( plane["out"] ) + query = optionQuery.addQuery( Gaffer.BoolPlug(), "test" ) + + pathFilter = GafferScene.PathFilter() + pathFilter["enabled"] .setInput( optionQuery.outPlugFromQuery( query )["value"] ) + + attributes = GafferScene.StandardAttributes() + attributes["in"].setInput( plane["out"] ) + attributes["filter"].setInput( pathFilter["out"] ) + + # Trigger an evaluation of the filter. We don't need to assert anything + # here, because all tests run with a ContextSanitiser active, and that + # will cause a failure if the filter leaks a context variable like + # `scene:path` into the evaluation of the scene globals. + attributes["out"].attributes( "/plane" ) + if __name__ == "__main__": unittest.main() diff --git a/src/GafferScene/Filter.cpp b/src/GafferScene/Filter.cpp index 5de3115af79..32a111ba44c 100644 --- a/src/GafferScene/Filter.cpp +++ b/src/GafferScene/Filter.cpp @@ -37,6 +37,7 @@ #include "GafferScene/Filter.h" #include "GafferScene/FilterPlug.h" +#include "GafferScene/ScenePlug.h" #include "Gaffer/Context.h" @@ -117,7 +118,7 @@ void Filter::hash( const Gaffer::ValuePlug *output, const Gaffer::Context *conte ComputeNode::hash( output, context, h ); if( output == outPlug() ) { - if( enabledPlug()->getValue() ) + if( enabled( context ) ) { /// \todo In SceneNode (and other enableable nodes) we /// require methods like hashMatch() to call the base class @@ -137,7 +138,7 @@ void Filter::compute( ValuePlug *output, const Context *context ) const if( output == outPlug() ) { unsigned match = IECore::PathMatcher::NoMatch; - if( enabledPlug()->getValue() ) + if( enabled( context ) ) { match = computeMatch( getInputScene( context ), context ); } @@ -166,3 +167,30 @@ unsigned Filter::computeMatch( const ScenePlug *scene, const Gaffer::Context *co { return IECore::PathMatcher::NoMatch; } + +bool Filter::enabled( const Gaffer::Context *context ) const +{ + const BoolPlug *plug = enabledPlug(); + const BoolPlug *sourcePlug = plug->source(); + if( !sourcePlug || sourcePlug->direction() == Plug::Out ) + { + // Value may be computed. We use a global scope for two reasons : + // + // - Because our implementation assumes the result is constant across + // the scene, and allowing it to vary by `scene:path` could produce + // results where AncestorMatch and DescendantMatch are not consistent across locations. + // - To reduce pressure on the hash cache. + // + // > Note : `sourcePlug` will be null if the source is not a BoolPlug. + // > In this case we call `getValue()` on `plug` and it will perform the + // > appropriate type conversion. + ScenePlug::GlobalScope globalScope( context ); + return sourcePlug ? sourcePlug->getValue() : plug->getValue(); + } + else + { + // Value is not computed so context is irrelevant. + // Avoid overhead of context creation. + return sourcePlug->getValue(); + } +}