From c876bb1f1333e47722e202b0916415e771137071 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Mon, 11 Jul 2016 19:55:24 +0200 Subject: [PATCH] QInputDialog: prevent crash in static get*() functions when parent gets deleted As explained in https://blogs.kde.org/2009/03/26/how-crash-almost-every-qtkde-application-and-how-fix-it-0 creating dialogs on the stack is a bad idea if the application or the dialog's parent window can be closed by means other than user interaction (such as a timer or an IPC call). Since we cannot know whether Qt is used to build such an application, we must assume it is, create the dialog on the heap, and monitor its lifetime with a QPointer. Instead of using manual resource management, add a minimal implementation of QAutoPointer, and use that in all static get*() functions. Task-number: QTBUG-54693 Change-Id: I6157dca18608e02be1ea2c2defbc31641defc9d1 Reviewed-by: Giuseppe D'Angelo Reviewed-by: David Faure --- src/widgets/dialogs/qdialog_p.h | 18 +++ src/widgets/dialogs/qinputdialog.cpp | 106 +++++++----------- .../dialogs/qinputdialog/qinputdialog.pro | 2 +- .../dialogs/qinputdialog/tst_qinputdialog.cpp | 71 ++++++++++++ 4 files changed, 131 insertions(+), 66 deletions(-) diff --git a/src/widgets/dialogs/qdialog_p.h b/src/widgets/dialogs/qdialog_p.h index 1c7c5f3c2a4..ae9e3bcc931 100644 --- a/src/widgets/dialogs/qdialog_p.h +++ b/src/widgets/dialogs/qdialog_p.h @@ -119,6 +119,24 @@ private: mutable bool m_platformHelperCreated; }; +template +class QAutoPointer { + QPointer o; + struct internal { void func() {} }; + typedef void (internal::*RestrictedBool)(); +public: + explicit QAutoPointer(T *t) Q_DECL_NOTHROW : o(t) {} + ~QAutoPointer() { delete o; } + + T *operator->() const Q_DECL_NOTHROW { return get(); } + T *get() const Q_DECL_NOTHROW { return o; } + T &operator*() const { return *get(); } + operator RestrictedBool() const Q_DECL_NOTHROW { return o ? &internal::func : Q_NULLPTR; } + bool operator!() const Q_DECL_NOTHROW { return !o; } +private: + Q_DISABLE_COPY(QAutoPointer); +}; + QT_END_NAMESPACE #endif // QDIALOG_P_H diff --git a/src/widgets/dialogs/qinputdialog.cpp b/src/widgets/dialogs/qinputdialog.cpp index d09f77ea355..4ca3923d8dc 100644 --- a/src/widgets/dialogs/qinputdialog.cpp +++ b/src/widgets/dialogs/qinputdialog.cpp @@ -1194,10 +1194,6 @@ void QInputDialog::done(int result) \snippet dialogs/standarddialogs/dialog.cpp 3 - \warning Do not delete \a parent during the execution of the dialog. If you - want to do this, you should create the dialog yourself using one of the - QInputDialog constructors. - \sa getInt(), getDouble(), getItem(), getMultiLineText() */ @@ -1205,18 +1201,18 @@ QString QInputDialog::getText(QWidget *parent, const QString &title, const QStri QLineEdit::EchoMode mode, const QString &text, bool *ok, Qt::WindowFlags flags, Qt::InputMethodHints inputMethodHints) { - QInputDialog dialog(parent, flags); - dialog.setWindowTitle(title); - dialog.setLabelText(label); - dialog.setTextValue(text); - dialog.setTextEchoMode(mode); - dialog.setInputMethodHints(inputMethodHints); + QAutoPointer dialog(new QInputDialog(parent, flags)); + dialog->setWindowTitle(title); + dialog->setLabelText(label); + dialog->setTextValue(text); + dialog->setTextEchoMode(mode); + dialog->setInputMethodHints(inputMethodHints); - int ret = dialog.exec(); + const int ret = dialog->exec(); if (ok) *ok = !!ret; if (ret) { - return dialog.textValue(); + return dialog->textValue(); } else { return QString(); } @@ -1246,10 +1242,6 @@ QString QInputDialog::getText(QWidget *parent, const QString &title, const QStri \snippet dialogs/standarddialogs/dialog.cpp 4 - \warning Do not delete \a parent during the execution of the dialog. If you - want to do this, you should create the dialog yourself using one of the - QInputDialog constructors. - \sa getInt(), getDouble(), getItem(), getText() */ @@ -1257,18 +1249,18 @@ QString QInputDialog::getMultiLineText(QWidget *parent, const QString &title, co const QString &text, bool *ok, Qt::WindowFlags flags, Qt::InputMethodHints inputMethodHints) { - QInputDialog dialog(parent, flags); - dialog.setOptions(QInputDialog::UsePlainTextEditForTextInput); - dialog.setWindowTitle(title); - dialog.setLabelText(label); - dialog.setTextValue(text); - dialog.setInputMethodHints(inputMethodHints); + QAutoPointer dialog(new QInputDialog(parent, flags)); + dialog->setOptions(QInputDialog::UsePlainTextEditForTextInput); + dialog->setWindowTitle(title); + dialog->setLabelText(label); + dialog->setTextValue(text); + dialog->setInputMethodHints(inputMethodHints); - int ret = dialog.exec(); + const int ret = dialog->exec(); if (ok) *ok = !!ret; if (ret) { - return dialog.textValue(); + return dialog->textValue(); } else { return QString(); } @@ -1298,28 +1290,24 @@ QString QInputDialog::getMultiLineText(QWidget *parent, const QString &title, co \snippet dialogs/standarddialogs/dialog.cpp 0 - \warning Do not delete \a parent during the execution of the dialog. If you - want to do this, you should create the dialog yourself using one of the - QInputDialog constructors. - \sa getText(), getDouble(), getItem(), getMultiLineText() */ int QInputDialog::getInt(QWidget *parent, const QString &title, const QString &label, int value, int min, int max, int step, bool *ok, Qt::WindowFlags flags) { - QInputDialog dialog(parent, flags); - dialog.setWindowTitle(title); - dialog.setLabelText(label); - dialog.setIntRange(min, max); - dialog.setIntValue(value); - dialog.setIntStep(step); + QAutoPointer dialog(new QInputDialog(parent, flags)); + dialog->setWindowTitle(title); + dialog->setLabelText(label); + dialog->setIntRange(min, max); + dialog->setIntValue(value); + dialog->setIntStep(step); - int ret = dialog.exec(); + const int ret = dialog->exec(); if (ok) *ok = !!ret; if (ret) { - return dialog.intValue(); + return dialog->intValue(); } else { return value; } @@ -1350,10 +1338,6 @@ int QInputDialog::getInt(QWidget *parent, const QString &title, const QString &l \snippet dialogs/standarddialogs/dialog.cpp 0 - \warning Do not delete \a parent during the execution of the dialog. If you - want to do this, you should create the dialog yourself using one of the - QInputDialog constructors. - \sa getText(), getDouble(), getItem(), getMultiLineText() */ @@ -1379,10 +1363,6 @@ int QInputDialog::getInt(QWidget *parent, const QString &title, const QString &l \snippet dialogs/standarddialogs/dialog.cpp 1 - \warning Do not delete \a parent during the execution of the dialog. If you - want to do this, you should create the dialog yourself using one of the - QInputDialog constructors. - \sa getText(), getInt(), getItem(), getMultiLineText() */ @@ -1390,18 +1370,18 @@ double QInputDialog::getDouble(QWidget *parent, const QString &title, const QStr double value, double min, double max, int decimals, bool *ok, Qt::WindowFlags flags) { - QInputDialog dialog(parent, flags); - dialog.setWindowTitle(title); - dialog.setLabelText(label); - dialog.setDoubleDecimals(decimals); - dialog.setDoubleRange(min, max); - dialog.setDoubleValue(value); + QAutoPointer dialog(new QInputDialog(parent, flags)); + dialog->setWindowTitle(title); + dialog->setLabelText(label); + dialog->setDoubleDecimals(decimals); + dialog->setDoubleRange(min, max); + dialog->setDoubleValue(value); - int ret = dialog.exec(); + const int ret = dialog->exec(); if (ok) *ok = !!ret; if (ret) { - return dialog.doubleValue(); + return dialog->doubleValue(); } else { return value; } @@ -1433,10 +1413,6 @@ double QInputDialog::getDouble(QWidget *parent, const QString &title, const QStr \snippet dialogs/standarddialogs/dialog.cpp 2 - \warning Do not delete \a parent during the execution of the dialog. If you - want to do this, you should create the dialog yourself using one of the - QInputDialog constructors. - \sa getText(), getInt(), getDouble(), getMultiLineText() */ @@ -1446,19 +1422,19 @@ QString QInputDialog::getItem(QWidget *parent, const QString &title, const QStri { QString text(items.value(current)); - QInputDialog dialog(parent, flags); - dialog.setWindowTitle(title); - dialog.setLabelText(label); - dialog.setComboBoxItems(items); - dialog.setTextValue(text); - dialog.setComboBoxEditable(editable); - dialog.setInputMethodHints(inputMethodHints); + QAutoPointer dialog(new QInputDialog(parent, flags)); + dialog->setWindowTitle(title); + dialog->setLabelText(label); + dialog->setComboBoxItems(items); + dialog->setTextValue(text); + dialog->setComboBoxEditable(editable); + dialog->setInputMethodHints(inputMethodHints); - int ret = dialog.exec(); + const int ret = dialog->exec(); if (ok) *ok = !!ret; if (ret) { - return dialog.textValue(); + return dialog->textValue(); } else { return text; } diff --git a/tests/auto/widgets/dialogs/qinputdialog/qinputdialog.pro b/tests/auto/widgets/dialogs/qinputdialog/qinputdialog.pro index cc479812a8a..9cb14c53501 100644 --- a/tests/auto/widgets/dialogs/qinputdialog/qinputdialog.pro +++ b/tests/auto/widgets/dialogs/qinputdialog/qinputdialog.pro @@ -1,4 +1,4 @@ CONFIG += testcase TARGET = tst_qinputdialog -QT += widgets testlib +QT += widgets-private testlib SOURCES += tst_qinputdialog.cpp diff --git a/tests/auto/widgets/dialogs/qinputdialog/tst_qinputdialog.cpp b/tests/auto/widgets/dialogs/qinputdialog/tst_qinputdialog.cpp index bbb68832381..0ea9e0259fa 100644 --- a/tests/auto/widgets/dialogs/qinputdialog/tst_qinputdialog.cpp +++ b/tests/auto/widgets/dialogs/qinputdialog/tst_qinputdialog.cpp @@ -35,6 +35,7 @@ #include #include #include +#include class tst_QInputDialog : public QObject { @@ -52,6 +53,7 @@ private slots: void getInt(); void getDouble_data(); void getDouble(); + void taskQTBUG_54693_crashWhenParentIsDeletedWhileDialogIsOpen(); void task255502getDouble(); void getText_data(); void getText(); @@ -311,6 +313,75 @@ void tst_QInputDialog::getDouble() delete parent; } +namespace { +class SelfDestructParent : public QWidget +{ + Q_OBJECT +public: + explicit SelfDestructParent(int delay = 100) + : QWidget(Q_NULLPTR) + { + QTimer::singleShot(delay, this, SLOT(deleteLater())); + } +}; +} + +void tst_QInputDialog::taskQTBUG_54693_crashWhenParentIsDeletedWhileDialogIsOpen() +{ + // getText + { + QAutoPointer dialog(new SelfDestructParent); + bool ok = true; + const QString result = QInputDialog::getText(dialog.get(), "Title", "Label", QLineEdit::Normal, "Text", &ok); + QVERIFY(!dialog); + QVERIFY(!ok); + QVERIFY(result.isNull()); + } + + // getMultiLineText + { + QAutoPointer dialog(new SelfDestructParent); + bool ok = true; + const QString result = QInputDialog::getMultiLineText(dialog.get(), "Title", "Label", "Text", &ok); + QVERIFY(!dialog); + QVERIFY(!ok); + QVERIFY(result.isNull()); + } + + // getItem + for (int editable = false; editable <= true; ++editable) { + QAutoPointer dialog(new SelfDestructParent); + bool ok = true; + const QString result = QInputDialog::getItem(dialog.get(), "Title", "Label", + QStringList() << "1" << "2", 1, editable, &ok); + QVERIFY(!dialog); + QVERIFY(!ok); + QCOMPARE(result, QLatin1String("2")); + } + + // getInt + { + const int initial = 7; + QAutoPointer dialog(new SelfDestructParent); + bool ok = true; + const int result = QInputDialog::getInt(dialog.get(), "Title", "Label", initial, -10, +10, 1, &ok); + QVERIFY(!dialog); + QVERIFY(!ok); + QCOMPARE(result, initial); + } + + // getDouble + { + const double initial = 7; + QAutoPointer dialog(new SelfDestructParent); + bool ok = true; + const double result = QInputDialog::getDouble(dialog.get(), "Title", "Label", initial, -10, +10, 2, &ok); + QVERIFY(!dialog); + QVERIFY(!ok); + QCOMPARE(result, initial); + } +} + void tst_QInputDialog::task255502getDouble() { parent = new QWidget;