Fix use-after-free in SwLayAction::IsShortCut

...as seen with valgrind during CppunitTest_sw_odfexport (see below).

It looks like the

if ( !IsEmptyPage() ) //#59184# unnecessary for empty pages

optimization in SwPageFrame::DestroyImpl (which was there ever since at least
84a3db80b4fd66c6854b3135b5f69b61fd828e62 "initial import") was wrong, preventing
the call to

pImp->GetLayAction().SetAgain();

that would cause SwLayAction::IsShortCut (sw/source/core/layout/layact.cxx) to
quit early from an IsAgain() call, before accessing prPage again that has
meanwhile been destroyed from within its

pContent->Calc(pRenderContext);

call.

The same failure started to show in ASan+UBSan builds only now after
integration of <https://gerrit.libreoffice.org/#/c/58263/> "the custom SAL
allocator is no longer used", for reasons I explained in a comment there:  "For
FORCE_SYSALLOC (which was esp. set for ASan/UBSan builds), alloc_mode was always
left at AllocMode::UNSET (as determine_alloc_mode was never called in
FORCE_SYSALLOC blocks), so rtl_cache_alloc (which only checked alloc_mode for
AllocMode::SYSTEM, but never called determine_alloc, and wasn't entirely short-
circuited under FORCE_SYSALLOC) actually called into the legacy, supposed-dead
slab allocator code.  That's apparently how some use-after-free bug in sw got
hidden from ASan+UBSan builds in the past, and only now starts to show up
(<https://ci.libreoffice.org/job/lo_ubsan/989/>)."

The valgrind failure log:
[...]
> testFdo58949::Import_Export_Import finished in: 191975ms
> File tested,Execution Time (ms)
> warn:sw:12555:12555:sw/inc/swrect.hxx:283: SVRect() without Width or Height
> warn:sw:12555:12555:sw/inc/swrect.hxx:283: SVRect() without Width or Height
> warn:sw:12555:12555:sw/inc/swrect.hxx:283: SVRect() without Width or Height
> warn:sw:12555:12555:sw/inc/swrect.hxx:283: SVRect() without Width or Height
> warn:sw:12555:12555:sw/inc/swrect.hxx:283: SVRect() without Width or Height
> warn:sw:12555:12555:sw/inc/swrect.hxx:283: SVRect() without Width or Height
> warn:sw:12555:12555:sw/inc/swrect.hxx:283: SVRect() without Width or Height
> warn:sw:12555:12555:sw/inc/swrect.hxx:283: SVRect() without Width or Height
> warn:sw:12555:12555:sw/inc/swrect.hxx:283: SVRect() without Width or Height
> warn:sw:12555:12555:sw/inc/swrect.hxx:283: SVRect() without Width or Height
> ==12555== Invalid read of size 8
> ==12555==    at 0x2AFDAA7C: SwFrame::GetPrev() (/sw/source/core/inc/frame.hxx:649)
> ==12555==    by 0x2B6A4F7A: SwLayAction::IsShortCut(SwPageFrame*&) (/sw/source/core/layout/layact.cxx:1071)
> ==12555==    by 0x2B6A342D: SwLayAction::InternalAction(OutputDevice*) (/sw/source/core/layout/layact.cxx:473)
> ==12555==    by 0x2B6A2E46: SwLayAction::Action(OutputDevice*) (/sw/source/core/layout/layact.cxx:340)
> ==12555==    by 0x2BE0F8A2: SwViewShell::ImplEndAction(bool) (/sw/source/core/view/viewsh.cxx:281)
> ==12555==    by 0x2AF7A6C0: SwViewShell::EndAction(bool) (/sw/inc/viewsh.hxx:595)
> ==12555==    by 0x2AF68B50: SwCursorShell::EndAction(bool, bool) (/sw/source/core/crsr/crsrsh.cxx:258)
> ==12555==    by 0x2C464281: SwView::OuterResizePixel(Point const&, Size const&) (/sw/source/uibase/uiview/viewport.cxx:1124)
> ==12555==    by 0x2A29D78F: SfxViewFrame::DoAdjustPosSizePixel(SfxViewShell*, Point const&, Size const&, bool) (/sfx2/source/view/viewfrm.cxx:1604)
> ==12555==    by 0x2A2A390E: SfxViewFrame::Resize(bool) (/sfx2/source/view/viewfrm.cxx:2395)
> ==12555==    by 0x2A2AF1A7: SfxFrameViewWindow_Impl::Resize() (/sfx2/source/view/viewfrm2.cxx:73)
> ==12555==    by 0x1A8C2A64: vcl::Window::ImplCallResize() (/vcl/source/window/event.cxx:523)
> ==12555==    by 0x1AA2AF8D: vcl::Window::Show(bool, ShowFlags) (/vcl/source/window/window.cxx:2277)
> ==12555==    by 0x2A282069: SfxBaseController::ConnectSfxFrame_Impl(SfxBaseController::ConnectSfxFrame) (/sfx2/source/view/sfxbasecontroller.cxx:1250)
> ==12555==    by 0x2A2815BA: SfxBaseController::attachFrame(com::sun::star::uno::Reference<com::sun::star::frame::XFrame> const&) (/sfx2/source/view/sfxbasecontroller.cxx:550)
> ==12555==    by 0x2A265C78: (anonymous namespace)::SfxFrameLoader_Impl::impl_createDocumentView(com::sun::star::uno::Reference<com::sun::star::frame::XModel2> const&, com::sun::star::uno::Reference<com::sun::star::frame::XFrame> const&, comphelper::NamedValueCollection const&, rtl::OUString const&) (/sfx2/source/view/frmload.cxx:594)
> ==12555==    by 0x2A26374F: (anonymous namespace)::SfxFrameLoader_Impl::load(com::sun::star::uno::Sequence<com::sun::star::beans::PropertyValue> const&, com::sun::star::uno::Reference<com::sun::star::frame::XFrame> const&) (/sfx2/source/view/frmload.cxx:711)
> ==12555==    by 0x36E8A2A1: framework::LoadEnv::impl_loadContent() (/framework/source/loadenv/loadenv.cxx:1149)
> ==12555==    by 0x36E84F2A: framework::LoadEnv::startLoading() (/framework/source/loadenv/loadenv.cxx:383)
> ==12555==    by 0x36E83979: framework::LoadEnv::loadComponentFromURL(com::sun::star::uno::Reference<com::sun::star::frame::XComponentLoader> const&, com::sun::star::uno::Reference<com::sun::star::uno::XComponentContext> const&, rtl::OUString const&, rtl::OUString const&, int, com::sun::star::uno::Sequence<com::sun::star::beans::PropertyValue> const&) (/framework/source/loadenv/loadenv.cxx:169)
> ==12555==    by 0x36EDDEB8: framework::Desktop::loadComponentFromURL(rtl::OUString const&, rtl::OUString const&, int, com::sun::star::uno::Sequence<com::sun::star::beans::PropertyValue> const&) (/framework/source/services/desktop.cxx:619)
> ==12555==    by 0x36EDDF6A: non-virtual thunk to framework::Desktop::loadComponentFromURL(rtl::OUString const&, rtl::OUString const&, int, com::sun::star::uno::Sequence<com::sun::star::beans::PropertyValue> const&) (/framework/source/services/desktop.cxx:0)
> ==12555==    by 0x2D456C63: unotest::MacrosTest::loadFromDesktop(rtl::OUString const&, rtl::OUString const&, com::sun::star::uno::Sequence<com::sun::star::beans::PropertyValue> const&) (/unotest/source/cpp/macros_test.cxx:50)
> ==12555==    by 0x295BCC30: SwModelTestBase::loadURL(rtl::OUString const&, char const*, char const*) (/sw/qa/extras/inc/swmodeltestbase.hxx:762)
> ==12555==    by 0x295BC7E9: SwModelTestBase::load(rtl::OUString const&, char const*, char const*) (/sw/qa/extras/inc/swmodeltestbase.hxx:717)
> ==12555==    by 0x295BC660: SwModelTestBase::executeImportTest(char const*, char const*) (/sw/qa/extras/inc/swmodeltestbase.hxx:264)
> ==12555==    by 0x295E4B5F: testStylePageNumber::Import() (/sw/qa/extras/odfexport/odfexport.cxx:686)
[...]
> ==12555==  Address 0x28ddde10 is 160 bytes inside a block of size 280 free'd
> ==12555==    at 0x4C2FDAC: free (/builddir/build/BUILD/valgrind-3.13.0/coregrind/m_replacemalloc/vg_replace_malloc.c:530)
> ==12555==    by 0x519E494: rtl_freeMemory (/sal/rtl/alloc_global.cxx:51)
> ==12555==    by 0x519DCDB: rtl_cache_free (/sal/rtl/alloc_cache.cxx:172)
> ==12555==    by 0x19BB090A: FixedMemPool::Free(void*) (/tools/source/memtools/mempool.cxx:49)
> ==12555==    by 0x2B68B89D: SwPageFrame::operator delete(void*, unsigned long) (/sw/source/core/inc/pagefrm.hxx:111)
> ==12555==    by 0x2B6DA2E8: SwPageFrame::~SwPageFrame() (/sw/source/core/layout/pagechg.cxx:301)
> ==12555==    by 0x2B74E763: SwFrame::DestroyFrame(SwFrame*) (/sw/source/core/layout/ssfrm.cxx:384)
> ==12555==    by 0x2B6E18D3: SwRootFrame::RemovePage(SwPageFrame**, SwRemoveResult) (/sw/source/core/layout/pagechg.cxx:1414)
> ==12555==    by 0x2B6E145D: (anonymous namespace)::doInsertPage(SwRootFrame*, SwPageFrame**, SwFrameFormat*, SwPageDesc*, bool, SwPageFrame**) (/sw/source/core/layout/pagechg.cxx:1270)
> ==12555==    by 0x2B6E0CA0: SwFrame::InsertPage(SwPageFrame*, bool) (/sw/source/core/layout/pagechg.cxx:1324)
> ==12555==    by 0x2B65373D: SwFrame::GetNextLeaf(MakePageType) (/sw/source/core/layout/flowfrm.cxx:997)
> ==12555==    by 0x2B65331A: SwFrame::GetLeaf(MakePageType, bool) (/sw/source/core/layout/flowfrm.cxx:818)
> ==12555==    by 0x2B657637: SwFlowFrame::MoveFwd(bool, bool, bool) (/sw/source/core/layout/flowfrm.cxx:1876)
> ==12555==    by 0x2B657133: SwFlowFrame::CheckMoveFwd(bool&, bool, bool) (/sw/source/core/layout/flowfrm.cxx:1796)
> ==12555==    by 0x2B634109: SwContentFrame::MakeAll(OutputDevice*) (/sw/source/core/layout/calcmove.cxx:1322)
> ==12555==    by 0x2B62CEF4: SwFrame::PrepareMake(OutputDevice*) (/sw/source/core/layout/calcmove.cxx:343)
> ==12555==    by 0x2B770166: SwFrame::Calc(OutputDevice*) const (/sw/source/core/layout/trvlfrm.cxx:1799)
> ==12555==    by 0x2B6A4F11: SwLayAction::IsShortCut(SwPageFrame*&) (/sw/source/core/layout/layact.cxx:1065)
> ==12555==    by 0x2B6A342D: SwLayAction::InternalAction(OutputDevice*) (/sw/source/core/layout/layact.cxx:473)
> ==12555==    by 0x2B6A2E46: SwLayAction::Action(OutputDevice*) (/sw/source/core/layout/layact.cxx:340)
> ==12555==    by 0x2BE0F8A2: SwViewShell::ImplEndAction(bool) (/sw/source/core/view/viewsh.cxx:281)
[...]

Change-Id: I57ebbab536ca41554e4681477cf1dea62abbc688
Reviewed-on: https://gerrit.libreoffice.org/58550
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>

1 files changed, 14 insertions, 17 deletions

diff --git a/sw/source/core/layout/pagechg.cxx b/sw/source/core/layout/pagechg.cxx
index b6aea800609f..e338f9824a3d 100644
--- a/sw/source/core/layout/pagechg.cxx
+++ b/sw/source/core/layout/pagechg.cxx
@@ -273,25 +273,22 @@ void SwPageFrame::DestroyImpl()
         m_pSortedObjs.reset(); // reset to zero to prevent problems when detaching the Flys
     }
 
-    if ( !IsEmptyPage() ) //#59184# unnecessary for empty pages
+    // prevent access to destroyed pages
+    SwDoc *pDoc = GetFormat() ? GetFormat()->GetDoc() : nullptr;
+    if( pDoc && !pDoc->IsInDtor() )
     {
-        // prevent access to destroyed pages
-        SwDoc *pDoc = GetFormat() ? GetFormat()->GetDoc() : nullptr;
-        if( pDoc && !pDoc->IsInDtor() )
+        if ( pSh )
         {
-            if ( pSh )
-            {
-                SwViewShellImp *pImp = pSh->Imp();
-                pImp->SetFirstVisPageInvalid();
-                if ( pImp->IsAction() )
-                    pImp->GetLayAction().SetAgain();
-                // #i9719# - retouche area of page
-                // including border and shadow area.
-                const bool bRightSidebar = (SidebarPosition() == sw::sidebarwindows::SidebarPosition::RIGHT);
-                SwRect aRetoucheRect;
-                SwPageFrame::GetBorderAndShadowBoundRect( getFrameArea(), pSh, pSh->GetOut(), aRetoucheRect, IsLeftShadowNeeded(), IsRightShadowNeeded(), bRightSidebar );
-                pSh->AddPaintRect( aRetoucheRect );
-            }
+            SwViewShellImp *pImp = pSh->Imp();
+            pImp->SetFirstVisPageInvalid();
+            if ( pImp->IsAction() )
+                pImp->GetLayAction().SetAgain();
+            // #i9719# - retouche area of page
+            // including border and shadow area.
+            const bool bRightSidebar = (SidebarPosition() == sw::sidebarwindows::SidebarPosition::RIGHT);
+            SwRect aRetoucheRect;
+            SwPageFrame::GetBorderAndShadowBoundRect( getFrameArea(), pSh, pSh->GetOut(), aRetoucheRect, IsLeftShadowNeeded(), IsRightShadowNeeded(), bRightSidebar );
+            pSh->AddPaintRect( aRetoucheRect );
         }
     }