strepsirrhini/office-gobmx
9
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Drop non-const Sequence::operator[] in internal code

Browse source 
This makes all non-const operations on Sequence explicit, to avoid
repeated COW checks in loops. Generally it would be desirable to
replace uses of Sequence with general-purpose containers wherever
possible, and only use Sequence for UNO API calls.

This change uncovered a very serious pre-existing problem inherent
to the Sequences used e.g. in range-based loops in our code: taking
a non-const reference to elements of a sequence, and then modifying
them at some later stage, brings a danger to also modify copies of
the Sequence, that were created between the points of taking the
reference and modifying it. This caused the change to
oox/source/drawingml/customshapeproperties.cxx, where
CustomShapeProperties::pushToPropSet took begin()/end() non-const
iterators to aGeoPropSeq at the start of the loop, and then in the
loop modified its elements and copied the Sequence passing it to
XPropertySet::setPropertyValue. This was the same also prior to
2484de6728, and only happened to not
cause problems because of *accidental* use of non-const operator[]
on the copy of the Sequence inside SdrCustomShapeGeometryItem ctor,
which *inadvertently* triggered COW, detaching the two copies one
from another.
This only emphasizes that we should minimize use of Sequences in
the codebase. I anticipate other similar problems uncovered by this
change, that happened to not break existing unit tests.

Change-Id: Id691d994a06eb14297c487ebb84d8e062e29fd47
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/123725
Tested-by: Mike Kaganski <mike.kaganski@collabora.com>
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>
This commit is contained in:
Mike Kaganski 2021-10-29 10:30:22 +03:00
parent 1a62041a44
commit fb3c04bd19
3 changed files with 15 additions and 4 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
include/com/sun/star/uno/Sequence.h
View file

@ -217,6 +217,7 @@ public:
*/
inline E const * end() const;
#if !defined LIBO_INTERNAL_ONLY
/** Non-const index operator: Obtains a reference to element indexed at
given position.
The implementation does not check for array bounds!
@ -228,6 +229,7 @@ public:
@return non-const C++ reference to element
*/
inline E & SAL_CALL operator [] ( sal_Int32 nIndex );
#endif
/** Const index operator: Obtains a reference to element indexed at
given position. The implementation does not check for array bounds!

2
include/com/sun/star/uno/Sequence.hxx
View file

@ -180,6 +180,7 @@ template<class E> E * Sequence<E>::end() { return begin() + getLength(); }
template<class E> E const * Sequence<E>::end() const
{ return begin() + getLength(); }
#if !defined LIBO_INTERNAL_ONLY
template< class E >
inline E & Sequence< E >::operator [] ( sal_Int32 nIndex )
{
@ -187,6 +188,7 @@ inline E & Sequence< E >::operator [] ( sal_Int32 nIndex )
assert(nIndex >= 0 && static_cast<sal_uInt32>(nIndex) < static_cast<sal_uInt32>(getLength()));
return getArray()[ nIndex ];
}
#endif
template< class E >
inline const E & Sequence< E >::operator [] ( sal_Int32 nIndex ) const

15
oox/source/drawingml/customshapeproperties.cxx
View file

@ -182,8 +182,11 @@ void CustomShapeProperties::pushToPropSet(
static const OUStringLiteral sType = u"Type";
if ( aGeoPropSet >>= aGeoPropSeq )
{
for ( auto& rGeoProp : asNonConstRange(aGeoPropSeq) )
// aGeoPropSeq gets modified in the loop, and gets copied elsewhere;
// don't use range-based for here
for ( sal_Int32 i = 0; i < aGeoPropSeq.getLength(); ++i )
{
const auto& rGeoProp = aGeoPropSeq[i];
if ( rGeoProp.Name == sAdjustmentValues )
{
uno::Sequence< css::drawing::EnhancedCustomShapeAdjustmentValue > aAdjustmentSeq;
@ -216,16 +219,20 @@ void CustomShapeProperties::pushToPropSet(
}
}
}
rGeoProp.Value <<= aAdjustmentSeq;
// getArray ensures COW here - there may be copies of aGeoPropSeq from
// prior calls to xPropSet->setPropertyValue, so getArray can't be
// moved out of the loop:
aGeoPropSeq.getArray()[i].Value <<= aAdjustmentSeq;
xPropSet->setPropertyValue( sCustomShapeGeometry, Any( aGeoPropSeq ) );
}
}
else if ( rGeoProp.Name == sType )
{
// getArray ensures COW here - there may be copies of aGeoPropSeq:
if ( sConnectorShapeType.getLength() > 0 )
rGeoProp.Value <<= sConnectorShapeType;
aGeoPropSeq.getArray()[i].Value <<= sConnectorShapeType;
else
rGeoProp.Value <<= OUString( "ooxml-CustomShape" );
aGeoPropSeq.getArray()[i].Value <<= OUString( "ooxml-CustomShape" );
}
}
}