Merge pull request #5679 from johnhaddon/contextFix

Context : Fix heap read after free

Changes.md

@@ -1,6 +1,11 @@
 1.2.10.x (relative to 1.2.10.5)
 ========
 
+Fixes
+-----
+
+- Context : Fixed potential crash when setting a variable with ownership.
+
 1.2.10.5 (relative to 1.2.10.4)
 ========
 

include/Gaffer/Context.h

@@ -357,9 +357,11 @@ class GAFFER_API Context : public IECore::RefCounted
 		};
 
 		// Sets a variable and emits `changedSignal()` as appropriate. Does not
-		// manage ownership in any way. If ownership is required, the caller must
-		// update `m_allocMap` appropriately _before_ calling `internalSet()`.
+		// manage ownership in any way. If ownership is required, call
+		// `internalSetWithOwner()` instead.
 		void internalSet( const IECore::InternedString &name, const Value &value );
+		// Sets a variable and maintains ownership of its data via `owner`.
+		void internalSetWithOwner( const IECore::InternedString &name, const Value &value, IECore::ConstDataPtr &&owner );
 		// Throws if variable doesn't exist.
 		const Value &internalGet( const IECore::InternedString &name ) const;
 		// Returns nullptr if variable doesn't exist.

include/Gaffer/Context.inl

@@ -155,12 +155,11 @@ void Context::Value::registerType()
 template<typename T, typename Enabler>
 void Context::set( const IECore::InternedString &name, const T &value )
 {
-	// Allocate a new typed Data, store it in m_allocMap so that it won't be deallocated,
-	// and call internalSet to reference it in the main m_map
 	using DataType = typename Gaffer::Detail::DataTraits<T>::DataType;
-	typename DataType::Ptr d = new DataType( value );
-	m_allocMap[name] = d;
-	internalSet( name, Value( name, &d->readable() ) );
+	typename DataType::ConstPtr d = new DataType( value );
+	const Value v( name, &d->readable() );
+	internalSetWithOwner( name, v, std::move( d ) );
+
 }
 
 inline void Context::internalSet( const IECore::InternedString &name, const Value &value )
@@ -191,6 +190,18 @@ inline void Context::internalSet( const IECore::InternedString &name, const Valu
 	}
 }
 
+inline void Context::internalSetWithOwner( const IECore::InternedString &name, const Value &value, IECore::ConstDataPtr &&owner )
+{
+	IECore::ConstDataPtr &currentOwner = m_allocMap[name];
+	// Keep old value alive for comparison with new value in `internalSet()`.
+	IECore::ConstDataPtr oldOwner( std::move( currentOwner ) );
+	// Assign new owner so that we have a consistent internal state when
+	// `internalSet()` emits `changedSignal()`.
+	currentOwner = owner;
+	// Update `m_map`.
+	internalSet( name, value );
+}
+
 inline const Context::Value &Context::internalGet( const IECore::InternedString &name ) const
 {
 	const Value *result = internalGetIfExists( name );

src/Gaffer/Context.cpp

@@ -270,15 +270,15 @@ Context::Context( const Context &other, CopyMode mode )
 			)
 			{
 				// The value is already owned by `other`, and is immutable, so we
-				// can just add our own reference to it to share ownership
-				// and then call `internalSet()`.
-				m_allocMap[i.first] = allocIt->second;
-				internalSet( i.first, i.second );
+				// can just share ownership with it.
+				internalSetWithOwner( i.first, i.second, ConstDataPtr( allocIt->second ) );
 			}
 			else
 			{
 				// Data not owned by `other`. Take a copy that we own, and call `internalSet()`.
-				internalSet( i.first, i.second.copy( m_allocMap[i.first] ) );
+				ConstDataPtr owner;
+				const Value v = i.second.copy( owner );
+				internalSetWithOwner( i.first, v, std::move( owner ) );
 			}
 		}
 	}
@@ -312,8 +312,8 @@ void Context::set( const IECore::InternedString &name, const IECore::Data *value
 {
 	// We copy the value so that the client can't invalidate this context by changing it.
 	ConstDataPtr copy = value->copy();
-	m_allocMap[name] = copy;
-	internalSet( name, Value( name, copy.get() ) );
+	const Value v( name, copy.get() );
+	internalSetWithOwner( name, v, std::move( copy ) );
 }
 
 IECore::DataPtr Context::getAsData( const IECore::InternedString &name ) const