From 13bb5a4b09f5b2ad19dad1b55f45d0fe2b2fb908 Mon Sep 17 00:00:00 2001 From: Stephan Bergmann Date: Fri, 16 Jun 2017 09:58:13 +0200 Subject: Make SfxItemSet ranges correct by construction This is a follow-up to 45a7f5b62d0b1b21763c1c94255ef2309ea4280b "Keep WID ranges sorted, and join adjacent ones". While SfxItemSet::MergeRange relies on the m_pWhichRanges being sorted (and, under DBG_UTIL, asserts if they are not), the various SfxItemSet constructors curiously only check (via assert or DBG_ASSERT) that each individual range has an upper bound not smaller than its lower bound. Arguably, all SfxItemSet instances should fulfill the stronger guarantees required and checked by MergeRange. And in many cases the ranges are statically known, so that the checking can happen at compile time. Therefore, replace the two SfxItemSet ctors taking explicit ranges with two other ctors that actually do proper checking. The (templated) overload taking an svl::Items struct should be used in all cases where the range values are statically known at compile time, while the overload taking a std::initializer_list is for the remaining cases (that can only do runtime checking via assert). Most of those latter cases are simple cases with a single range covering a single item, but a few are more complex. (At least some of the uses of the existing SfxItemSet overload taking a const sal_uInt16* pWhichPairTable can probably also be strengthened, but that is left for another day.) This commit is the first in a series of two. Apart from the manual changes to compilerplugins/clang/store/sfxitemsetrewrite.cxx, include/svl/itemset.hxx, and svl/source/items/itemset.cxx, it only consists of automatic rewriting of the relevant SfxItemSet ctor calls (plus a few required manual fixes, see next). But it does not yet check that the individual ranges are properly sorted (see the TODO in svl::detail::validGap). That check will be enabled, and the ensuing manual fixes will be made in a follow-up commit, to reduce the likelyhood of accidents. There were three cases of necessary manual intervention: * sw/source/core/unocore/unostyle.cxx uses eAtr of enum type RES_FRMATR in braced-init-list syntax now, so needs explicit narrowing conversion to sal_uInt16. * In sw/source/uibase/uiview/formatclipboard.cxx, the trailiing comma in the definition of macro FORMAT_PAINTBRUSH_FRAME_IDS needed to be removed manually. * In svx/source/svdraw/svdoashp.cxx, svx/source/svdraw/svdotext.cxx, sw/source/uibase/app/docstyle.cxx, sw/source/uibase/shells/frmsh.cxx, sw/source/uibase/shells/grfsh.cxx, and sw/source/uibase/shells/textsh1.cxx, some comments had to be put back (see "TODO: the replaced range can contain relevant comments" in compilerplugins/clang/store/sfxitemsetrewrite.cxx). A few uses of the variadic form erroneously used nullptr instead of 0 for termination. But this should have been harmless even if promoted std::nullptr_t is larger than promoted sal_uInt16, assuming that the part of the nullptr value that was interpreted as sal_uInt16/promoted int was all-zero bits. Similarly, some uses made the harmless error of using 0L instead of 0. Change-Id: I2afea97282803cb311b9321a99bb627520ef5e35 Reviewed-on: https://gerrit.libreoffice.org/38861 Reviewed-by: Stephan Bergmann Tested-by: Stephan Bergmann --- svl/source/items/itemset.cxx | 140 +++++++++++++++---------------------------- 1 file changed, 49 insertions(+), 91 deletions(-) (limited to 'svl') diff --git a/svl/source/items/itemset.cxx b/svl/source/items/itemset.cxx index 664e36acfcfe..39ec0a18222c 100644 --- a/svl/source/items/itemset.cxx +++ b/svl/source/items/itemset.cxx @@ -20,8 +20,10 @@ #include +#include #include -#include +#include + #include #include @@ -41,50 +43,6 @@ static const sal_uInt16 nInitCount = 10; // Single USHORTs => 5 pairs without '0 namespace { -/** - * Creates a sal_uInt16-ranges-array in 'rpRanges' using 'nWh1' and 'nWh2' as - * first range, 'nNull' as terminator or start of 2nd range and 'pArgs' as - * remainder. - * - * It returns the number of sal_uInt16s which are contained in the described - * set of sal_uInt16s. - */ -sal_uInt16 InitializeRanges_Impl( sal_uInt16 *&rpRanges, va_list pArgs, - sal_uInt16 nWh1, sal_uInt16 nWh2, sal_uInt16 nNull ) -{ - sal_uInt16 nSize = 0, nIns = 0; - std::vector aNumArr; - aNumArr.push_back( nWh1 ); - aNumArr.push_back( nWh2 ); - DBG_ASSERT( nWh1 <= nWh2, "Invalid range" ); - nSize += nWh2 - nWh1 + 1; - aNumArr.push_back( nNull ); - bool bEndOfRange = false; - while ( 0 != - ( nIns = - sal::static_int_cast< sal_uInt16 >( - va_arg( pArgs, int ) ) ) ) - { - bEndOfRange = !bEndOfRange; - if ( bEndOfRange ) - { - const sal_uInt16 nPrev(*aNumArr.rbegin()); - DBG_ASSERT( nPrev <= nIns, "Invalid range" ); - nSize += nIns - nPrev + 1; - } - aNumArr.push_back( nIns ); - } - - assert( bEndOfRange ); // odd number of WhichIds - - // Now all ranges are present - rpRanges = new sal_uInt16[ aNumArr.size() + 1 ]; - std::copy( aNumArr.begin(), aNumArr.end(), rpRanges); - *(rpRanges + aNumArr.size()) = 0; - - return nSize; -} - /** * Determines the number of sal_uInt16s in a 0-terminated array of pairs of * sal_uInt16s. @@ -143,52 +101,6 @@ SfxItemSet::SfxItemSet(SfxItemPool& rPool) m_pItems = new const SfxPoolItem*[nSize]{}; } -SfxItemSet::SfxItemSet(SfxItemPool& rPool, sal_uInt16 nWhich1, sal_uInt16 nWhich2) - : m_pPool( &rPool ) - , m_pParent(nullptr) - , m_nCount(0) -{ - assert(nWhich1 <= nWhich2); - - InitRanges_Impl(nWhich1, nWhich2); -} - -void SfxItemSet::InitRanges_Impl(sal_uInt16 nWh1, sal_uInt16 nWh2) -{ - m_pWhichRanges = new sal_uInt16[3]{nWh1, nWh2, 0}; - const sal_uInt16 nRg = nWh2 - nWh1 + 1; - m_pItems = new const SfxPoolItem*[nRg]{}; -} - -void SfxItemSet::InitRanges_Impl(va_list pArgs, sal_uInt16 nWh1, sal_uInt16 nWh2, sal_uInt16 nNull) -{ - sal_uInt16 nSize = InitializeRanges_Impl(m_pWhichRanges, pArgs, nWh1, nWh2, nNull); - m_pItems = new const SfxPoolItem*[nSize]{}; -} - -SfxItemSet::SfxItemSet(SfxItemPool& rPool, int nWh1, int nWh2, int nNull, ...) - : m_pPool( &rPool ) - , m_pParent(nullptr) - , m_pWhichRanges(nullptr) - , m_nCount(0) -{ - assert(nWh1 <= nWh2); - - if(!nNull) - InitRanges_Impl( - sal::static_int_cast< sal_uInt16 >(nWh1), - sal::static_int_cast< sal_uInt16 >(nWh2)); - else { - va_list pArgs; - va_start( pArgs, nNull ); - InitRanges_Impl( - pArgs, sal::static_int_cast< sal_uInt16 >(nWh1), - sal::static_int_cast< sal_uInt16 >(nWh2), - sal::static_int_cast< sal_uInt16 >(nNull)); - va_end(pArgs); - } -} - void SfxItemSet::InitRanges_Impl(const sal_uInt16 *pWhichPairTable) { sal_uInt16 nCnt = 0; @@ -206,6 +118,52 @@ void SfxItemSet::InitRanges_Impl(const sal_uInt16 *pWhichPairTable) memcpy( m_pWhichRanges, pWhichPairTable, sizeof( sal_uInt16 ) * cnt ); } +SfxItemSet::SfxItemSet( + SfxItemPool & pool, std::initializer_list wids, + std::size_t items): + m_pPool(&pool), m_pParent(nullptr), + m_pItems(new SfxPoolItem const *[items]{}), + m_pWhichRanges(new sal_uInt16[wids.size() + 1]), + // cannot overflow, assuming std::size_t is no smaller than sal_uInt16, + // as wids.size() must be substantially smaller than + // std::numeric_limits::max() by construction in + // SfxItemSet::create + m_nCount(0) +{ + assert(wids.size() != 0); + assert(wids.size() % 2 == 0); + std::copy(wids.begin(), wids.end(), m_pWhichRanges); + m_pWhichRanges[wids.size()] = 0; +} + +SfxItemSet::SfxItemSet( + SfxItemPool & pool, std::initializer_list wids): + m_pPool(&pool), m_pParent(nullptr), + m_pWhichRanges(new sal_uInt16[2 * wids.size() + 1]), //TODO: overflow + m_nCount(0) +{ + assert(wids.size() != 0); + std::size_t i = 0; + std::size_t size = 0; +#if !defined NDEBUG + sal_uInt16 prev = 0; +#endif + for (auto const & p: wids) { + assert(svl::detail::validRange(p.wid1, p.wid2)); + assert(prev == 0 || svl::detail::validGap(prev, p.wid1)); + m_pWhichRanges[i++] = p.wid1; + m_pWhichRanges[i++] = p.wid2; + size += svl::detail::rangeSize(p.wid1, p.wid2); + // cannot overflow, assuming std::size_t is no smaller than + // sal_uInt16 +#if !defined NDEBUG + prev = p.wid2; +#endif + } + m_pWhichRanges[i] = 0; + m_pItems = new SfxPoolItem const *[size]{}; +} + SfxItemSet::SfxItemSet( SfxItemPool& rPool, const sal_uInt16* pWhichPairTable ) : m_pPool(&rPool) , m_pParent(nullptr) -- cgit