From 9d854e778d8d6ef8e03e1ea74fe958675b24fd45 Mon Sep 17 00:00:00 2001
From: Nicolas Fella <nicolas.fella@gmx.de>
Date: Sun, 27 Dec 2020 21:24:06 +0100
Subject: Fix use-after-free of QNetworkReply in BaseJob

Usually QNetworkAccessManager expects the user to delete the replies, but when the QNetworkAccessManager itself is deleted it deletes all pending replies (https://code.woboq.org/qt5/qtbase/src/network/access/qnetworkaccessmanager.cpp.html#529).

This can lead to use-after-free crashes when d->reply is accessed. By putting the reply into a QPointer the exiting if(d->reply) checks can work properly.
---
 lib/jobs/basejob.cpp | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

(limited to 'lib/jobs')

diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp
index 3fa1cd94..2ac942f5 100644
--- a/lib/jobs/basejob.cpp
+++ b/lib/jobs/basejob.cpp
@@ -24,6 +24,7 @@
 #include <QtCore/QTimer>
 #include <QtCore/QStringBuilder>
 #include <QtCore/QMetaEnum>
+#include <QtCore/QPointer>
 #include <QtNetwork/QNetworkAccessManager>
 #include <QtNetwork/QNetworkReply>
 #include <QtNetwork/QNetworkRequest>
@@ -76,15 +77,6 @@ QDebug BaseJob::Status::dumpToLog(QDebug dbg) const
     return dbg << ": " << message;
 }
 
-struct NetworkReplyDeleter : public QScopedPointerDeleteLater {
-    static inline void cleanup(QNetworkReply* reply)
-    {
-        if (reply && reply->isRunning())
-            reply->abort();
-        QScopedPointerDeleteLater::cleanup(reply);
-    }
-};
-
 template <typename... Ts>
 constexpr auto make_array(Ts&&... items)
 {
@@ -112,6 +104,16 @@ public:
         retryTimer.setSingleShot(true);
     }
 
+    ~Private()
+    {
+        if (reply) {
+            if (reply->isRunning()) {
+                reply->abort();
+            }
+            delete reply;
+        }
+    }
+
     void sendRequest();
     /*! \brief Parse the response byte array into JSON
      *
@@ -140,7 +142,10 @@ public:
 
     QByteArrayList expectedKeys;
 
-    QScopedPointer<QNetworkReply, NetworkReplyDeleter> reply;
+    // When the QNetworkAccessManager is destroyed it destroys all pending replies.
+    // Using QPointer allows us to know when that happend.
+    QPointer<QNetworkReply> reply;
+
     Status status = Unprepared;
     QByteArray rawResponse;
     /// Contains a null document in case of non-JSON body (for a successful
@@ -315,16 +320,16 @@ void BaseJob::Private::sendRequest()
 
     switch (verb) {
     case HttpVerb::Get:
-        reply.reset(connection->nam()->get(req));
+        reply = connection->nam()->get(req);
         break;
     case HttpVerb::Post:
-        reply.reset(connection->nam()->post(req, requestData.source()));
+        reply = connection->nam()->post(req, requestData.source());
         break;
     case HttpVerb::Put:
-        reply.reset(connection->nam()->put(req, requestData.source()));
+        reply = connection->nam()->put(req, requestData.source());
         break;
     case HttpVerb::Delete:
-        reply.reset(connection->nam()->sendCustomRequest(req, "DELETE", requestData.source()));
+        reply = connection->nam()->sendCustomRequest(req, "DELETE", requestData.source());
         break;
     }
 }
-- 
cgit v1.2.3