Merge pull request #5510 from johnhaddon/filterContextLeak

Filter : Sanitise context used to evaluate `Filter.enabled`

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).

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;

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()

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<BoolPlug>();
+	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();
+	}
+}