From 746c1b611d39aeaa3b1d7217848a9e1abc22db63 Mon Sep 17 00:00:00 2001 From: Tatiana Borisova Date: Tue, 27 Aug 2024 16:55:22 +0200 Subject: [PATCH] QDialogs: 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. 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 (cherry picked from commit 81d55f772892445686862e49d0a6092668f8c725) (cherry picked from commit ca3fa76a4e769b403975ce9f0b4b83792b9e074d) Reviewed-by: Qt Cherry-pick Bot --- src/widgets/dialogs/qcolordialog.cpp | 18 +++-- src/widgets/dialogs/qfiledialog.cpp | 66 ++++++++----------- src/widgets/dialogs/qfontdialog.cpp | 22 ++----- src/widgets/dialogs/qinputdialog.cpp | 10 +-- .../dialogs/qcolordialog/tst_qcolordialog.cpp | 13 ++++ .../dialogs/qfiledialog2/tst_qfiledialog2.cpp | 49 ++++++++++++++ .../dialogs/qfontdialog/tst_qfontdialog.cpp | 32 +++++++++ 7 files changed, 145 insertions(+), 65 deletions(-) diff --git a/src/widgets/dialogs/qcolordialog.cpp b/src/widgets/dialogs/qcolordialog.cpp index 27315fe53cf..3636ceb537d 100644 --- 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 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 index 22e6d44e6f6..1bcd89494ca 100644 --- 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 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 QFileDialog::getOpenFileUrls(QWidget *parent, args.mode = ExistingFiles; args.options = options; - QFileDialog dialog(args); - dialog.setSupportedSchemes(supportedSchemes); + QAutoPointer 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(); } @@ -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 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 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 index afc46e75068..36bcd62a270 100644 --- 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 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 index 6d1a0b2873b..480e5bb891b 100644 --- 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 index 5ae8eaf30d3..95000906bed 100644 --- 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 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("colorString"); diff --git a/tests/auto/widgets/dialogs/qfiledialog2/tst_qfiledialog2.cpp b/tests/auto/widgets/dialogs/qfiledialog2/tst_qfiledialog2.cpp index 2af896511a7..1f971903a2f 100644 --- a/tests/auto/widgets/dialogs/qfiledialog2/tst_qfiledialog2.cpp +++ b/tests/auto/widgets/dialogs/qfiledialog2/tst_qfiledialog2.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #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 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 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 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 mainWindow = new QWidget(); + QTimer::singleShot(1000, mainWindow, [mainWindow] + { if (mainWindow.get()) mainWindow->deleteLater(); }); + const QList 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 index 01f3e7ec955..76869c3e87e 100644 --- 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 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 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 standardSizesList = QFontDatabase::standardSizes();