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.kaganski@collabora.com>
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>

1 files changed, 11 insertions, 4 deletions

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" );
                     }
                 }
             }