[Libreoffice-commits] core.git: sw/source
Libreoffice Gerrit user
logerrit at kemper.freedesktop.org
Fri Aug 3 20:57:30 UTC 2018
sw/source/core/layout/pagechg.cxx | 31 ++++++++++++++-----------------
1 file changed, 14 insertions(+), 17 deletions(-)
New commits:
commit 554e090538bd8aa0948b010d2d80dc54b8e1d3bd
Author: Stephan Bergmann <sbergman at redhat.com>
AuthorDate: Fri Aug 3 10:25:12 2018 +0200
Commit: Stephan Bergmann <sbergman at redhat.com>
CommitDate: Fri Aug 3 22:57:05 2018 +0200
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 at redhat.com>
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 );
}
}
More information about the Libreoffice-commits
mailing list