[Libreoffice-commits] core.git: Branch 'libreoffice-5-3' - sw/source

[Michael Stahl]

 sw/source/core/layout/newfrm.cxx |   24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

New commits:
commit 6d4a041fe81b36e1e8f933bfe4216afcea72c76d
Author: Michael Stahl <mstahl at redhat.com>
Date:   Thu Jun 22 10:39:36 2017 +0200

    tdf#101821 sw: fix layout footnote use-after-free in SwRootFrame
    
    The ClearSwLayouterEntries() accesses anchored objects and if they are
    anchored in footnotes then RemoveFootnotes() has already deleted them.
    
    (regression from 962d0500c4debaef43e5f146e47e08c66d851562)
    
    Invalid write of size 1
       at 0x4143CCB3: SwAnchoredObject::SetTmpConsiderWrapInfluence(bool) (anchoredobject.cxx:739)
       by 0x414D8A21: SwObjsMarkedAsTmpConsiderWrapInfluence::Clear() (objstmpconsiderwrapinfl.cxx:58)
       by 0x414C943E: SwLayouter::ClearObjsTmpConsiderWrapInfluence(SwDoc const&) (layouter.cxx:401)
       by 0x411DBE57: sw::DocumentLayoutManager::ClearSwLayouterEntries() (DocumentLayoutManager.cxx:504)
       by 0x414D05D9: SwRootFrame::DestroyImpl() (newfrm.cxx:594)
       by 0x41535AB3: SwFrame::DestroyFrame(SwFrame*) (ssfrm.cxx:389)
       by 0x419E8171: std::_Sp_counted_deleter<SwRootFrame*, void (*)(SwFrame*), std::allocator<void>, (__gnu_cxx::_Lock_policy)2>::_M_dispose() (shared_ptr_base.h:464)
       by 0x40EB6DB5: std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (shared_ptr_base.h:150)
       by 0x40EB5E76: std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() (shared_ptr_base.h:662)
       by 0x419E65F9: std::__shared_ptr<SwRootFrame, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() (shared_ptr_base.h:928)
       by 0x419E6615: std::shared_ptr<SwRootFrame>::~shared_ptr() (shared_ptr.h:93)
       by 0x419E619D: SwViewShell::~SwViewShell() (vnew.cxx:285)
      Address 0x5feb6eee is 334 bytes inside a block of size 488 free'd
       at 0x4C2F21A: operator delete(void*) (vg_replace_malloc.c:576)
       by 0x41488962: SwFlyAtContentFrame::~SwFlyAtContentFrame() (flyfrms.hxx:134)
       by 0x41535AFC: SwFrame::DestroyFrame(SwFrame*) (ssfrm.cxx:391)
       by 0x415360BD: SwLayoutFrame::DestroyImpl() (ssfrm.cxx:477)
       by 0x41535AB3: SwFrame::DestroyFrame(SwFrame*) (ssfrm.cxx:389)
       by 0x414A2FF4: sw_RemoveFootnotes(SwFootnoteBossFrame*, bool, bool) (ftnfrm.cxx:852)
       by 0x414A3104: sw_RemoveFootnotes(SwFootnoteBossFrame*, bool, bool) (ftnfrm.cxx:874)
       by 0x414A321A: SwRootFrame::RemoveFootnotes(SwPageFrame*, bool, bool) (ftnfrm.cxx:897)
       by 0x414D0558: SwRootFrame::DestroyImpl() (newfrm.cxx:585)
       by 0x41535AB3: SwFrame::DestroyFrame(SwFrame*) (ssfrm.cxx:389)
       by 0x419E8171: std::_Sp_counted_deleter<SwRootFrame*, void (*)(SwFrame*), std::allocator<void>, (__gnu_cxx::_Lock_policy)2>::_M_dispose() (shared_ptr_base.h:464)
       by 0x40EB6DB5: std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (shared_ptr_base.h:150)
       by 0x40EB5E76: std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() (shared_ptr_base.h:662)
       by 0x419E65F9: std::__shared_ptr<SwRootFrame, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() (shared_ptr_base.h:928)
       by 0x419E6615: std::shared_ptr<SwRootFrame>::~shared_ptr() (shared_ptr.h:93)
       by 0x419E619D: SwViewShell::~SwViewShell() (vnew.cxx:285)
    
    Change-Id: I147f46d49c90de46189ad34feed66c289adddc15
    (cherry picked from commit c7782c7c27d85866872cc24a618df02504ff12ca)
    Reviewed-on: https://gerrit.libreoffice.org/39106
    Tested-by: Jenkins <ci at libreoffice.org>
    Reviewed-by: Björn Michaelsen <bjoern.michaelsen at libreoffice.org>

diff --git a/sw/source/core/layout/newfrm.cxx b/sw/source/core/layout/newfrm.cxx
index e1a96cd25ac1..dae63d1f8aa6 100644
--- a/sw/source/core/layout/newfrm.cxx
+++ b/sw/source/core/layout/newfrm.cxx
@@ -573,16 +573,6 @@ void SwRootFrame::DestroyImpl()
 {
     mbTurboAllowed = false;
     mpTurbo = nullptr;
-    // fdo#39510 crash on document close with footnotes
-    // Object ownership in writer and esp. in layout are a mess: Before the
-    // document/layout split SwDoc and SwRootFrame were essentially one object
-    // and magically/uncleanly worked around their common destruction by call
-    // to SwDoc::IsInDtor() -- even from the layout. As of now destruction of
-    // the layout proceeds forward through the frames. Since SwTextFootnote::DelFrames
-    // also searches backwards to find the master of footnotes, they must be
-    // considered to be owned by the SwRootFrame and also be destroyed here,
-    // before tearing down the (now footnote free) rest of the layout.
-    RemoveFootnotes(nullptr, false, true);
 
     if(pBlink)
         pBlink->FrameDelete( this );
@@ -591,8 +581,11 @@ void SwRootFrame::DestroyImpl()
     {
         SwDoc *pDoc = pRegisteredInNonConst->GetDoc();
         pDoc->DelFrameFormat( pRegisteredInNonConst );
+        // do this before calling RemoveFootnotes() because footnotes
+        // can contain anchored objects
         pDoc->GetDocumentLayoutManager().ClearSwLayouterEntries();
     }
+
     delete mpDestroy;
     mpDestroy = nullptr;
 
@@ -606,6 +599,17 @@ void SwRootFrame::DestroyImpl()
     // Some accessible shells are left => problems on second SwFrame::Destroy call
     assert(0 == mnAccessibleShells);
 
+    // fdo#39510 crash on document close with footnotes
+    // Object ownership in writer and esp. in layout are a mess: Before the
+    // document/layout split SwDoc and SwRootFrame were essentially one object
+    // and magically/uncleanly worked around their common destruction by call
+    // to SwDoc::IsInDtor() -- even from the layout. As of now destruction of
+    // the layout proceeds forward through the frames. Since SwTextFootnote::DelFrames
+    // also searches backwards to find the master of footnotes, they must be
+    // considered to be owned by the SwRootFrame and also be destroyed here,
+    // before tearing down the (now footnote free) rest of the layout.
+    RemoveFootnotes(nullptr, false, true);
+
     SwLayoutFrame::DestroyImpl();
 }