From 34c2a1dcf05661562cd00fe6dbdf884d8917933d Mon Sep 17 00:00:00 2001
From: Marc Mutz <marc.mutz@kdab.com>
Date: Tue, 27 Sep 2016 15:30:15 +0200
Subject: [PATCH] QFormLayout: fix use-after-free in clearQLayoutItem()

Found by ASan when it should have been found by me in the initial
review...

The old code did, in this order:

1. delete item->layout() (which deletes item, as QLayoutItem::layout()
   is just a dynamic_cast implemented with virtual functions)

2. delete item->widget() (which is correct, but too late, as 'item'
   may have already been deleted; this is the first use-after-free
   bug)

3. delete item->spacerItem() (which is the second use-after-free).

Fix by deleting item->widget() (which may be a no-op), _then_ checking
for a QLayout, deleting all children (but not the layout), and finally
deleting item as a QLayoutItem.

Rename clearQLayoutItem() to clearAndDestroyQLayoutItem() to better
match what it actually does.

Change-Id: Id70a7a137dac5de466ef619f01bfd61dfc150418
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
Reviewed-by: Samuel Gaist <samuel.gaist@edeltech.ch>
Reviewed-by: Marc Mutz <marc.mutz@kdab.com>
---
 src/widgets/kernel/qformlayout.cpp | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/src/widgets/kernel/qformlayout.cpp b/src/widgets/kernel/qformlayout.cpp
index 3e60723f17..f3f7280030 100644
--- a/src/widgets/kernel/qformlayout.cpp
+++ b/src/widgets/kernel/qformlayout.cpp
@@ -1410,18 +1410,15 @@ static QLayoutItem *ownershipCleanedItem(QFormLayoutItem *item, QFormLayout *lay
     return i;
 }
 
-static void clearQLayoutItem(QLayoutItem *item)
+static void clearAndDestroyQLayoutItem(QLayoutItem *item)
 {
     if (Q_LIKELY(item)) {
-        if (QLayout *layout = item->layout()) {
-            while (QLayoutItem *child = layout->takeAt(0)) {
-                clearQLayoutItem(child);
-                delete child;
-            }
-            delete layout;
-        }
         delete item->widget();
-        delete item->spacerItem();
+        if (QLayout *layout = item->layout()) {
+            while (QLayoutItem *child = layout->takeAt(0))
+                clearAndDestroyQLayoutItem(child);
+        }
+        delete item;
     }
 }
 
@@ -1453,8 +1450,8 @@ static void clearQLayoutItem(QLayoutItem *item)
 void QFormLayout::removeRow(int row)
 {
     TakeRowResult result = takeRow(row);
-    clearQLayoutItem(result.labelItem);
-    clearQLayoutItem(result.fieldItem);
+    clearAndDestroyQLayoutItem(result.labelItem);
+    clearAndDestroyQLayoutItem(result.fieldItem);
 }
 
 /*!
@@ -1485,8 +1482,8 @@ void QFormLayout::removeRow(int row)
 void QFormLayout::removeRow(QWidget *widget)
 {
     TakeRowResult result = takeRow(widget);
-    clearQLayoutItem(result.labelItem);
-    clearQLayoutItem(result.fieldItem);
+    clearAndDestroyQLayoutItem(result.labelItem);
+    clearAndDestroyQLayoutItem(result.fieldItem);
 }
 
 /*!
@@ -1518,8 +1515,8 @@ void QFormLayout::removeRow(QWidget *widget)
 void QFormLayout::removeRow(QLayout *layout)
 {
     TakeRowResult result = takeRow(layout);
-    clearQLayoutItem(result.labelItem);
-    clearQLayoutItem(result.fieldItem);
+    clearAndDestroyQLayoutItem(result.labelItem);
+    clearAndDestroyQLayoutItem(result.fieldItem);
 }
 
 /*!