[Libreoffice-commits] core.git: Branch 'libreoffice-5-4' - include/svx svx/source

[Michael Stahl]

 include/svx/svdedtv.hxx       |    3 ++-
 svx/source/svdraw/svdedtv.cxx |   41 +++++++++++++++++++++++------------------
 2 files changed, 25 insertions(+), 19 deletions(-)

New commits:
commit c6640f93273475b6d686f14820051cbfa4b1b6c2
Author: Michael Stahl <mstahl at redhat.com>
Date:   Fri Jun 30 12:43:21 2017 +0200

    tdf#108863 svx: fix use-after-free in SdrEditView::DeleteMarkedObj()
    
    The sdr::ViewSelection has multiple vectors with pointers to the same
    SdrObjects, and those are only cleared in
    sdr::ViewSelection::SetEdgesOfMarkedNodesDirty(), so deleting SdrObjects
    that are marked must be delayed until after that is called.
    
    (cherry picked from commit a54ba50db2c341f0f0e47d77dbe64a6e588bc911)
    
    loplugin:unusedvariablecheck
    
    leftover from a54ba50db2c341f0f0e47d77dbe64a6e588bc911 "tdf#108863 svx: fix use-
    after-free in SdrEditView::DeleteMarkedObj()"
    (cherry picked from commit 77f98204ba2fbb51b0a0bb2ac94b839249eec9a4)
    
    Change-Id: I7ab18cb2116164a71dce29bf10eca974061ab424
    Reviewed-on: https://gerrit.libreoffice.org/39418
    Tested-by: Jenkins <ci at libreoffice.org>
    Reviewed-by: Miklos Vajna <vmiklos at collabora.co.uk>

diff --git a/include/svx/svdedtv.hxx b/include/svx/svdedtv.hxx
index f849846f720a..51045e0b0856 100644
--- a/include/svx/svdedtv.hxx
+++ b/include/svx/svdedtv.hxx
@@ -160,7 +160,8 @@ protected:
 
     // Removes all objects of the MarkList from their ObjLists including Undo.
     // The entries in rMark remain.
-    void DeleteMarkedList(const SdrMarkList& rMark); // DeleteMarked -> DeleteMarkedList
+    // @return a list of objects that must be deleted after the outermost EndUndo
+    std::vector<SdrObject *> DeleteMarkedList(SdrMarkList const& rMark); // DeleteMarked -> DeleteMarkedList
 
     // Check possibilities of all marked objects
     virtual void CheckPossibilities();
diff --git a/svx/source/svdraw/svdedtv.cxx b/svx/source/svdraw/svdedtv.cxx
index cdb208e571eb..521ebcfd3ef2 100644
--- a/svx/source/svdraw/svdedtv.cxx
+++ b/svx/source/svdraw/svdedtv.cxx
@@ -687,8 +687,9 @@ void SdrEditView::ForceMarkedObjToAnotherPage()
     }
 }
 
-void SdrEditView::DeleteMarkedList(const SdrMarkList& rMark)
+std::vector<SdrObject*> SdrEditView::DeleteMarkedList(SdrMarkList const& rMark)
 {
+    std::vector<SdrObject*> ret;
     if (rMark.GetMarkCount()!=0)
     {
         rMark.ForceSort();
@@ -721,8 +722,6 @@ void SdrEditView::DeleteMarkedList(const SdrMarkList& rMark)
             // make sure, OrderNums are correct:
             rMark.GetMark(0)->GetMarkedSdrObj()->GetOrdNum();
 
-            std::vector< SdrObject* > aRemoved3DObjects;
-
             for(size_t nm = nMarkCount; nm > 0;)
             {
                 --nm;
@@ -742,10 +741,8 @@ void SdrEditView::DeleteMarkedList(const SdrMarkList& rMark)
 
                 if( !bUndo )
                 {
-                    if( bIs3D )
-                        aRemoved3DObjects.push_back( pObj ); // may be needed later
-                    else
-                        SdrObject::Free(pObj);
+                    // tdf#108863 don't delete objects before EndUndo()
+                    ret.push_back(pObj);
                 }
             }
 
@@ -755,21 +752,22 @@ void SdrEditView::DeleteMarkedList(const SdrMarkList& rMark)
                 delete aUpdaters.back();
                 aUpdaters.pop_back();
             }
-
-            if( !bUndo )
-            {
-                // now delete removed scene objects
-                while(!aRemoved3DObjects.empty())
-                {
-                    SdrObject::Free( aRemoved3DObjects.back() );
-                    aRemoved3DObjects.pop_back();
-                }
-            }
         }
 
         if( bUndo )
             EndUndo();
     }
+    return ret;
+}
+
+static void lcl_LazyDelete(std::vector<SdrObject*> & rLazyDelete)
+{
+    // now delete removed scene objects
+    while (!rLazyDelete.empty())
+    {
+        SdrObject::Free( rLazyDelete.back() );
+        rLazyDelete.pop_back();
+    }
 }
 
 void SdrEditView::DeleteMarkedObj()
@@ -784,6 +782,7 @@ void SdrEditView::DeleteMarkedObj()
     BrkAction();
     BegUndo(ImpGetResStr(STR_EditDelete),GetDescriptionOfMarkedObjects(),SdrRepeatFunc::Delete);
 
+    std::vector<SdrObject*> lazyDeleteObjects;
     // remove as long as something is selected. This allows to schedule objects for
     // removal for a next run as needed
     while(GetMarkedObjectCount())
@@ -844,7 +843,11 @@ void SdrEditView::DeleteMarkedObj()
 
         // original stuff: remove selected objects. Handle clear will
         // do something only once
-        DeleteMarkedList(GetMarkedObjectList());
+        auto temp(DeleteMarkedList(GetMarkedObjectList()));
+        for (auto p : temp)
+        {
+            lazyDeleteObjects.push_back(p);
+        }
         GetMarkedObjectListWriteAccess().Clear();
         maHdlList.Clear();
 
@@ -874,6 +877,8 @@ void SdrEditView::DeleteMarkedObj()
     // end undo and change messaging moved at the end
     EndUndo();
     MarkListHasChanged();
+
+    lcl_LazyDelete(lazyDeleteObjects);
 }
 
 void SdrEditView::CopyMarkedObj()