diff --git a/Changes.md b/Changes.md index c63ef355458..7b7773e1faf 100644 --- a/Changes.md +++ b/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) ======== diff --git a/include/Gaffer/Context.h b/include/Gaffer/Context.h index 68d3cf565cf..c9842164aca 100644 --- a/include/Gaffer/Context.h +++ b/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. diff --git a/include/Gaffer/Context.inl b/include/Gaffer/Context.inl index 18b9bcb0511..81a8c1304a9 100644 --- a/include/Gaffer/Context.inl +++ b/include/Gaffer/Context.inl @@ -155,12 +155,11 @@ void Context::Value::registerType() template 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::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 ¤tOwner = 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 ); diff --git a/src/Gaffer/Context.cpp b/src/Gaffer/Context.cpp index 91f67733968..02118f681c6 100644 --- a/src/Gaffer/Context.cpp +++ b/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