Hi Stephan, I have checked Tamás and my changes, and both of them work well with your fix. Many thanks for it and for the detailed commit message!
Best regards, László 2015-06-09 11:27 GMT+02:00 Stephan Bergmann <sberg...@redhat.com>: > On 06/01/2015 07:26 PM, Németh László wrote: >> >> _ImplAdjustVertRelPos( ) function was for only limiting a numerical >> value (vertical position of text frames) without any modification of >> its arguments, but I extended it with a direct modification of a frame >> attribute using this SetFlyFrmAttr call. I am afraid, this was a bad >> idea, when _ImplAdjustVertRelPos() is called during a similar >> modification of an attribute set. Perhaps moving the >> SetFlyFrmAttr/modification to the calling code part will solve this >> problem, I will check it. Thanks for your mail! > > > I have now come up with a solution for this that at least makes ASan+UBSan > "make check" happy, > <http://cgit.freedesktop.org/libreoffice/core/commit/?id=0eb52534de536fbe33585c91f4f173653b184e24> > "Unlock SwCacheObj before potentially deleting it from SwCache." > > László, Tamás, please verify that this does not negatively impact your > changes > <http://cgit.freedesktop.org/libreoffice/core/commit/?id=a4dee94afed9ade6ac50237c8d99a6e49d3bebc1> > "tdf#91260: allow textboxes extending beyond the page bottom" etc. and > <http://cgit.freedesktop.org/libreoffice/core/commit/?id=cb19042f4395c97d123a27c6960d5e30d666c010> > "New feature: vertical alignment for text frames: Layout part." > > And I would not mind any Writer expert looking at that "The hope is..." > part, either. > > >> 2015-06-01 17:39 GMT+02:00 Stephan Bergmann <sberg...@redhat.com>: >>> >>> I'm chasing a problem that my UBSan build originally pointed me at, but >>> where I fail to make good progress. Maybe some Writer expert has a clue. >>> >>> I have broken it down to two pieces of information: >>> >>> For one, valgrind reports a dangling SwModify pointer toward the end of >>> CppunitTest_sw_ooxmlexport, >>> >>>> Invalid read of size 1 >>>> at 0x20FD6659: SwModify::SetInCache(bool) (in >>>> /home/sbergman/lo/core/instdir/program/libswlo.so) >>>> by 0x215FECD7: SwBorderAttrs::~SwBorderAttrs() >>>> (/sw/source/core/layout/frmtool.cxx:1872) >>>> by 0x215FED48: SwBorderAttrs::~SwBorderAttrs() >>>> (/sw/source/core/layout/frmtool.cxx:1871) >>>> by 0x20FFE78B: SwCache::~SwCache() >>>> (/sw/source/core/bastyp/swcache.cxx:123) >>>> by 0x21640B21: _FrmFinit() (/sw/source/core/layout/newfrm.cxx:369) >>>> by 0x20FF86CF: _FinitCore() (/sw/source/core/bastyp/init.cxx:750) >>>> by 0x21EDEBC8: SwDLL::~SwDLL() (/sw/source/uibase/app/swdll.cxx:158) >>>> by 0x21EE045A: std::default_delete<SwDLL>::operator()(SwDLL*) const >>>> >>>> (/usr/lib/gcc/x86_64-redhat-linux/4.9.2/../../../../include/c++/4.9.2/bits/unique_ptr.h:76) >>>> by 0x21EE0598: std::unique_ptr<SwDLL, std::default_delete<SwDLL> >>>>> >>>>> ::reset(SwDLL*) >>>> >>>> >>>> (/usr/lib/gcc/x86_64-redhat-linux/4.9.2/../../../../include/c++/4.9.2/bits/unique_ptr.h:344) >>>> by 0x21EDF5EB: >>>> comphelper::unique_disposing_ptr<SwDLL>::reset(SwDLL*) >>>> (/include/comphelper/unique_disposing_ptr.hxx:41) >>>> by 0x21EDF02D: >>>> comphelper::unique_disposing_solar_mutex_reset_ptr<SwDLL>::reset(SwDLL*) >>>> (/include/comphelper/unique_disposing_ptr.hxx:152) >>>> by 0x21EDFFA1: >>>> >>>> comphelper::unique_disposing_ptr<SwDLL>::TerminateListener::disposing(com::sun::star::lang::EventObject >>>> const&) (/include/comphelper/unique_disposing_ptr.hxx:118) >>>> by 0x21EE01BB: non-virtual thunk to >>>> >>>> comphelper::unique_disposing_ptr<SwDLL>::TerminateListener::disposing(com::sun::star::lang::EventObject >>>> const&) (/include/comphelper/unique_disposing_ptr.hxx:102) >>>> by 0xD4A4561: >>>> >>>> cppu::OInterfaceContainerHelper::disposeAndClear(com::sun::star::lang::EventObject >>>> const&) (/cppuhelper/source/interfacecontainer.cxx:312) >>>> by 0xD4A54C3: >>>> >>>> cppu::OMultiTypeInterfaceContainerHelper::disposeAndClear(com::sun::star::lang::EventObject >>>> const&) (/cppuhelper/source/interfacecontainer.cxx:485) >>>> by 0x2BB8A540: framework::Desktop::disposing() >>>> (/framework/source/services/desktop.cxx:1051) >>>> by 0xD49EB2E: cppu::WeakComponentImplHelperBase::dispose() >>>> (/cppuhelper/source/implbase.cxx:109) >>>> by 0x2BB8E710: >>>> cppu::WeakComponentImplHelper6<com::sun::star::lang::XServiceInfo, >>>> com::sun::star::frame::XDesktop2, com::sun::star::frame::XTasksSupplier, >>>> com::sun::star::frame::XDispatchResultListener, >>>> com::sun::star::task::XInteractionHandler, >>>> com::sun::star::frame::XUntitledNumbers>::dispose() (in >>>> /home/sbergman/lo/core/instdir/program/libfwklo.so) >>>> by 0x2BB8E988: non-virtual thunk to >>>> cppu::WeakComponentImplHelper6<com::sun::star::lang::XServiceInfo, >>>> com::sun::star::frame::XDesktop2, com::sun::star::frame::XTasksSupplier, >>>> com::sun::star::frame::XDispatchResultListener, >>>> com::sun::star::task::XInteractionHandler, >>>> com::sun::star::frame::XUntitledNumbers>::dispose() >>>> (/include/cppuhelper/compbase6.hxx:59) >>>> by 0xD4F3BAD: cppuhelper::ServiceManager::disposing() >>>> (/cppuhelper/source/servicemanager.cxx:925) >>>> by 0xD49EB2E: cppu::WeakComponentImplHelperBase::dispose() >>>> (/cppuhelper/source/implbase.cxx:109) >>>> by 0xD484A70: >>>> cppu::WeakComponentImplHelper8<com::sun::star::lang::XServiceInfo, >>>> com::sun::star::lang::XMultiServiceFactory, >>>> com::sun::star::lang::XMultiComponentFactory, >>>> com::sun::star::container::XSet, >>>> com::sun::star::container::XContentEnumerationAccess, >>>> com::sun::star::beans::XPropertySet, >>>> com::sun::star::beans::XPropertySetInfo, >>>> com::sun::star::lang::XEventListener>::dispose() (in >>>> /home/sbergman/lo/core/instdir/program/libuno_cppuhelpergcc3.so.3) >>>> by 0xD484C78: non-virtual thunk to >>>> cppu::WeakComponentImplHelper8<com::sun::star::lang::XServiceInfo, >>>> com::sun::star::lang::XMultiServiceFactory, >>>> com::sun::star::lang::XMultiComponentFactory, >>>> com::sun::star::container::XSet, >>>> com::sun::star::container::XContentEnumerationAccess, >>>> com::sun::star::beans::XPropertySet, >>>> com::sun::star::beans::XPropertySetInfo, >>>> com::sun::star::lang::XEventListener>::dispose() >>>> (/include/cppuhelper/compbase8.hxx:59) >>>> by 0xD468F85: >>>> >>>> cppu::try_dispose(com::sun::star::uno::Reference<com::sun::star::uno::XInterface> >>>> const&) (/cppuhelper/source/component_context.cxx:276) >>>> by 0xD469A2E: cppu::ComponentContext::disposing() >>>> (/cppuhelper/source/component_context.cxx:724) >>>> by 0xD49EB2E: cppu::WeakComponentImplHelperBase::dispose() >>>> (/cppuhelper/source/implbase.cxx:109) >>>> by 0xD46F880: >>>> cppu::WeakComponentImplHelper<com::sun::star::uno::XComponentContext, >>>> com::sun::star::container::XNameContainer>::dispose() (in >>>> /home/sbergman/lo/core/instdir/program/libuno_cppuhelpergcc3.so.3) >>>> by 0xD46FA68: non-virtual thunk to >>>> cppu::WeakComponentImplHelper<com::sun::star::uno::XComponentContext, >>>> com::sun::star::container::XNameContainer>::dispose() >>>> (/include/cppuhelper/compbase.hxx:82) >>>> by 0x116D49D9: (anonymous >>>> namespace)::Protector::deinitHook((anonymous >>>> namespace)::Protector*, void*) >>>> (/test/source/vclbootstrapprotector.cxx:81) >>>> by 0x116D46C7: (anonymous >>>> namespace)::Protector::LinkStubdeinitHook(void*, void*) >>>> (/test/source/vclbootstrapprotector.cxx:66) >>>> by 0x122B931F: Link<void*, long>::Call(void*) const (in >>>> /home/sbergman/lo/core/instdir/program/libvcllo.so) >>>> by 0x12A49634: DeInitVCL() (/vcl/source/app/svmain.cxx:456) >>>> by 0x116D46F1: (anonymous namespace)::Protector::~Protector() >>>> (/test/source/vclbootstrapprotector.cxx:51) >>>> by 0x116D4748: (anonymous namespace)::Protector::~Protector() >>>> (/test/source/vclbootstrapprotector.cxx:50) >>>> by 0x4EBE750: CppUnit::ProtectorChain::pop() >>>> (/workdir/UnpackedTarball/cppunit/src/cppunit/ProtectorChain.cpp:47) >>>> by 0x4ED9068: CppUnit::TestResult::popProtector() >>>> (/workdir/UnpackedTarball/cppunit/src/cppunit/TestResult.cpp:195) >>>> by 0x405BFF: (anonymous namespace)::ProtectedFixtureFunctor::run() >>>> const (/sal/cppunittester/cppunittester.cxx:285) >>>> by 0x404F0F: sal_main() (/sal/cppunittester/cppunittester.cxx:379) >>>> by 0x404846: main (/sal/cppunittester/cppunittester.cxx:297) >>> >>> >>> >>> but unfortunately the memory is dead long enough already that the point >>> where it got deleted has already been lost from valgrind's database (it >>> only >>> reports "Address 0x311d9b28 is 16 bytes after a block of size 24 free'd" >>> in >>> a delete in basebmp::detail::CompositeIteratorBase<...>, so clearly >>> unrelated). >>> >>> For another, bisecting shows that >>> >>> <http://cgit.freedesktop.org/libreoffice/core/commit/?id=a4dee94afed9ade6ac50237c8d99a6e49d3bebc1> >>> "tdf#91260: allow textboxes extending beyond the page bottom" is what >>> started the dangling pointer dereference to happen during >>> CppunitTest_sw_ooxmlexport. Commenting out the call to SetFlyFrmAttr >>> towards the end of that change's >>> >>>> diff --git a/sw/source/core/objectpositioning/anchoredobjectposition.cxx >>>> b/sw/source/core/objectpositioning/anchoredobjectposition.cxx >>>> index c7b916a..54bf5e0 100644 >>>> --- a/sw/source/core/objectpositioning/anchoredobjectposition.cxx >>>> +++ b/sw/source/core/objectpositioning/anchoredobjectposition.cxx >>>> @@ -471,8 +473,28 @@ SwTwips >>>> SwAnchoredObjectPosition::_ImplAdjustVertRelPos( const SwTwips nTopOfAnc >>>> { >>>> nAdjustedRelPosY = aPgAlignArea.Top() - nTopOfAnch; >>>> } >>>> - } >>>> >>>> + // tdf#91260 - allow textboxes extending beyond the page bottom >>>> + if ( nAdjustedRelPosY < nProposedRelPosY ) >>>> + { >>>> + const SwFrmFmt* pFmt = &(GetFrmFmt()); >>>> + if ( SwTextBoxHelper::isTextBox(&GetObject()) ) >>>> + { >>>> + // shrink textboxes to extend beyond the page bottom >>>> + SwFrmFmt* pFrmFmt = ::FindFrmFmt(&GetObject()); >>>> + SfxItemSet >>>> aTextBoxSet(pFrmFmt->GetDoc()->GetAttrPool(), >>>> aFrmFmtSetRange); >>>> + SwFmtFrmSize aSize(pFmt->GetFrmSize()); >>>> + SwTwips nShrinked = aSize.GetHeight() - >>>> (nProposedRelPosY >>>> - nAdjustedRelPosY); >>>> + aSize.SetHeight( nShrinked > 0 ? nShrinked : 0 ); >>>> + aTextBoxSet.Put(aSize); >>>> + if (aTextBoxSet.Count()) >>>> + pFrmFmt->GetDoc()->SetFlyFrmAttr(*pFrmFmt, >>>> aTextBoxSet); >>>> + nAdjustedRelPosY = nProposedRelPosY; >>>> + } else if ( SwTextBoxHelper::findTextBox(pFmt) ) >>>> + // when the shape has a textbox, use only the proposed >>>> vertical position >>>> + nAdjustedRelPosY = nProposedRelPosY; >>>> + } >>>> + } >>>> return nAdjustedRelPosY; >>>> } >>>> >>> >>> would make the dangling pointer dereference go away again, but it is >>> unclear >>> to me how that call to SetFlyFrmAttr affects the later ~SwCache clean-up. >>> >>> Ideas, anyone? _______________________________________________ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice