Plug leak in QFormLayout::setWidget()

Unlike layouts and spacer items, a QWidget is-not-a QLayoutItem.
QWidgetItem simply wraps the QWidget, so in QFormLayout::setWidget(),
we allocate a widget item for the widget passed, and hand that down
to Private::setItem() for adding to the various data structures.

Private::setItem() has a bunch of guard clauses, though, that return
without deleting the item.

A test triggered this code path and made asan complain.

This is just one part of a larger problem: QFormLayout::setLayout()
normally takes ownership of the layout passed, because QLayouts own
their QLayoutItems, and QLayout is-a QLayoutItem. But setLayout()
fails to live up to the owner role when it fails to add a layout,
and there's no easy way for the API user to check for success.

A fix for this breaks tst_qformlayout, and while those checks that
break deserve to be broken, I'll refrain from proposing the larger
fix for 5.6 LTS, but will propose it for 5.8 or 5.9 instead.

This fix here only fixes the leak in setWidget() by adding a bool
return to Private::setItem() informing Private::setWidget() of the
need to manually delete the item it allocated for the widget.

Change-Id: I81409c260f9bee2e95c9a98542d8c60bc19a1332
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
Reviewed-by: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>

1 files changed, 9 insertions, 6 deletions

diff --git a/src/widgets/kernel/qformlayout.cpp b/src/widgets/kernel/qformlayout.cpp
index 24bf80f16c..0ae617d945 100644
--- a/src/widgets/kernel/qformlayout.cpp
+++ b/src/widgets/kernel/qformlayout.cpp
@@ -179,7 +179,7 @@ public:
 
     int insertRow(int row);
     void insertRows(int row, int count);
-    void setItem(int row, QFormLayout::ItemRole role, QLayoutItem *item);
+    bool setItem(int row, QFormLayout::ItemRole role, QLayoutItem *item);
     void setLayout(int row, QFormLayout::ItemRole role, QLayout *layout);
     void setWidget(int row, QFormLayout::ItemRole role, QWidget *widget);
 
@@ -941,21 +941,21 @@ void QFormLayoutPrivate::insertRows(int row, int count)
     }
 }
 
-void QFormLayoutPrivate::setItem(int row, QFormLayout::ItemRole role, QLayoutItem *item)
+bool QFormLayoutPrivate::setItem(int row, QFormLayout::ItemRole role, QLayoutItem *item)
 {
     const bool fullRow = role == QFormLayout::SpanningRole;
     const int column =  role == QFormLayout::SpanningRole ? 1 : static_cast<int>(role);
     if (uint(row) >= uint(m_matrix.rowCount()) || uint(column) > 1U) {
         qWarning("QFormLayoutPrivate::setItem: Invalid cell (%d, %d)", row, column);
-        return;
+        return false;
     }
 
     if (!item)
-        return;
+        return false;
 
     if (m_matrix(row, column)) {
         qWarning("QFormLayoutPrivate::setItem: Cell (%d, %d) already occupied", row, column);
-        return;
+        return false;
     }
 
     QFormLayoutItem *i = new QFormLayoutItem(item);
@@ -963,6 +963,7 @@ void QFormLayoutPrivate::setItem(int row, QFormLayout::ItemRole role, QLayoutIte
     m_matrix(row, column) = i;
 
     m_things.append(i);
+    return true;
 }
 
 void QFormLayoutPrivate::setLayout(int row, QFormLayout::ItemRole role, QLayout *layout)
@@ -979,7 +980,9 @@ void QFormLayoutPrivate::setWidget(int row, QFormLayout::ItemRole role, QWidget
     if (widget) {
         Q_Q(QFormLayout);
         q->addChildWidget(widget);
-        setItem(row, role, QLayoutPrivate::createWidgetItem(q, widget));
+        QWidgetItem *item = QLayoutPrivate::createWidgetItem(q, widget);
+        if (!setItem(row, role, item))
+            delete item;
     }
 }