include/com/sun/star/uno/Sequence.h | 2 ++
include/com/sun/star/uno/Sequence.hxx | 2 ++
oox/source/drawingml/customshapeproperties.cxx | 15 +++++++++++----
3 files changed, 15 insertions(+), 4 deletions(-)
New commits:
commit fb3c04bd1930eedacd406874e1a285d62bbf27d9
Author: Mike Kaganski <mike.kagan...@collabora.com>
AuthorDate: Fri Oct 29 10:30:22 2021 +0300
Commit: Mike Kaganski <mike.kagan...@collabora.com>
CommitDate: Fri Nov 5 12:30:28 2021 +0100
Drop non-const Sequence::operator[] in internal code
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
2484de6728bd11bb7949003d112f1ece2223c7a1, 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.kagan...@collabora.com>
Reviewed-by: Mike Kaganski <mike.kagan...@collabora.com>
diff --git a/include/com/sun/star/uno/Sequence.h
b/include/com/sun/star/uno/Sequence.h
index cc2c515f1322..b557ec87b4ca 100644
--- a/include/com/sun/star/uno/Sequence.h
+++ b/include/com/sun/star/uno/Sequence.h
@@ -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!
diff --git a/include/com/sun/star/uno/Sequence.hxx
b/include/com/sun/star/uno/Sequence.hxx
index 73e51a9bc7f5..7aa873e91223 100644
--- a/include/com/sun/star/uno/Sequence.hxx
+++ b/include/com/sun/star/uno/Sequence.hxx
@@ -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
diff --git a/oox/source/drawingml/customshapeproperties.cxx
b/oox/source/drawingml/customshapeproperties.cxx
index 37c42b5119dc..0aba70b7a337 100644
--- a/oox/source/drawingml/customshapeproperties.cxx
+++ b/oox/source/drawingml/customshapeproperties.cxx
@@ -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" );
}
}
}
