From 1693d00383b992ace87b01393f7aca813f784be9 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 16 Feb 2024 17:04:26 +0000 Subject: [PATCH 1/2] Context : Fix heap read after free This was introduced in 520c7a35d8818dba0a9f37bd36fbf8ce3dd2678d, and detected by running `ContextTest` and/or `ScriptNodeTest` using an ASAN-instrumented build. The issue was that `internalSet()` compares the new value against the (non-owning) old value stored in `m_map`, but the memory that points to has been deleted already by the assignment to `m_allocMap` (because assignment to `intrusive_ptr` decrements the reference count for the previous pointee). The solution is to temporarily hold on to the old value until after calling `internalSet()`, and to do all the `intrusive_ptr` juggling using `std::move()` so we steal references rather than do unnecessary increment/decrement pairs. The one other place we call `internalSet()` while using `m_allocMap` is in the Context copy constructor, where we are OK, because `m_allocMap` starts out empty. --- Changes.md | 5 +++++ include/Gaffer/Context.h | 4 +++- include/Gaffer/Context.inl | 15 ++++++++++----- src/Gaffer/Context.cpp | 12 ++++++++---- 4 files changed, 26 insertions(+), 10 deletions(-) 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..8e9ef70eb6f 100644 --- a/include/Gaffer/Context.h +++ b/include/Gaffer/Context.h @@ -358,7 +358,9 @@ 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()`. + // update `m_allocMap` appropriately _before_ calling `internalSet()`, _and_ + // must keep any previous allocation alive until `internalSet()` has + // returned. void internalSet( const IECore::InternedString &name, const Value &value ); // Throws if variable doesn't exist. const Value &internalGet( const IECore::InternedString &name ) const; diff --git a/include/Gaffer/Context.inl b/include/Gaffer/Context.inl index 18b9bcb0511..4866e628af9 100644 --- a/include/Gaffer/Context.inl +++ b/include/Gaffer/Context.inl @@ -155,12 +155,17 @@ 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 + IECore::ConstDataPtr &ownedValue = m_allocMap[name]; + // Keep old value alive for comparison with new value in `internalSet()`. + IECore::ConstDataPtr oldValue( std::move( ownedValue ) ); + // Allocate new Data to own a copy of the value. 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 T *v = &d->readable(); + ownedValue = std::move( d ); + // Update `m_map`. + internalSet( name, Value( name, v ) ); + } inline void Context::internalSet( const IECore::InternedString &name, const Value &value ) diff --git a/src/Gaffer/Context.cpp b/src/Gaffer/Context.cpp index 91f67733968..6cc7805e562 100644 --- a/src/Gaffer/Context.cpp +++ b/src/Gaffer/Context.cpp @@ -310,10 +310,14 @@ Context::~Context() 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() ) ); + IECore::ConstDataPtr &ownedValue = m_allocMap[name]; + // Keep old value alive for comparison with new value in `internalSet()`. + IECore::ConstDataPtr oldValue( std::move( ownedValue ) ); + // Take ownership of a copy of the value so that the client can't invalidate + // this context by changing it. + ownedValue = value->copy(); + // Update `m_map`. + internalSet( name, Value( name, ownedValue.get() ) ); } IECore::DataPtr Context::getAsData( const IECore::InternedString &name ) const From 87303cd22875dfa2fc479d7b1a080a4e8bb9c410 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Mon, 19 Feb 2024 14:43:33 +0000 Subject: [PATCH 2/2] fixup! Context : Fix heap read after free --- include/Gaffer/Context.h | 8 ++++---- include/Gaffer/Context.inl | 22 ++++++++++++++-------- src/Gaffer/Context.cpp | 22 +++++++++------------- 3 files changed, 27 insertions(+), 25 deletions(-) diff --git a/include/Gaffer/Context.h b/include/Gaffer/Context.h index 8e9ef70eb6f..c9842164aca 100644 --- a/include/Gaffer/Context.h +++ b/include/Gaffer/Context.h @@ -357,11 +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()`, _and_ - // must keep any previous allocation alive until `internalSet()` has - // returned. + // 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 4866e628af9..81a8c1304a9 100644 --- a/include/Gaffer/Context.inl +++ b/include/Gaffer/Context.inl @@ -155,16 +155,10 @@ void Context::Value::registerType() template void Context::set( const IECore::InternedString &name, const T &value ) { - IECore::ConstDataPtr &ownedValue = m_allocMap[name]; - // Keep old value alive for comparison with new value in `internalSet()`. - IECore::ConstDataPtr oldValue( std::move( ownedValue ) ); - // Allocate new Data to own a copy of the value. using DataType = typename Gaffer::Detail::DataTraits::DataType; typename DataType::ConstPtr d = new DataType( value ); - const T *v = &d->readable(); - ownedValue = std::move( d ); - // Update `m_map`. - internalSet( name, Value( name, v ) ); + const Value v( name, &d->readable() ); + internalSetWithOwner( name, v, std::move( d ) ); } @@ -196,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 6cc7805e562..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 ) ); } } } @@ -310,14 +310,10 @@ Context::~Context() void Context::set( const IECore::InternedString &name, const IECore::Data *value ) { - IECore::ConstDataPtr &ownedValue = m_allocMap[name]; - // Keep old value alive for comparison with new value in `internalSet()`. - IECore::ConstDataPtr oldValue( std::move( ownedValue ) ); - // Take ownership of a copy of the value so that the client can't invalidate - // this context by changing it. - ownedValue = value->copy(); - // Update `m_map`. - internalSet( name, Value( name, ownedValue.get() ) ); + // We copy the value so that the client can't invalidate this context by changing it. + ConstDataPtr copy = value->copy(); + const Value v( name, copy.get() ); + internalSetWithOwner( name, v, std::move( copy ) ); } IECore::DataPtr Context::getAsData( const IECore::InternedString &name ) const