QGAL: Fix memory management issues (leak + invalid read)

Fixing QGraphicsAnchor memory leak and access to free'd region.

-- Leak:

User-created anchors have two representations in QGAL, one
visible externally (QGraphicsAnchor) and other internal (AnchorData).

When such anchors are removed externally (QGraphicsAnchor is deleted),
the former implementation ensured that the internal representation
would be deleted too. However the opposite was not true. In cases
where the anchors are deleted internally (in the layout destructor,
for instance, or when an item is removed through the removeAt API),
the public QGraphicsAnchor object would leak.

This commit ensures the deletion will happen in both directions
and adds protection to avoid a deletion loop.

-- Invalid read:

In QGAL::removeAnchor(vertex1, vertex2), we read vertex information
after calling removeAnchor_helper(vertex1, vertex2).

The problem is that in cases where the removed anchor is the last
anchor to connect to a center vertex, its removal will cause also
the removal of such vertex. Thus, accessing the vertices after
the removeAnchor_helper() call is unsafe.

To solve that we cache the information we need and then clear the
vertex pointers to avoid errors in the future.

Signed-off-by: Eduardo M. Fleury <eduardo.fleury@openbossa.org>
Reviewed-by: Artur Duque de Souza <artur.souza@openbossa.org>

2 files changed, 27 insertions, 6 deletions

diff --git a/src/gui/graphicsview/qgraphicsanchorlayout_p.cpp b/src/gui/graphicsview/qgraphicsanchorlayout_p.cpp
index 137806a..b698c02 100644
--- a/src/gui/graphicsview/qgraphicsanchorlayout_p.cpp
+++ b/src/gui/graphicsview/qgraphicsanchorlayout_p.cpp
@@ -62,7 +62,13 @@ QGraphicsAnchorPrivate::QGraphicsAnchorPrivate(int version)
 
 QGraphicsAnchorPrivate::~QGraphicsAnchorPrivate()
 {
-    layoutPrivate->removeAnchor(data->from, data->to);
+    if (data) {
+        // The QGraphicsAnchor was already deleted at this moment. We must clean
+        // the dangling pointer to avoid double deletion in the AnchorData dtor.
+        data->graphicsAnchor = 0;
+
+        layoutPrivate->removeAnchor(data->from, data->to);
+    }
 }
 
 void QGraphicsAnchorPrivate::setSizePolicy(QSizePolicy::Policy policy)
@@ -167,6 +173,18 @@ static void internalSizeHints(QSizePolicy::Policy policy,
         *prefSize = prefSizeHint;
 }
 
+AnchorData::~AnchorData()
+{
+    if (graphicsAnchor) {
+        // Remove reference to ourself to avoid double removal in
+        // QGraphicsAnchorPrivate dtor.
+        graphicsAnchor->d_func()->data = 0;
+
+        delete graphicsAnchor;
+    }
+}
+
+
 void AnchorData::refreshSizeHints(const QLayoutStyleInfo *styleInfo)
 {
     QSizePolicy::Policy policy;
@@ -1687,12 +1705,16 @@ void QGraphicsAnchorLayoutPrivate::removeAnchor(AnchorVertex *firstVertex,
 {
     Q_Q(QGraphicsAnchorLayout);
 
-    // Actually delete the anchor
-    removeAnchor_helper(firstVertex, secondVertex);
-
+    // Save references to items while it's safe to assume the vertices exist
     QGraphicsLayoutItem *firstItem = firstVertex->m_item;
     QGraphicsLayoutItem *secondItem = secondVertex->m_item;
 
+    // Delete the anchor (may trigger deletion of center vertices)
+    removeAnchor_helper(firstVertex, secondVertex);
+
+    // Ensure no dangling pointer is left behind
+    firstVertex = secondVertex = 0;
+
     // Checking if the item stays in the layout or not
     bool keepFirstItem = false;
     bool keepSecondItem = false;
diff --git a/src/gui/graphicsview/qgraphicsanchorlayout_p.h b/src/gui/graphicsview/qgraphicsanchorlayout_p.h
index 8529e2e..e96110c 100644
--- a/src/gui/graphicsview/qgraphicsanchorlayout_p.h
+++ b/src/gui/graphicsview/qgraphicsanchorlayout_p.h
@@ -128,12 +128,11 @@ struct AnchorData : public QSimplexVariable {
           type(Normal), isLayoutAnchor(false),
           isCenterAnchor(false), orientation(0),
           dependency(Independent) {}
+    virtual ~AnchorData();
 
     virtual void updateChildrenSizes() {}
     void refreshSizeHints(const QLayoutStyleInfo *styleInfo = 0);
 
-    virtual ~AnchorData() {}
-
 #ifdef QT_DEBUG
     void dump(int indent = 2);
     inline QString toString() const;