From fb3c04bd1930eedacd406874e1a285d62bbf27d9 Mon Sep 17 00:00:00 2001 From: Mike Kaganski Date: Fri, 29 Oct 2021 10:30:22 +0300 Subject: 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 Reviewed-by: Mike Kaganski --- oox/source/drawingml/customshapeproperties.cxx | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) (limited to 'oox') 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" ); } } } -- cgit