diff --git a/include/svx/svdmodel.hxx b/include/svx/svdmodel.hxx index 81ec0927db4c..f029d928aa8a 100644 --- a/include/svx/svdmodel.hxx +++ b/include/svx/svdmodel.hxx @@ -42,11 +42,7 @@ #include #include - -#ifdef DBG_UTIL -// SdrObjectLifetimeWatchDog #include -#endif class OutputDevice; class SdrOutliner; @@ -153,8 +149,9 @@ struct SdrModelImpl; class SVXCORE_DLLPUBLIC SdrModel : public SfxBroadcaster, public tools::WeakBase { -#ifdef DBG_UTIL - // SdrObjectLifetimeWatchDog: + // We need to keep track of all the SdrObjects linked to this SdrModel so that we can + // shut them down cleanly, otherwise we end up with use-after-free issues. + // // Use maAllIncarnatedObjects to keep track of all SdrObjects incarnated using this SdrModel // (what is now possible after the paradigm change that a SdrObject stays at a single SdrModel // for it's whole lifetime). @@ -174,7 +171,6 @@ class SVXCORE_DLLPUBLIC SdrModel : public SfxBroadcaster, public tools::WeakBase friend void impAddIncarnatedSdrObjectToSdrModel(SdrObject& rSdrObject, SdrModel& rSdrModel); friend void impRemoveIncarnatedSdrObjectToSdrModel(SdrObject& rSdrObject, SdrModel& rSdrModel); std::unordered_set< SdrObject* > maAllIncarnatedObjects; -#endif protected: std::vector> maMasterPages; std::vector> maPages; diff --git a/include/svx/unoshape.hxx b/include/svx/unoshape.hxx index a72c3beae1b5..4ae84a2bc74e 100644 --- a/include/svx/unoshape.hxx +++ b/include/svx/unoshape.hxx @@ -226,7 +226,7 @@ public: void setMaster( SvxShapeMaster* pMaster ); // SfxListener - virtual void Notify( SfxBroadcaster& rBC, const SfxHint& rHint ) noexcept override; + virtual void Notify( SfxBroadcaster& rBC, const SfxHint& rHint ) noexcept override final; // XAggregation virtual css::uno::Any SAL_CALL queryAggregation( const css::uno::Type& aType ) override; diff --git a/svx/source/svdraw/svdmodel.cxx b/svx/source/svdraw/svdmodel.cxx index 835689a00726..3c43cf2c81bc 100644 --- a/svx/source/svdraw/svdmodel.cxx +++ b/svx/source/svdraw/svdmodel.cxx @@ -44,6 +44,7 @@ #include #include #include +#include #include #include #include @@ -222,7 +223,6 @@ SdrModel::~SdrModel() implDtorClearModel(); #ifdef DBG_UTIL - // SdrObjectLifetimeWatchDog: if(!maAllIncarnatedObjects.empty()) { SAL_WARN("svx", @@ -580,6 +580,25 @@ void SdrModel::ClearModel(bool bCalledFromDestructor) mbInDestruction = true; } + // Disconnect all SvxShape's from their SdrObjects to prevent the SdrObjects + // from hanging around and causing use-after-free. + // Make a copy because it might modified during InvalidateSdrObject calls. + std::vector> allObjs(maAllIncarnatedObjects.begin(), maAllIncarnatedObjects.end()); + for (const auto & pSdrObj : allObjs) + { + uno::Reference xShape = pSdrObj->getWeakUnoShape().get(); + rtl::Reference pSvxShape = dynamic_cast(xShape.get()); + // calling getWeakUnoShape so we don't accidentally create new UNO shapes + if (pSvxShape) + pSvxShape->InvalidateSdrObject(); + else + { + // because some things like SwXShape don't subclass SvxShape + uno::Reference xComp(xShape, uno::UNO_QUERY); + if (xComp) + xComp->dispose(); + } + } sal_Int32 i; // delete all drawing pages sal_Int32 nCount=GetPageCount(); diff --git a/svx/source/svdraw/svdobj.cxx b/svx/source/svdraw/svdobj.cxx index 6c8330185f0a..1b768aee12d9 100644 --- a/svx/source/svdraw/svdobj.cxx +++ b/svx/source/svdraw/svdobj.cxx @@ -321,8 +321,6 @@ void SdrObject::SetBoundRectDirty() resetOutRectangle(); } -#ifdef DBG_UTIL -// SdrObjectLifetimeWatchDog: void impAddIncarnatedSdrObjectToSdrModel(SdrObject& rSdrObject, SdrModel& rSdrModel) { rSdrModel.maAllIncarnatedObjects.insert(&rSdrObject); @@ -334,7 +332,6 @@ void impRemoveIncarnatedSdrObjectToSdrModel(SdrObject& rSdrObject, SdrModel& rSd assert(false && "SdrObject::~SdrObject: Destructed incarnation of SdrObject not member of this SdrModel (!)"); } } -#endif SdrObject::SdrObject(SdrModel& rSdrModel) : mpFillGeometryDefiningShape(nullptr) @@ -369,10 +366,7 @@ SdrObject::SdrObject(SdrModel& rSdrModel) m_bIs3DObj=false; m_bMarkProt=false; m_bIsUnoObj=false; -#ifdef DBG_UTIL - // SdrObjectLifetimeWatchDog: impAddIncarnatedSdrObjectToSdrModel(*this, getSdrModelFromSdrObject()); -#endif } SdrObject::SdrObject(SdrModel& rSdrModel, SdrObject const & rSource) @@ -440,10 +434,7 @@ SdrObject::SdrObject(SdrModel& rSdrModel, SdrObject const & rSource) m_pGrabBagItem.reset(); if (rSource.m_pGrabBagItem!=nullptr) m_pGrabBagItem.reset(rSource.m_pGrabBagItem->Clone()); -#ifdef DBG_UTIL - // SdrObjectLifetimeWatchDog: impAddIncarnatedSdrObjectToSdrModel(*this, getSdrModelFromSdrObject()); -#endif } SdrObject::~SdrObject() @@ -470,10 +461,7 @@ SdrObject::~SdrObject() m_pGrabBagItem.reset(); mpProperties.reset(); mpViewContact.reset(); -#ifdef DBG_UTIL - // SdrObjectLifetimeWatchDog: impRemoveIncarnatedSdrObjectToSdrModel(*this, getSdrModelFromSdrObject()); -#endif } void SdrObject::acquire() noexcept diff --git a/svx/source/unodraw/unoshape.cxx b/svx/source/unodraw/unoshape.cxx index 5c3807817a6f..b547db4b436a 100644 --- a/svx/source/unodraw/unoshape.cxx +++ b/svx/source/unodraw/unoshape.cxx @@ -192,7 +192,7 @@ SvxShape::~SvxShape() noexcept if ( mxSdrObject ) { - EndListening(mxSdrObject->getSdrModelFromSdrObject()); + mxSdrObject->RemoveListener(*this); mxSdrObject->setUnoShape(nullptr); mxSdrObject.clear(); } @@ -205,7 +205,7 @@ void SvxShape::InvalidateSdrObject() { if(mxSdrObject) { - EndListening(mxSdrObject->getSdrModelFromSdrObject()); + mxSdrObject->RemoveListener(*this); mxSdrObject.clear(); } }; @@ -285,7 +285,7 @@ void SvxShape::impl_construct() { if ( HasSdrObject() ) { - StartListening(GetSdrObject()->getSdrModelFromSdrObject()); + GetSdrObject()->AddListener(*this); impl_initFromSdrObject(); } } @@ -355,14 +355,14 @@ void SvxShape::Create( SdrObject* pNewObj, SvxDrawPage* /*pNewPage*/ ) if( HasSdrObject() ) { - EndListening( GetSdrObject()->getSdrModelFromSdrObject() ); + GetSdrObject()->RemoveListener( *this ); } mxSdrObject = pNewObj; if( HasSdrObject() ) { - StartListening( GetSdrObject()->getSdrModelFromSdrObject() ); + GetSdrObject()->AddListener( *this ); } OSL_ENSURE( !mbIsMultiPropertyCall, "SvxShape::Create: hmm?" ); @@ -929,8 +929,7 @@ void SvxShape::Notify( SfxBroadcaster&, const SfxHint& rHint ) noexcept return; const SdrHint* pSdrHint = static_cast(&rHint); // #i55919# SdrHintKind::ObjectChange is only interesting if it's for this object - if ((pSdrHint->GetKind() != SdrHintKind::ModelCleared) && - (pSdrHint->GetKind() != SdrHintKind::ObjectChange || pSdrHint->GetObject() != mxSdrObject.get() )) + if (pSdrHint->GetKind() != SdrHintKind::ObjectChange || pSdrHint->GetObject() != mxSdrObject.get()) return; // prevent object being deleted from under us @@ -938,24 +937,12 @@ void SvxShape::Notify( SfxBroadcaster&, const SfxHint& rHint ) noexcept uno::Reference< uno::XInterface > xSelf( mxSdrObject->getWeakUnoShape() ); if( !xSelf.is() ) { - EndListening(mxSdrObject->getSdrModelFromSdrObject()); + mxSdrObject->RemoveListener(*this); mxSdrObject.clear(); return; } - if (pSdrHint->GetKind() == SdrHintKind::ObjectChange) - { - updateShapeKind(); - } - else // (pSdrHint->GetKind() == SdrHintKind::ModelCleared) - { - EndListening(mxSdrObject->getSdrModelFromSdrObject()); - mxSdrObject->setUnoShape(nullptr); - mxSdrObject.clear(); - - if(!mpImpl->mbDisposing) - dispose(); - } + updateShapeKind(); } // XShape @@ -1190,7 +1177,7 @@ void SAL_CALL SvxShape::dispose() if (!pObject) return; - EndListening( pObject->getSdrModelFromSdrObject() ); + pObject->RemoveListener( *this ); if ( pObject->IsInserted() && pObject->getSdrPageFromSdrObject() ) {