QDialogs: prevent crash in static get*() functions when parent gets d…

…eleted - 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. Long time ago instead of using manual resource management, QAutoPointer was added to fix the crash problem for QInputDialog. The same fix now is being applied for the rest of QDialog classes: QColorDialog, QFileDialog, QFontDialog. Documentation warnings about deleting the parent are not actual anymore. Deleted. Pick-to: 6.5 Task-number: QTBUG-54693 Change-Id: Ib7cc0575ea25f392a295538e21de9015dc49ebe4 Reviewed-by: Ivan Solovev <[email protected]> (cherry picked from commit 81d55f7) (cherry picked from commit ca3fa76) Reviewed-by: Qt Cherry-pick Bot <[email protected]>
qt · Sep 17, 2024 · 746c1b6 · 746c1b6
1 parent d545580
commit 746c1b6
Show file tree

Hide file tree

Showing 7 changed files with 145 additions and 65 deletions.
diff --git a/src/widgets/dialogs/qcolordialog.cpp b/src/widgets/dialogs/qcolordialog.cpp
@@ -2211,13 +2211,19 @@ void QColorDialog::open(QObject *receiver, const char *member)
 QColor QColorDialog::getColor(const QColor &initial, QWidget *parent, const QString &title,
                               ColorDialogOptions options)
 {
-    QColorDialog dlg(parent);
+    QAutoPointer<QColorDialog> dlg(new QColorDialog(parent));
     if (!title.isEmpty())
-        dlg.setWindowTitle(title);
-    dlg.setOptions(options);
-    dlg.setCurrentColor(initial);
-    dlg.exec();
-    return dlg.selectedColor();
+        dlg->setWindowTitle(title);
+    dlg->setOptions(options);
+    dlg->setCurrentColor(initial);
+
+    // If the dlg was deleted with a parent window,
+    // dlg == nullptr after leaving the exec().
+    dlg->exec();
+    if (bool(dlg))
+        return dlg->selectedColor();
+    else
+        return QColor();
 }
 
 /*!

diff --git a/src/widgets/dialogs/qfiledialog.cpp b/src/widgets/dialogs/qfiledialog.cpp
@@ -2093,10 +2093,6 @@ QString QFileDialog::labelText(DialogLabel label) const
     \a options includes DontResolveSymlinks, the file dialog treats
     symlinks as regular directories.
 
-    \warning Do not delete \a parent during the execution of the dialog. If you
-    want to do this, you must create the dialog yourself using one of the
-    QFileDialog constructors.
-
     \sa getOpenFileNames(), getSaveFileName(), getExistingDirectory()
 */
 QString QFileDialog::getOpenFileName(QWidget *parent,
@@ -2157,14 +2153,15 @@ QUrl QFileDialog::getOpenFileUrl(QWidget *parent,
     args.mode = ExistingFile;
     args.options = options;
 
-    QFileDialog dialog(args);
-    dialog.setSupportedSchemes(supportedSchemes);
+    QAutoPointer<QFileDialog> dialog(new QFileDialog(args));
+    dialog->setSupportedSchemes(supportedSchemes);
     if (selectedFilter && !selectedFilter->isEmpty())
-        dialog.selectNameFilter(*selectedFilter);
-    if (dialog.exec() == QDialog::Accepted) {
+        dialog->selectNameFilter(*selectedFilter);
+    const int execResult = dialog->exec();
+    if (bool(dialog) && execResult == QDialog::Accepted) {
         if (selectedFilter)
-            *selectedFilter = dialog.selectedNameFilter();
-        return dialog.selectedUrls().value(0);
+            *selectedFilter = dialog->selectedNameFilter();
+        return dialog->selectedUrls().value(0);
     }
     return QUrl();
 }
@@ -2206,10 +2203,6 @@ QUrl QFileDialog::getOpenFileUrl(QWidget *parent,
     see the QFileDialog::Option enum for more information on the flags you can
     pass.
 
-    \warning Do not delete \a parent during the execution of the dialog. If you
-    want to do this, you must create the dialog yourself using one of the
-    QFileDialog constructors.
-
     \sa getOpenFileName(), getSaveFileName(), getExistingDirectory()
 */
 QStringList QFileDialog::getOpenFileNames(QWidget *parent,
@@ -2272,14 +2265,15 @@ QList<QUrl> QFileDialog::getOpenFileUrls(QWidget *parent,
     args.mode = ExistingFiles;
     args.options = options;
 
-    QFileDialog dialog(args);
-    dialog.setSupportedSchemes(supportedSchemes);
+    QAutoPointer<QFileDialog> dialog(new QFileDialog(args));
+    dialog->setSupportedSchemes(supportedSchemes);
     if (selectedFilter && !selectedFilter->isEmpty())
-        dialog.selectNameFilter(*selectedFilter);
-    if (dialog.exec() == QDialog::Accepted) {
+        dialog->selectNameFilter(*selectedFilter);
+    const int execResult = dialog->exec();
+    if (bool(dialog) && execResult == QDialog::Accepted) {
         if (selectedFilter)
-            *selectedFilter = dialog.selectedNameFilter();
-        return dialog.selectedUrls();
+            *selectedFilter = dialog->selectedNameFilter();
+        return dialog->selectedUrls();
     }
     return QList<QUrl>();
 }
@@ -2441,10 +2435,6 @@ void QFileDialog::saveFileContent(const QByteArray &fileContent, const QString &
     \a options includes DontResolveSymlinks, the file dialog treats symlinks
     as regular directories.
 
-    \warning Do not delete \a parent during the execution of the dialog. If you
-    want to do this, you must create the dialog yourself using one of the
-    QFileDialog constructors.
-
     \sa getOpenFileName(), getOpenFileNames(), getExistingDirectory()
 */
 QString QFileDialog::getSaveFileName(QWidget *parent,
@@ -2505,15 +2495,16 @@ QUrl QFileDialog::getSaveFileUrl(QWidget *parent,
     args.mode = AnyFile;
     args.options = options;
 
-    QFileDialog dialog(args);
-    dialog.setSupportedSchemes(supportedSchemes);
-    dialog.setAcceptMode(AcceptSave);
+    QAutoPointer<QFileDialog> dialog(new QFileDialog(args));
+    dialog->setSupportedSchemes(supportedSchemes);
+    dialog->setAcceptMode(AcceptSave);
     if (selectedFilter && !selectedFilter->isEmpty())
-        dialog.selectNameFilter(*selectedFilter);
-    if (dialog.exec() == QDialog::Accepted) {
+        dialog->selectNameFilter(*selectedFilter);
+    const int execResult = dialog->exec();
+    if (bool(dialog) && execResult == QDialog::Accepted) {
         if (selectedFilter)
-            *selectedFilter = dialog.selectedNameFilter();
-        return dialog.selectedUrls().value(0);
+            *selectedFilter = dialog->selectedNameFilter();
+        return dialog->selectedUrls().value(0);
     }
     return QUrl();
 }
@@ -2556,10 +2547,6 @@ QUrl QFileDialog::getSaveFileUrl(QWidget *parent,
     dispatch any QTimers, and if \a parent is not \nullptr then it positions
     the dialog just below the parent's title bar.
 
-    \warning Do not delete \a parent during the execution of the dialog. If you
-    want to do this, you must create the dialog yourself using one of the
-    QFileDialog constructors.
-
     \sa getOpenFileName(), getOpenFileNames(), getSaveFileName()
 */
 QString QFileDialog::getExistingDirectory(QWidget *parent,
@@ -2615,10 +2602,11 @@ QUrl QFileDialog::getExistingDirectoryUrl(QWidget *parent,
     args.mode = Directory;
     args.options = options;
 
-    QFileDialog dialog(args);
-    dialog.setSupportedSchemes(supportedSchemes);
-    if (dialog.exec() == QDialog::Accepted)
-        return dialog.selectedUrls().value(0);
+    QAutoPointer<QFileDialog> dialog(new QFileDialog(args));
+    dialog->setSupportedSchemes(supportedSchemes);
+    const int execResult = dialog->exec();
+    if (bool(dialog) && execResult == QDialog::Accepted)
+        return dialog->selectedUrls().value(0);
     return QUrl();
 }
 

diff --git a/src/widgets/dialogs/qfontdialog.cpp b/src/widgets/dialogs/qfontdialog.cpp
@@ -330,10 +330,6 @@ QFontDialog::~QFontDialog()
   \snippet code/src_gui_dialogs_qfontdialog.cpp 3
   In this example, if the user clicks OK the font they chose will be
   used, and if they click Cancel the original font is used.
-
-  \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 QFontDialog constructors.
 */
 QFont QFontDialog::getFont(bool *ok, const QFont &initial, QWidget *parent, const QString &title,
                            FontDialogOptions options)
@@ -356,10 +352,6 @@ QFont QFontDialog::getFont(bool *ok, const QFont &initial, QWidget *parent, cons
 
   Example:
   \snippet code/src_gui_dialogs_qfontdialog.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 QFontDialog constructors.
 */
 QFont QFontDialog::getFont(bool *ok, QWidget *parent)
 {
@@ -370,17 +362,17 @@ QFont QFontDialog::getFont(bool *ok, QWidget *parent)
 QFont QFontDialogPrivate::getFont(bool *ok, const QFont &initial, QWidget *parent,
                                   const QString &title, QFontDialog::FontDialogOptions options)
 {
-    QFontDialog dlg(parent);
-    dlg.setOptions(options);
-    dlg.setCurrentFont(initial);
+    QAutoPointer<QFontDialog> dlg(new QFontDialog(parent));
+    dlg->setOptions(options);
+    dlg->setCurrentFont(initial);
     if (!title.isEmpty())
-        dlg.setWindowTitle(title);
+        dlg->setWindowTitle(title);
 
-    int ret = (dlg.exec() || (options & QFontDialog::NoButtons));
+    int ret = (dlg->exec() || (options & QFontDialog::NoButtons));
     if (ok)
         *ok = !!ret;
-    if (ret) {
-        return dlg.selectedFont();
+    if (ret && bool(dlg)) {
+        return dlg->selectedFont();
     } else {
         return initial;
     }

diff --git a/src/widgets/dialogs/qinputdialog.cpp b/src/widgets/dialogs/qinputdialog.cpp
@@ -1184,7 +1184,7 @@ QString QInputDialog::getText(QWidget *parent, const QString &title, const QStri
     const int ret = dialog->exec();
     if (ok)
         *ok = !!ret;
-    if (ret) {
+    if (bool(dialog) && ret) {
         return dialog->textValue();
     } else {
         return QString();
@@ -1232,7 +1232,7 @@ QString QInputDialog::getMultiLineText(QWidget *parent, const QString &title, co
     const int ret = dialog->exec();
     if (ok)
         *ok = !!ret;
-    if (ret) {
+    if (bool(dialog) && ret) {
         return dialog->textValue();
     } else {
         return QString();
@@ -1279,7 +1279,7 @@ int QInputDialog::getInt(QWidget *parent, const QString &title, const QString &l
     const int ret = dialog->exec();
     if (ok)
         *ok = !!ret;
-    if (ret) {
+    if (bool(dialog) && ret) {
         return dialog->intValue();
     } else {
         return value;
@@ -1328,7 +1328,7 @@ double QInputDialog::getDouble(QWidget *parent, const QString &title, const QStr
     const int ret = dialog->exec();
     if (ok)
         *ok = !!ret;
-    if (ret) {
+    if (bool(dialog) && ret) {
         return dialog->doubleValue();
     } else {
         return value;
@@ -1381,7 +1381,7 @@ QString QInputDialog::getItem(QWidget *parent, const QString &title, const QStri
     const int ret = dialog->exec();
     if (ok)
         *ok = !!ret;
-    if (ret) {
+    if (bool(dialog) && ret) {
         return dialog->textValue();
     } else {
         return text;

diff --git a/tests/auto/widgets/dialogs/qcolordialog/tst_qcolordialog.cpp b/tests/auto/widgets/dialogs/qcolordialog/tst_qcolordialog.cpp
@@ -27,6 +27,7 @@ private slots:
     void native_activeModalWidget();
     void task247349_alpha();
     void QTBUG_43548_initialColor();
+    void noCrashWhenParentIsDeleted();
     void hexColor_data();
     void hexColor();
 
@@ -132,6 +133,18 @@ void tst_QColorDialog::QTBUG_43548_initialColor()
     QCOMPARE(a, dialog.currentColor());
 }
 
+void tst_QColorDialog::noCrashWhenParentIsDeleted()
+{
+    QPointer<QWidget> mainWindow = new QWidget();
+    QTimer::singleShot(1000, mainWindow, [mainWindow]
+                       { if (mainWindow.get()) mainWindow->deleteLater(); });
+    const QColor color = QColorDialog::getColor(QColor::fromRgba(0xffffffff), mainWindow.get(),
+                                                QString(), QColorDialog::DontUseNativeDialog);
+
+    QVERIFY(!color.isValid());
+    QVERIFY(!mainWindow.get());
+}
+
 void tst_QColorDialog::hexColor_data()
 {
     QTest::addColumn<const QString>("colorString");

diff --git a/tests/auto/widgets/dialogs/qfiledialog2/tst_qfiledialog2.cpp b/tests/auto/widgets/dialogs/qfiledialog2/tst_qfiledialog2.cpp
@@ -25,6 +25,7 @@
 #include <qlayout.h>
 #include <qmenu.h>
 #include <qrandom.h>
+#include <qpointer.h>
 #include "../../../../../src/widgets/dialogs/qsidebar_p.h"
 #include "../../../../../src/gui/itemmodels/qfilesystemmodel_p.h"
 #include "../../../../../src/widgets/dialogs/qfiledialog_p.h"
@@ -104,6 +105,7 @@ private slots:
     void QTBUG4419_lineEditSelectAll();
     void QTBUG6558_showDirsOnly();
     void QTBUG4842_selectFilterWithHideNameFilterDetails();
+    void noCrashWhenParentIsDeleted();
     void dontShowCompleterOnRoot();
     void nameFilterParsing_data();
     void nameFilterParsing();
@@ -1337,6 +1339,53 @@ void tst_QFileDialog2::QTBUG4842_selectFilterWithHideNameFilterDetails()
 
 }
 
+void tst_QFileDialog2::noCrashWhenParentIsDeleted()
+{
+    {
+        QPointer<QWidget> mainWindow = new QWidget();
+        QTimer::singleShot(1000, mainWindow, [mainWindow]
+                           { if (mainWindow.get()) mainWindow->deleteLater(); });
+        const QUrl url = QFileDialog::getOpenFileUrl(mainWindow.get(),
+                                                     QStringLiteral("getOpenFileUrl"));
+        QVERIFY(url.isEmpty());
+        QVERIFY(!url.isValid());
+        QVERIFY(!mainWindow.get());
+    }
+
+    {
+        QPointer<QWidget> mainWindow = new QWidget();
+        QTimer::singleShot(1000, mainWindow, [mainWindow]
+                           { if (mainWindow.get()) mainWindow->deleteLater(); });
+        const QUrl url = QFileDialog::getSaveFileUrl(mainWindow.get(),
+                                                     QStringLiteral("getSaveFileUrl"));
+        QVERIFY(url.isEmpty());
+        QVERIFY(!url.isValid());
+        QVERIFY(!mainWindow.get());
+    }
+
+    {
+        QPointer<QWidget> mainWindow = new QWidget();
+        QTimer::singleShot(1000, mainWindow, [mainWindow]
+                           { if (mainWindow.get()) mainWindow->deleteLater(); });
+        const QUrl url
+                = QFileDialog::getExistingDirectoryUrl(mainWindow.get(),
+                                                       QStringLiteral("getExistingDirectoryUrl"));
+        QVERIFY(url.isEmpty());
+        QVERIFY(!url.isValid());
+        QVERIFY(!mainWindow.get());
+    }
+
+    {
+        QPointer<QWidget> mainWindow = new QWidget();
+        QTimer::singleShot(1000, mainWindow, [mainWindow]
+                           { if (mainWindow.get()) mainWindow->deleteLater(); });
+        const QList<QUrl> url = QFileDialog::getOpenFileUrls(mainWindow.get(),
+                                                      QStringLiteral("getOpenFileUrls"));
+        QVERIFY(url.isEmpty());
+        QVERIFY(!mainWindow.get());
+    }
+}
+
 void tst_QFileDialog2::dontShowCompleterOnRoot()
 {
     if (!QGuiApplicationPrivate::platformIntegration()->hasCapability(QPlatformIntegration::WindowActivation))

diff --git a/tests/auto/widgets/dialogs/qfontdialog/tst_qfontdialog.cpp b/tests/auto/widgets/dialogs/qfontdialog/tst_qfontdialog.cpp
@@ -44,6 +44,7 @@ private slots:
 #ifndef QT_NO_STYLE_STYLESHEET
     void qtbug_41513_stylesheetStyle();
 #endif
+    void noCrashWhenParentIsDeleted();
 
     void hideNativeByDestruction();
 
@@ -211,6 +212,37 @@ void tst_QFontDialog::qtbug_41513_stylesheetStyle()
 }
 #endif // QT_NO_STYLE_STYLESHEET
 
+void tst_QFontDialog::noCrashWhenParentIsDeleted()
+{
+    {
+        QPointer<QWidget> mainWindow = new QWidget();
+        QTimer::singleShot(1000, mainWindow, [mainWindow]
+                           { if (mainWindow.get()) mainWindow->deleteLater(); });
+        bool accepted = false;
+        const QFont testFont = QFont(QStringLiteral("QtsSpecialTestFont1"));
+        QFontDialog::getFont(&accepted, testFont,
+                             mainWindow.get(),
+                             QLatin1String("QFontDialog - crash parent test"),
+                             QFontDialog::DontUseNativeDialog);
+        QVERIFY(!accepted);
+        QVERIFY(!mainWindow.get());
+    }
+
+    {
+        QPointer<QWidget> mainWindow = new QWidget();
+        QTimer::singleShot(1000, mainWindow, [mainWindow]
+                           { if (mainWindow.get()) mainWindow->deleteLater(); });
+        bool accepted = false;
+        const QFont testFont = QFont(QStringLiteral("QtsSpecialTestFont2"));
+        QFontDialog::getFont(&accepted, testFont,
+                             mainWindow.get(),
+                             QLatin1String("QFontDialog - crash parent test"),
+                             QFontDialog::NoButtons | QFontDialog::DontUseNativeDialog);
+        QVERIFY(accepted);
+        QVERIFY(!mainWindow.get());
+    }
+}
+
 void tst_QFontDialog::testNonStandardFontSize()
 {
     QList<int> standardSizesList = QFontDatabase::standardSizes();