Writer problem with a dangling SwModify pointer
Németh László
nemeth at numbertext.org
Tue Jun 9 08:00:24 PDT 2015
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 <sbergman at 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 <sbergman at 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?
More information about the LibreOffice
mailing list