diff options
author | Marc Mutz <marc.mutz@kdab.com> | 2016-09-27 15:30:15 +0200 |
---|---|---|
committer | Marc Mutz <marc.mutz@kdab.com> | 2016-11-22 11:32:13 +0000 |
commit | 34c2a1dcf05661562cd00fe6dbdf884d8917933d (patch) | |
tree | 0548c89b2fa144b36a07bdde934d4239e08d26e8 | |
parent | 7920cfe46ad11ce1cd5592a71cfa16422e9207e1 (diff) |
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>
-rw-r--r-- | src/widgets/kernel/qformlayout.cpp | 25 |
1 files changed, 11 insertions, 14 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)) { + delete item->widget(); if (QLayout *layout = item->layout()) { - while (QLayoutItem *child = layout->takeAt(0)) { - clearQLayoutItem(child); - delete child; - } - delete layout; + while (QLayoutItem *child = layout->takeAt(0)) + clearAndDestroyQLayoutItem(child); } - delete item->widget(); - delete item->spacerItem(); + 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); } /*! |