include/svl/itemset.hxx | 2 - svl/source/items/itemset.cxx | 64 ++----------------------------------------- 2 files changed, 4 insertions(+), 62 deletions(-)
New commits: commit fa98d2da9219a03fb1fb0f213a8bdddf5040fe60 Author: Stephan Bergmann <[email protected]> AuthorDate: Mon Oct 11 09:43:18 2021 +0200 Commit: Stephan Bergmann <[email protected]> CommitDate: Tue Oct 12 14:50:36 2021 +0200 Revert "Use placement new to avoid one of the allocation calls..." This reverts commit 503ab1ca9ae11978d9717557546c01ff598aaf88, plus follow-up 17915ab5202a4d7456e9bc031c3f6a72bc861844 "fix ubsan alloc-dealloc-mismatch". It failed to properly destroy the object assembly, and caused e.g. CppunitTest_svl_items to fail with > ==850754==ERROR: AddressSanitizer: new-delete-type-mismatch on 0x6060000024e0 in thread T0: > object passed to delete has wrong type: > size of the allocated type: 64 bytes; > size of the deallocated type: 56 bytes. > #0 in operator delete(void*, unsigned long) at /home/sbergman/github.com/llvm/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:164:3 (workdir/LinkTarget/Executable/cppunittester +0x330ae2) > #1 in SfxItemSet::~SfxItemSet() at svl/source/items/itemset.cxx:202:1 (instdir/program/libsvllo.so +0x110ccf6) > #2 in std::default_delete<SfxItemSet>::operator()(SfxItemSet*) const at /home/sbergman/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/12.0.0/../../../../include/c++/12.0.0/bits/unique_ptr.h:85:2 (instdir/program/libsvllo.so +0x1142a28) > #3 in std::_Sp_counted_deleter<SfxItemSet*, std::default_delete<SfxItemSet>, std::allocator<void>, (__gnu_cxx::_Lock_policy)2>::_M_dispose() at /home/sbergman/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/12.0.0/../../../../include/c++/12.0.0/bits/shared_ptr_base.h:442:9 (instdir/program/libsvllo.so +0x12696e4) > #4 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() at /home/sbergman/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/12.0.0/../../../../include/c++/12.0.0/bits/shared_ptr_base.h:168:6 (instdir/program/libsvllo.so +0xe500b5) [...] > 0x6060000024e0 is located 0 bytes inside of 64-byte region [0x6060000024e0,0x606000002520) > allocated by thread T0 here: > #0 in operator new(unsigned long) at /home/sbergman/github.com/llvm/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:95:3 (workdir/LinkTarget/Executable/cppunittester +0x32fe7d) > #1 in SfxItemSet::Clone(bool, SfxItemPool*) const at svl/source/items/itemset.cxx:1270:34 (instdir/program/libsvllo.so +0x1127854) > #2 in (anonymous namespace)::Node::setItemSet(SfxItemSet const&) at svl/source/items/stylepool.cxx:65:107 (instdir/program/libsvllo.so +0x1212179) > #3 in StylePoolImpl::insertItemSet(SfxItemSet const&, rtl::OUString const*) at svl/source/items/stylepool.cxx:417:19 (instdir/program/libsvllo.so +0x12103e1) > #4 in StylePool::insertItemSet(SfxItemSet const&, rtl::OUString const*) at svl/source/items/stylepool.cxx:456:17 (instdir/program/libsvllo.so +0x1212ffb) [...] in Clang ASan builds done with -fsized-deallocation. Change-Id: I3ccba7e7d9712ecabf38a0149252d3cd70cdb446 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/123446 Tested-by: Jenkins Reviewed-by: Stephan Bergmann <[email protected]> diff --git a/include/svl/itemset.hxx b/include/svl/itemset.hxx index 5f9da72e3339..5ce13bb1f4c8 100644 --- a/include/svl/itemset.hxx +++ b/include/svl/itemset.hxx @@ -70,8 +70,6 @@ protected: SfxItemSet( SfxItemPool&, SfxAllItemSetFlag ); /** special constructor for SfxItemSetFixed */ SfxItemSet( SfxItemPool&, WhichRangesContainer&& ranges, SfxPoolItem const ** ppItems ); - /** special constructor for Clone */ - SfxItemSet( const SfxItemSet&, SfxPoolItem const ** ppItems ); public: SfxItemSet( const SfxItemSet& ); diff --git a/svl/source/items/itemset.cxx b/svl/source/items/itemset.cxx index 3b47719dbf04..04fab3c9306e 100644 --- a/svl/source/items/itemset.cxx +++ b/svl/source/items/itemset.cxx @@ -136,44 +136,6 @@ SfxItemSet::SfxItemSet( const SfxItemSet& rASet ) assert(svl::detail::validRanges2(m_pWhichRanges)); } -/** special constructor for Clone */ -SfxItemSet::SfxItemSet( const SfxItemSet& rASet, SfxPoolItem const ** ppItems ) - : m_pPool( rASet.m_pPool ) - , m_pParent( rASet.m_pParent ) - , m_ppItems( ppItems ) - , m_pWhichRanges( rASet.m_pWhichRanges ) - , m_nCount( rASet.m_nCount ) - , m_bItemsFixed(true) -{ - if (rASet.m_pWhichRanges.empty()) - return; - - auto nCnt = svl::detail::CountRanges(m_pWhichRanges); - - // Copy attributes - SfxPoolItem const** ppDst = m_ppItems; - SfxPoolItem const** ppSrc = rASet.m_ppItems; - for( sal_uInt16 n = nCnt; n; --n, ++ppDst, ++ppSrc ) - if ( nullptr == *ppSrc || // Current Default? - IsInvalidItem(*ppSrc) || // DontCare? - IsStaticDefaultItem(*ppSrc) ) // Defaults that are not to be pooled? - // Just copy the pointer - *ppDst = *ppSrc; - else if (m_pPool->IsItemPoolable( **ppSrc )) - { - // Just copy the pointer and increase RefCount - *ppDst = *ppSrc; - (*ppDst)->AddRef(); - } - else if ( !(*ppSrc)->Which() ) - *ppDst = (*ppSrc)->Clone(); - else - // !IsPoolable() => assign via Pool - *ppDst = &m_pPool->Put( **ppSrc ); - - assert(svl::detail::validRanges2(m_pWhichRanges)); -} - SfxItemSet::SfxItemSet(SfxItemSet&& rASet) noexcept : m_pPool( rASet.m_pPool ) , m_pParent( rASet.m_pParent ) @@ -1264,20 +1226,9 @@ bool SfxItemSet::Equals(const SfxItemSet &rCmp, bool bComparePool) const std::unique_ptr<SfxItemSet> SfxItemSet::Clone(bool bItems, SfxItemPool *pToPool ) const { - // Use placement new to avoid one of the allocation calls when constructing a new SfxItemSet. - // This is effectively a run-time equivalent of the SfxItemSetFixed template. - const int cnt = TotalCount(); - char* p = static_cast<char*>(::operator new(sizeof(SfxItemSet) + (sizeof(const SfxPoolItem*) * cnt))); - SfxItemSet* pNewItemSet = reinterpret_cast<SfxItemSet*>(p); - const SfxPoolItem** ppNewItems = reinterpret_cast<const SfxPoolItem **>(p + sizeof(SfxItemSet)); if (pToPool && pToPool != m_pPool) { - std::fill(ppNewItems, ppNewItems + cnt, nullptr); - std::unique_ptr<SfxItemSet> pNewSet( - new (pNewItemSet) - SfxItemSet(*pToPool, - WhichRangesContainer(m_pWhichRanges), - ppNewItems)); + std::unique_ptr<SfxItemSet> pNewSet(new SfxItemSet(*pToPool, m_pWhichRanges)); if ( bItems ) { SfxWhichIter aIter(*pNewSet); @@ -1292,17 +1243,10 @@ std::unique_ptr<SfxItemSet> SfxItemSet::Clone(bool bItems, SfxItemPool *pToPool } return pNewSet; } - else if (bItems) - { - return std::unique_ptr<SfxItemSet>( - new (pNewItemSet) SfxItemSet(*this, ppNewItems)); - } else - { - std::fill(ppNewItems, ppNewItems + cnt, nullptr); - return std::unique_ptr<SfxItemSet>( - new (pNewItemSet) SfxItemSet(*m_pPool, WhichRangesContainer(m_pWhichRanges), ppNewItems)); - } + return std::unique_ptr<SfxItemSet>(bItems + ? new SfxItemSet(*this) + : new SfxItemSet(*m_pPool, m_pWhichRanges)); } SfxItemSet SfxItemSet::CloneAsValue(bool bItems, SfxItemPool *pToPool ) const
