include/svx/unoshape.hxx | 3
svx/source/unodraw/unoshap2.cxx | 20 ++--
sw/inc/unodraw.hxx | 30 +-----
sw/inc/unotxdoc.hxx | 4
sw/source/core/unocore/unodraw.cxx | 174 +++++++++++--------------------------
sw/source/uibase/uno/unotxdoc.cxx | 11 +-
6 files changed, 85 insertions(+), 157 deletions(-)
New commits:
commit eae7a05b94fcfdf0ae48adab2006fe0b82c95921
Author: Noel Grandin <noel.gran...@collabora.co.uk>
AuthorDate: Tue Sep 13 09:56:15 2022 +0200
Commit: Noel Grandin <noel.gran...@collabora.co.uk>
CommitDate: Tue Sep 13 13:53:24 2022 +0200
ofz#50767 docxfuzzer: Indirect-leak in rtl_allocateMemory
Two fixes required here
(1) the unnecessary use of aggregation in SwXDrawPage, which leads to a
very confusing cycle that I don't fully follow. No indication why this
is necessary in the git history, has been this way since initial commit
(2) a ref-counting cycle through SvxShapeGroup and SvxDrawPage, use a
weak reference here to break the ref-counting cycle.
Change-Id: Ie7ec583960ed0864a073ad8489fb65964bd83080
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/139828
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.gran...@collabora.co.uk>
diff --git a/include/svx/unoshape.hxx b/include/svx/unoshape.hxx
index 0eb3ef33101c..862cbb1a0766 100644
--- a/include/svx/unoshape.hxx
+++ b/include/svx/unoshape.hxx
@@ -428,7 +428,8 @@ class SVXCORE_DLLPUBLIC SvxShapeGroup final : public
SvxShapeGroupAnyD,
public css::drawing::XShapes2
{
private:
- rtl::Reference< SvxDrawPage> mxPage;
+ /// using a weak reference to prevent leaks via ref-counting cycles
+ unotools::WeakReference< SvxDrawPage> mxWeakPage;
void addUnoShape( const css::uno::Reference< css::drawing::XShape >&
xShape, size_t nPos );
void addShape(SvxShape& rShape, size_t nPos);
diff --git a/svx/source/unodraw/unoshap2.cxx b/svx/source/unodraw/unoshap2.cxx
index cf72530e6c7d..0315d2a5c74e 100644
--- a/svx/source/unodraw/unoshap2.cxx
+++ b/svx/source/unodraw/unoshap2.cxx
@@ -80,7 +80,7 @@ using namespace ::com::sun::star::container;
SvxShapeGroup::SvxShapeGroup(SdrObject* pObj, SvxDrawPage* pDrawPage)
: SvxShapeGroupAnyD(pObj, getSvxMapProvider().GetMap(SVXMAP_GROUP),
getSvxMapProvider().GetPropertySet(SVXMAP_GROUP,
SdrObject::GetGlobalDrawObjectItemPool()))
- , mxPage(pDrawPage)
+ , mxWeakPage(pDrawPage)
{
}
@@ -91,7 +91,7 @@ SvxShapeGroup::~SvxShapeGroup() noexcept
void SvxShapeGroup::Create( SdrObject* pNewObj, SvxDrawPage* pNewPage )
{
SvxShape::Create( pNewObj, pNewPage );
- mxPage = pNewPage;
+ mxWeakPage = pNewPage;
}
@@ -185,7 +185,13 @@ void SvxShapeGroup::addShape( SvxShape& rShape )
void SvxShapeGroup::addShape( SvxShape& rShape, size_t nPos )
{
- if (!HasSdrObject() || !mxPage.is())
+ SdrObject* pSdrObject = GetSdrObject();
+ if (!pSdrObject)
+ {
+ return;
+ }
+ rtl::Reference<SvxDrawPage> xPage = mxWeakPage.get();
+ if (!xPage)
{
OSL_FAIL("could not add XShape to group shape!");
return;
@@ -193,12 +199,12 @@ void SvxShapeGroup::addShape( SvxShape& rShape, size_t
nPos )
rtl::Reference<SdrObject> pSdrShape = rShape.GetSdrObject();
if( pSdrShape == nullptr )
- pSdrShape = mxPage->CreateSdrObject_( &rShape );
+ pSdrShape = xPage->CreateSdrObject_( &rShape );
if( pSdrShape->IsInserted() )
pSdrShape->getParentSdrObjListFromSdrObject()->RemoveObject(
pSdrShape->GetOrdNum() );
- GetSdrObject()->GetSubList()->InsertObject(pSdrShape.get(), nPos);
+ pSdrObject->GetSubList()->InsertObject(pSdrShape.get(), nPos);
// TTTT Was created using mpModel in CreateSdrObject_ above
// TTTT may be good to add an assertion here for the future
// pSdrShape->SetModel(GetSdrObject()->GetModel());
@@ -214,9 +220,9 @@ void SvxShapeGroup::addShape( SvxShape& rShape, size_t nPos
)
// Establish connection between new SdrObject and its wrapper before
// inserting the new shape into the group. There a new wrapper
// would be created when this connection would not already exist.
- rShape.Create( pSdrShape.get(), mxPage.get() );
+ rShape.Create( pSdrShape.get(), xPage.get() );
- GetSdrObject()->getSdrModelFromSdrObject().SetChanged();
+ pSdrObject->getSdrModelFromSdrObject().SetChanged();
}
// XShapes
diff --git a/sw/inc/unodraw.hxx b/sw/inc/unodraw.hxx
index e252499f0453..d7a768317bbf 100644
--- a/sw/inc/unodraw.hxx
+++ b/sw/inc/unodraw.hxx
@@ -30,7 +30,6 @@
#include <com/sun/star/beans/XPropertySet.hpp>
#include <com/sun/star/beans/XPropertyState.hpp>
#include <com/sun/star/drawing/XShapes.hpp>
-#include <cppuhelper/implbase4.hxx>
#include <cppuhelper/implbase6.hxx>
#include <com/sun/star/container/XEnumerationAccess.hpp>
#include <com/sun/star/drawing/HomogenMatrix3.hpp>
@@ -40,12 +39,18 @@ class SdrView;
class SwDoc;
class SwXShape;
-class SwFmDrawPage final : public SvxFmDrawPage
+typedef cppu::ImplInheritanceHelper
+<
+ SvxFmDrawPage,
+ css::container::XEnumerationAccess
+> SwFmDrawPage_Base;
+class SwFmDrawPage final : public SwFmDrawPage_Base
{
+ SwDoc* m_pDoc;
SdrPageView* m_pPageView;
std::vector<rtl::Reference<SwXShape>> m_vShapes;
public:
- SwFmDrawPage( SdrPage* pPage );
+ SwFmDrawPage( SwDoc* pDoc, SdrPage* pPage );
virtual ~SwFmDrawPage() noexcept override;
const SdrMarkList& PreGroup(const css::uno::Reference<
css::drawing::XShapes >& rShapes);
@@ -67,23 +72,6 @@ public:
if(ppShape != m_vShapes.end())
m_vShapes.erase(ppShape);
};
-};
-
-typedef cppu::WeakAggImplHelper4
-<
- css::container::XEnumerationAccess,
- css::drawing::XDrawPage,
- css::lang::XServiceInfo,
- css::drawing::XShapeGrouper
->
-SwXDrawPageBaseClass;
-class SwXDrawPage final : public SwXDrawPageBaseClass
-{
- SwDoc* m_pDoc;
- rtl::Reference<SwFmDrawPage> m_pDrawPage;
-public:
- SwXDrawPage(SwDoc* pDoc);
- virtual ~SwXDrawPage() override;
//XEnumerationAccess
virtual css::uno::Reference< css::container::XEnumeration > SAL_CALL
createEnumeration() override;
@@ -112,7 +100,6 @@ public:
virtual sal_Bool SAL_CALL supportsService(const OUString& ServiceName)
override;
virtual css::uno::Sequence< OUString > SAL_CALL getSupportedServiceNames()
override;
- SwFmDrawPage* GetSvxPage();
// renamed and outlined to detect where it's called
void InvalidateSwDoc(); // {pDoc = 0;}
};
@@ -132,7 +119,6 @@ SwXShapeBaseClass;
class SwXShape : public SwXShapeBaseClass, public SvtListener
{
friend class SwXGroupShape;
- friend class SwXDrawPage;
friend class SwFmDrawPage;
const SwFmDrawPage* m_pPage;
SwFrameFormat* m_pFormat;
diff --git a/sw/inc/unotxdoc.hxx b/sw/inc/unotxdoc.hxx
index 16f83ae149bc..a69a678a6cf1 100644
--- a/sw/inc/unotxdoc.hxx
+++ b/sw/inc/unotxdoc.hxx
@@ -73,7 +73,7 @@ class SwDoc;
class SwDocShell;
class UnoActionContext;
class SwXBodyText;
-class SwXDrawPage;
+class SwFmDrawPage;
class SwUnoCursor;
class SwXDocumentPropertyHelper;
class SfxViewFrame;
@@ -146,7 +146,7 @@ private:
SwDocShell* m_pDocShell;
bool m_bObjectValid;
- rtl::Reference<SwXDrawPage> m_xDrawPage;
+ rtl::Reference<SwFmDrawPage> m_xDrawPage;
rtl::Reference<SwXBodyText> m_xBodyText;
css::uno::Reference< css::uno::XAggregation >
m_xNumFormatAgg;
diff --git a/sw/source/core/unocore/unodraw.cxx
b/sw/source/core/unocore/unodraw.cxx
index 8b001ded5f0d..556be300bcff 100644
--- a/sw/source/core/unocore/unodraw.cxx
+++ b/sw/source/core/unocore/unodraw.cxx
@@ -252,8 +252,8 @@ public:
}
};
-SwFmDrawPage::SwFmDrawPage( SdrPage* pPage ) :
- SvxFmDrawPage( pPage ), m_pPageView(nullptr)
+SwFmDrawPage::SwFmDrawPage( SwDoc* pDoc, SdrPage* pPage ) :
+ SwFmDrawPage_Base( pPage ), m_pDoc(pDoc), m_pPageView(nullptr)
{
}
@@ -389,7 +389,7 @@ namespace
protected:
virtual ~SwXShapesEnumeration() override {};
public:
- explicit SwXShapesEnumeration(SwXDrawPage* const pDrawPage);
+ explicit SwXShapesEnumeration(SwFmDrawPage* const pDrawPage);
//XEnumeration
virtual sal_Bool SAL_CALL hasMoreElements() override;
@@ -402,7 +402,7 @@ namespace
};
}
-SwXShapesEnumeration::SwXShapesEnumeration(SwXDrawPage* const pDrawPage)
+SwXShapesEnumeration::SwXShapesEnumeration(SwFmDrawPage* const pDrawPage)
{
SolarMutexGuard aGuard;
sal_Int32 nCount = pDrawPage->getCount();
@@ -445,70 +445,45 @@ uno::Sequence< OUString >
SwXShapesEnumeration::getSupportedServiceNames()
return { OUString("com.sun.star.container.XEnumeration") };
}
-uno::Reference< container::XEnumeration > SwXDrawPage::createEnumeration()
+uno::Reference< container::XEnumeration > SwFmDrawPage::createEnumeration()
{
SolarMutexGuard aGuard;
return uno::Reference< container::XEnumeration >(
new SwXShapesEnumeration(this));
}
-OUString SwXDrawPage::getImplementationName()
+OUString SwFmDrawPage::getImplementationName()
{
- return "SwXDrawPage";
+ return "SwFmDrawPage";
}
-sal_Bool SwXDrawPage::supportsService(const OUString& rServiceName)
+sal_Bool SwFmDrawPage::supportsService(const OUString& rServiceName)
{
return cppu::supportsService(this, rServiceName);
}
-uno::Sequence< OUString > SwXDrawPage::getSupportedServiceNames()
+uno::Sequence< OUString > SwFmDrawPage::getSupportedServiceNames()
{
return { "com.sun.star.drawing.GenericDrawPage" };
}
-SwXDrawPage::SwXDrawPage(SwDoc* pDc) :
- m_pDoc(pDc)
+uno::Any SwFmDrawPage::queryInterface( const uno::Type& aType )
{
-}
-
-SwXDrawPage::~SwXDrawPage()
-{
- if(m_pDrawPage.is())
- {
- uno::Reference< uno::XInterface > xInt;
- m_pDrawPage->setDelegator(xInt);
- }
-}
-
-uno::Any SwXDrawPage::queryInterface( const uno::Type& aType )
-{
- uno::Any aRet = SwXDrawPageBaseClass::queryInterface(aType);
+ uno::Any aRet = SvxFmDrawPage::queryInterface(aType);
if(!aRet.hasValue())
- {
- // secure with checking if page exists. This may not be the case
- // either for new SW docs with no yet graphics usage or when
- // the doc is closed and someone else still holds a UNO reference
- // to the XDrawPage (in that case, pDoc is set to 0)
- SwFmDrawPage* pPage = GetSvxPage();
-
- if(pPage)
- {
- aRet = pPage->queryAggregation(aType);
- }
- }
+ aRet = SwFmDrawPage_Base::queryInterface(aType);
return aRet;
}
-uno::Sequence< uno::Type > SwXDrawPage::getTypes()
+uno::Sequence< uno::Type > SwFmDrawPage::getTypes()
{
return comphelper::concatSequences(
- SwXDrawPageBaseClass::getTypes(),
- GetSvxPage()->getTypes(),
+ SvxFmDrawPage::getTypes(),
+ SwFmDrawPage_Base::getTypes(),
uno::Sequence { cppu::UnoType<form::XFormsSupplier2>::get() });
}
-sal_Int32 SwXDrawPage::getCount()
+sal_Int32 SwFmDrawPage::getCount()
{
SolarMutexGuard aGuard;
if(!m_pDoc)
@@ -516,13 +491,10 @@ sal_Int32 SwXDrawPage::getCount()
if(!m_pDoc->getIDocumentDrawModelAccess().GetDrawModel())
return 0;
else
- {
- GetSvxPage();
- return SwTextBoxHelper::getCount(m_pDrawPage->GetSdrPage());
- }
+ return SwTextBoxHelper::getCount(GetSdrPage());
}
-uno::Any SwXDrawPage::getByIndex(sal_Int32 nIndex)
+uno::Any SwFmDrawPage::getByIndex(sal_Int32 nIndex)
{
SolarMutexGuard aGuard;
if(!m_pDoc)
@@ -530,27 +502,25 @@ uno::Any SwXDrawPage::getByIndex(sal_Int32 nIndex)
if(!m_pDoc->getIDocumentDrawModelAccess().GetDrawModel())
throw lang::IndexOutOfBoundsException();
- GetSvxPage();
- return SwTextBoxHelper::getByIndex(m_pDrawPage->GetSdrPage(), nIndex);
+ return SwTextBoxHelper::getByIndex(GetSdrPage(), nIndex);
}
-uno::Type SwXDrawPage::getElementType()
+uno::Type SwFmDrawPage::getElementType()
{
return cppu::UnoType<drawing::XShape>::get();
}
-sal_Bool SwXDrawPage::hasElements()
+sal_Bool SwFmDrawPage::hasElements()
{
SolarMutexGuard aGuard;
if(!m_pDoc)
throw uno::RuntimeException();
if(!m_pDoc->getIDocumentDrawModelAccess().GetDrawModel())
return false;
- else
- return GetSvxPage()->hasElements();
+ return SvxFmDrawPage::hasElements();
}
-void SwXDrawPage::add(const uno::Reference< drawing::XShape > & xShape)
+void SwFmDrawPage::add(const uno::Reference< drawing::XShape > & xShape)
{
SolarMutexGuard aGuard;
if(!m_pDoc)
@@ -576,7 +546,7 @@ void SwXDrawPage::add(const uno::Reference< drawing::XShape
> & xShape)
return;
}
}
- GetSvxPage()->add(xShape);
+ SvxFmDrawPage::add(xShape);
OSL_ENSURE(pSvxShape, "Why is here no SvxShape?");
// this position is definitely in 1/100 mm
@@ -721,7 +691,7 @@ void SwXDrawPage::add(const uno::Reference< drawing::XShape
> & xShape)
pInternalPam.reset();
}
-void SwXDrawPage::remove(const uno::Reference< drawing::XShape > & xShape)
+void SwFmDrawPage::remove(const uno::Reference< drawing::XShape > & xShape)
{
SolarMutexGuard aGuard;
if(!m_pDoc)
@@ -739,101 +709,67 @@ void SwXDrawPage::remove(const uno::Reference<
drawing::XShape > & xShape)
xComp->dispose();
}
-uno::Reference< drawing::XShapeGroup > SwXDrawPage::group(const
uno::Reference< drawing::XShapes > & xShapes)
+uno::Reference< drawing::XShapeGroup > SwFmDrawPage::group(const
uno::Reference< drawing::XShapes > & xShapes)
{
SolarMutexGuard aGuard;
if(!m_pDoc || !xShapes.is())
throw uno::RuntimeException();
uno::Reference< drawing::XShapeGroup > xRet;
- if(m_pDrawPage.is())
+ // mark and return MarkList
+ const SdrMarkList& rMarkList = PreGroup(xShapes);
+ if ( rMarkList.GetMarkCount() > 0 )
{
-
- SwFmDrawPage* pPage = GetSvxPage();
- if(pPage) //TODO: can this be Null?
+ for (size_t i = 0; i < rMarkList.GetMarkCount(); ++i)
{
- // mark and return MarkList
- const SdrMarkList& rMarkList = pPage->PreGroup(xShapes);
- if ( rMarkList.GetMarkCount() > 0 )
+ const SdrObject *pObj = rMarkList.GetMark( i )->GetMarkedSdrObj();
+ if (RndStdIds::FLY_AS_CHAR ==
::FindFrameFormat(const_cast<SdrObject*>(
+ pObj))->GetAnchor().GetAnchorId())
{
- for (size_t i = 0; i < rMarkList.GetMarkCount(); ++i)
- {
- const SdrObject *pObj = rMarkList.GetMark( i
)->GetMarkedSdrObj();
- if (RndStdIds::FLY_AS_CHAR ==
::FindFrameFormat(const_cast<SdrObject*>(
- pObj))->GetAnchor().GetAnchorId())
- {
- throw lang::IllegalArgumentException(
- "Shape must not have 'as character' anchor!",
nullptr, 0);
- }
- }
+ throw lang::IllegalArgumentException(
+ "Shape must not have 'as character' anchor!", nullptr, 0);
+ }
+ }
- UnoActionContext aContext(m_pDoc);
- m_pDoc->GetIDocumentUndoRedo().StartUndo( SwUndoId::START,
nullptr );
+ UnoActionContext aContext(m_pDoc);
+ m_pDoc->GetIDocumentUndoRedo().StartUndo( SwUndoId::START, nullptr );
- SwDrawContact* pContact = m_pDoc->GroupSelection(
*pPage->GetDrawView() );
- m_pDoc->ChgAnchor(
- pPage->GetDrawView()->GetMarkedObjectList(),
- RndStdIds::FLY_AT_PARA,
- true, false );
+ SwDrawContact* pContact = m_pDoc->GroupSelection( *GetDrawView() );
+ m_pDoc->ChgAnchor(
+ GetDrawView()->GetMarkedObjectList(),
+ RndStdIds::FLY_AT_PARA,
+ true, false );
- pPage->GetDrawView()->UnmarkAll();
- if(pContact)
- xRet = SwFmDrawPage::GetShapeGroup( pContact->GetMaster()
);
- m_pDoc->GetIDocumentUndoRedo().EndUndo( SwUndoId::END, nullptr
);
- }
- pPage->RemovePageView();
- }
+ GetDrawView()->UnmarkAll();
+ if(pContact)
+ xRet = SwFmDrawPage::GetShapeGroup( pContact->GetMaster() );
+ m_pDoc->GetIDocumentUndoRedo().EndUndo( SwUndoId::END, nullptr );
}
+ RemovePageView();
return xRet;
}
-void SwXDrawPage::ungroup(const uno::Reference< drawing::XShapeGroup > &
rShapeGroup)
+void SwFmDrawPage::ungroup(const uno::Reference< drawing::XShapeGroup > &
rShapeGroup)
{
SolarMutexGuard aGuard;
if(!m_pDoc)
throw uno::RuntimeException();
- if(!m_pDrawPage.is())
- return;
-
- SwFmDrawPage* pPage = GetSvxPage();
- if(!pPage) //TODO: can this be Null?
- return;
- pPage->PreUnGroup(rShapeGroup);
+ PreUnGroup(rShapeGroup);
UnoActionContext aContext(m_pDoc);
m_pDoc->GetIDocumentUndoRedo().StartUndo( SwUndoId::START, nullptr );
- m_pDoc->UnGroupSelection( *pPage->GetDrawView() );
- m_pDoc->ChgAnchor( pPage->GetDrawView()->GetMarkedObjectList(),
+ m_pDoc->UnGroupSelection( *GetDrawView() );
+ m_pDoc->ChgAnchor( GetDrawView()->GetMarkedObjectList(),
RndStdIds::FLY_AT_PARA,
true, false );
m_pDoc->GetIDocumentUndoRedo().EndUndo( SwUndoId::END, nullptr );
- pPage->RemovePageView();
-}
-
-SwFmDrawPage* SwXDrawPage::GetSvxPage()
-{
- if(!m_pDrawPage.is() && m_pDoc)
- {
- SolarMutexGuard aGuard;
- // #i52858#
- SwDrawModel* pModel =
m_pDoc->getIDocumentDrawModelAccess().GetOrCreateDrawModel();
- SdrPage* pPage = pModel->GetPage( 0 );
-
- {
- // We need a Ref to the object during queryInterface or else
- // it will be deleted
- m_pDrawPage = new SwFmDrawPage(pPage);
- }
- if( m_pDrawPage.is() )
- m_pDrawPage->setDelegator( static_cast<cppu::OWeakObject*>(this) );
- }
- return m_pDrawPage.get();
+ RemovePageView();
}
/**
* Renamed and outlined to detect where it's called
*/
-void SwXDrawPage::InvalidateSwDoc()
+void SwFmDrawPage::InvalidateSwDoc()
{
m_pDoc = nullptr;
}
diff --git a/sw/source/uibase/uno/unotxdoc.cxx
b/sw/source/uibase/uno/unotxdoc.cxx
index 4c67a45cdcf8..502e1997c90e 100644
--- a/sw/source/uibase/uno/unotxdoc.cxx
+++ b/sw/source/uibase/uno/unotxdoc.cxx
@@ -1280,12 +1280,11 @@ Reference< drawing::XDrawPage >
SwXTextDocument::getDrawPage()
throw DisposedException("", static_cast< XTextDocument* >(this));
if(!m_xDrawPage.is())
{
- m_xDrawPage = new SwXDrawPage(m_pDocShell->GetDoc());
- // Create a Reference to trigger the complete initialization of the
- // object. Otherwise in some corner cases it would get initialized
- // at ::InitNewDoc -> which would get called during
- // close() or dispose() -> n#681746
- uno::Reference<lang::XComponent> xTriggerInit(
static_cast<cppu::OWeakObject*>(m_xDrawPage.get()), uno::UNO_QUERY );
+ SwDoc* pDoc = m_pDocShell->GetDoc();
+ // #i52858#
+ SwDrawModel* pModel =
pDoc->getIDocumentDrawModelAccess().GetOrCreateDrawModel();
+ SdrPage* pPage = pModel->GetPage( 0 );
+ m_xDrawPage = new SwFmDrawPage(pDoc, pPage);
}
return m_xDrawPage;
}
