summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorKirill Burtsev <kirill.burtsev@qt.io>2019-02-11 19:21:03 +0100
committerKirill Burtsev <kirill.burtsev@qt.io>2019-02-19 15:30:44 +0000
commit7537526093c92e89672d1e952a9baceecaa91730 (patch)
treedcb37c57802b90eff5835f54f13ef6a8db059ac7
parent755f7e414583c5458c2d421d047a1c7890c8d8d2 (diff)
Remove download properly on profile destruction to avoid use after free
In the Widgets API, download items are children of the profile and are destroyed when the parent profile destroys its children. The download item's destructor can therefore not access the profile, as it would cause a heap-use-after-free crashes. On quick side turn ongoing downloads cleanup to match widgets one. Fixes: QTBUG-73839 Change-Id: Iabb379e91187e3e68ebcd4693fec35883b72b1f2 Reviewed-by: Michael Brüning <michael.bruning@qt.io> Reviewed-by: Jüri Valdmann <juri.valdmann@qt.io>
-rw-r--r--src/webengine/api/qquickwebenginedownloaditem.cpp2
-rw-r--r--src/webengine/api/qquickwebengineprofile.cpp28
-rw-r--r--src/webengine/api/qquickwebengineprofile_p.h4
-rw-r--r--src/webenginewidgets/api/qwebenginedownloaditem.cpp4
-rw-r--r--src/webenginewidgets/api/qwebengineprofile.cpp30
-rw-r--r--src/webenginewidgets/api/qwebengineprofile_p.h2
-rw-r--r--tests/auto/widgets/qwebenginedownloaditem/tst_qwebenginedownloaditem.cpp32
7 files changed, 77 insertions, 25 deletions
diff --git a/src/webengine/api/qquickwebenginedownloaditem.cpp b/src/webengine/api/qquickwebenginedownloaditem.cpp
index 7d138287..981d1163 100644
--- a/src/webengine/api/qquickwebenginedownloaditem.cpp
+++ b/src/webengine/api/qquickwebenginedownloaditem.cpp
@@ -629,8 +629,6 @@ QQuickWebEngineDownloadItem::~QQuickWebEngineDownloadItem()
{
if (!isFinished())
cancel();
- if (d_ptr->profile)
- d_ptr->profile->d_ptr->profileAdapter()->removeDownload(d_ptr->downloadId);
}
QT_END_NAMESPACE
diff --git a/src/webengine/api/qquickwebengineprofile.cpp b/src/webengine/api/qquickwebengineprofile.cpp
index ddc71602..26fcf28f 100644
--- a/src/webengine/api/qquickwebengineprofile.cpp
+++ b/src/webengine/api/qquickwebengineprofile.cpp
@@ -175,13 +175,6 @@ QQuickWebEngineProfilePrivate::~QQuickWebEngineProfilePrivate()
m_profileAdapter->removeClient(this);
}
- for (QQuickWebEngineDownloadItem *download : qAsConst(m_ongoingDownloads)) {
- if (download)
- download->cancel();
- }
-
- m_ongoingDownloads.clear();
-
if (m_profileAdapter != QtWebEngineCore::ProfileAdapter::defaultProfileAdapter())
delete m_profileAdapter;
}
@@ -215,6 +208,23 @@ void QQuickWebEngineProfilePrivate::cancelDownload(quint32 downloadId)
void QQuickWebEngineProfilePrivate::downloadDestroyed(quint32 downloadId)
{
m_ongoingDownloads.remove(downloadId);
+ if (m_profileAdapter)
+ m_profileAdapter->removeDownload(downloadId);
+}
+
+void QQuickWebEngineProfilePrivate::cleanDownloads()
+{
+ for (auto download : m_ongoingDownloads.values()) {
+ if (!download)
+ continue;
+
+ if (!download->isFinished())
+ download->cancel();
+
+ if (m_profileAdapter)
+ m_profileAdapter->removeDownload(download->id());
+ }
+ m_ongoingDownloads.clear();
}
void QQuickWebEngineProfilePrivate::downloadRequested(DownloadItemInfo &info)
@@ -239,6 +249,7 @@ void QQuickWebEngineProfilePrivate::downloadRequested(DownloadItemInfo &info)
QQuickWebEngineDownloadItem *download = new QQuickWebEngineDownloadItem(itemPrivate, q);
m_ongoingDownloads.insert(info.id, download);
+ QObject::connect(download, &QQuickWebEngineDownloadItem::destroyed, q, [id = info.id, this] () { downloadDestroyed(id); });
QQmlEngine::setObjectOwnership(download, QQmlEngine::JavaScriptOwnership);
Q_EMIT q->downloadRequested(download);
@@ -252,7 +263,6 @@ void QQuickWebEngineProfilePrivate::downloadRequested(DownloadItemInfo &info)
if (state == QQuickWebEngineDownloadItem::DownloadRequested) {
// Delete unaccepted downloads.
info.accepted = false;
- m_ongoingDownloads.remove(info.id);
delete download;
}
}
@@ -275,7 +285,6 @@ void QQuickWebEngineProfilePrivate::downloadUpdated(const DownloadItemInfo &info
if (info.state != ProfileAdapterClient::DownloadInProgress) {
Q_EMIT q->downloadFinished(download);
- m_ongoingDownloads.remove(info.id);
}
}
@@ -380,6 +389,7 @@ QQuickWebEngineProfile::QQuickWebEngineProfile(QQuickWebEngineProfilePrivate *pr
*/
QQuickWebEngineProfile::~QQuickWebEngineProfile()
{
+ d_ptr->cleanDownloads();
}
/*!
diff --git a/src/webengine/api/qquickwebengineprofile_p.h b/src/webengine/api/qquickwebengineprofile_p.h
index d31ded0e..2b1a5b13 100644
--- a/src/webengine/api/qquickwebengineprofile_p.h
+++ b/src/webengine/api/qquickwebengineprofile_p.h
@@ -53,7 +53,7 @@
#include "profile_adapter_client.h"
#include "profile_adapter.h"
-#include "qquickwebengineprofile_p.h"
+#include "qquickwebengineprofile.h"
#include <QExplicitlySharedDataPointer>
#include <QMap>
@@ -80,6 +80,8 @@ public:
void cancelDownload(quint32 downloadId);
void downloadDestroyed(quint32 downloadId);
+ void cleanDownloads();
+
void downloadRequested(DownloadItemInfo &info) override;
void downloadUpdated(const DownloadItemInfo &info) override;
diff --git a/src/webenginewidgets/api/qwebenginedownloaditem.cpp b/src/webenginewidgets/api/qwebenginedownloaditem.cpp
index f0f0958a..deb92bfd 100644
--- a/src/webenginewidgets/api/qwebenginedownloaditem.cpp
+++ b/src/webenginewidgets/api/qwebenginedownloaditem.cpp
@@ -660,10 +660,10 @@ QWebEngineDownloadItem::QWebEngineDownloadItem(QWebEngineDownloadItemPrivate *p,
*/
QWebEngineDownloadItem::~QWebEngineDownloadItem()
{
+ // MEMO Items are owned by profile by default and will be destroyed on profile's destruction
+ // It's not safe to access profile in that case, so we rely on profile to clean up items
if (!isFinished())
cancel();
- if (auto profileAdapter = d_ptr->profile->profileAdapter())
- profileAdapter->removeDownload(d_ptr->downloadId);
}
QT_END_NAMESPACE
diff --git a/src/webenginewidgets/api/qwebengineprofile.cpp b/src/webenginewidgets/api/qwebengineprofile.cpp
index 03ce5e0b..0d12fdae 100644
--- a/src/webenginewidgets/api/qwebengineprofile.cpp
+++ b/src/webenginewidgets/api/qwebengineprofile.cpp
@@ -175,13 +175,6 @@ QWebEngineProfilePrivate::~QWebEngineProfilePrivate()
m_profileAdapter->removeClient(this);
}
- for (QWebEngineDownloadItem *download : qAsConst(m_ongoingDownloads)) {
- if (download)
- download->cancel();
- }
-
- m_ongoingDownloads.clear();
-
if (m_profileAdapter != QtWebEngineCore::ProfileAdapter::defaultProfileAdapter())
delete m_profileAdapter;
@@ -196,6 +189,23 @@ ProfileAdapter* QWebEngineProfilePrivate::profileAdapter() const
void QWebEngineProfilePrivate::downloadDestroyed(quint32 downloadId)
{
m_ongoingDownloads.remove(downloadId);
+ if (m_profileAdapter)
+ m_profileAdapter->removeDownload(downloadId);
+}
+
+void QWebEngineProfilePrivate::cleanDownloads()
+{
+ for (auto download : m_ongoingDownloads.values()) {
+ if (!download)
+ continue;
+
+ if (!download->isFinished())
+ download->cancel();
+
+ if (m_profileAdapter)
+ m_profileAdapter->removeDownload(download->id());
+ }
+ m_ongoingDownloads.clear();
}
void QWebEngineProfilePrivate::downloadRequested(DownloadItemInfo &info)
@@ -219,6 +229,7 @@ void QWebEngineProfilePrivate::downloadRequested(DownloadItemInfo &info)
QWebEngineDownloadItem *download = new QWebEngineDownloadItem(itemPrivate, q);
m_ongoingDownloads.insert(info.id, download);
+ QObject::connect(download, &QWebEngineDownloadItem::destroyed, q, [id = info.id, this] () { downloadDestroyed(id); });
Q_EMIT q->downloadRequested(download);
@@ -232,7 +243,6 @@ void QWebEngineProfilePrivate::downloadRequested(DownloadItemInfo &info)
if (state == QWebEngineDownloadItem::DownloadRequested) {
// Delete unaccepted downloads.
info.accepted = false;
- m_ongoingDownloads.remove(info.id);
delete download;
}
}
@@ -250,9 +260,6 @@ void QWebEngineProfilePrivate::downloadUpdated(const DownloadItemInfo &info)
}
download->d_func()->update(info);
-
- if (download->isFinished())
- m_ongoingDownloads.remove(info.id);
}
/*!
@@ -301,6 +308,7 @@ QWebEngineProfile::QWebEngineProfile(QWebEngineProfilePrivate *privatePtr, QObje
*/
QWebEngineProfile::~QWebEngineProfile()
{
+ d_ptr->cleanDownloads();
}
/*!
diff --git a/src/webenginewidgets/api/qwebengineprofile_p.h b/src/webenginewidgets/api/qwebengineprofile_p.h
index 9ff8df84..4a76f457 100644
--- a/src/webenginewidgets/api/qwebengineprofile_p.h
+++ b/src/webenginewidgets/api/qwebengineprofile_p.h
@@ -81,6 +81,8 @@ public:
void downloadDestroyed(quint32 downloadId);
+ void cleanDownloads();
+
void downloadRequested(DownloadItemInfo &info) override;
void downloadUpdated(const DownloadItemInfo &info) override;
diff --git a/tests/auto/widgets/qwebenginedownloaditem/tst_qwebenginedownloaditem.cpp b/tests/auto/widgets/qwebenginedownloaditem/tst_qwebenginedownloaditem.cpp
index 2af81892..b30fc725 100644
--- a/tests/auto/widgets/qwebenginedownloaditem/tst_qwebenginedownloaditem.cpp
+++ b/tests/auto/widgets/qwebenginedownloaditem/tst_qwebenginedownloaditem.cpp
@@ -69,6 +69,7 @@ private Q_SLOTS:
void downloadFileNot1();
void downloadFileNot2();
void downloadDeleted();
+ void downloadDeletedByProfile();
private:
void saveLink(QPoint linkPos);
@@ -812,5 +813,36 @@ void tst_QWebEngineDownloadItem::downloadDeleted()
QTRY_COMPARE(finishedCount, 1);
}
+void tst_QWebEngineDownloadItem::downloadDeletedByProfile()
+{
+ m_server->setExpectError(true);
+
+ QPointer<QWebEngineProfile> profile(new QWebEngineProfile);
+ profile->setHttpCacheType(QWebEngineProfile::NoCache);
+ profile->settings()->setAttribute(QWebEngineSettings::AutoLoadIconsForPage, false);
+
+ bool downloadFinished = false;
+ QPointer<QWebEngineDownloadItem> downloadItem;
+ connect(profile, &QWebEngineProfile::downloadRequested, [&] (QWebEngineDownloadItem *item) {
+ connect(item, &QWebEngineDownloadItem::finished, [&] () {
+ downloadFinished = true;
+ });
+ downloadItem = item;
+ item->accept();
+ });
+
+ QPointer<QWebEnginePage> page(new QWebEnginePage(profile));
+ page->download(m_server->url(QByteArrayLiteral("/file")));
+
+ QTRY_COMPARE(downloadItem.isNull(), false);
+ QVERIFY(downloadItem);
+
+ page->deleteLater();
+ profile->deleteLater();
+
+ QTRY_COMPARE(downloadFinished, true);
+ QTRY_COMPARE(downloadItem.isNull(), true);
+}
+
QTEST_MAIN(tst_QWebEngineDownloadItem)
#include "tst_qwebenginedownloaditem.moc"