From 056c89e95d8138682df60b0a24b5381080f71f37 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Thu, 4 Jul 2024 14:21:15 +0200 Subject: [PATCH] QRestAccessManager: don't leak slot objects when no QNAM was set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We try to pass QSlotObjectBase as raw pointer through the ABI boundary, because we hope to have the template-wrapper tail-call the out-of-line helper¹. But that means that the out-of-line helper needs to ensure that it deletes the slot object on every possible exit from the function. The old code didn't do that e.g. when the qnam check failed, or, theoretically, if requestOperation() threw an exception. The new code places the slot object under SlotObjUniquePtr guard as soon as possible (but, due to amount of callers, not before executeRequest()). Port createActiveRequest() to receive the slot object already in a smart pointer. Tail-calling isn't required here, because caller and callee are part of the same TU so the optimizer can do whatever it wants. Unique_ptr passing shouldn't be that hard to optimize. ¹ If we were to pass by SlotObjUniquePtr, tail-calling would be impossible due to the need to run the unique_ptr's dtor after he call to the helper. Amends 9ba5c7ff6aa42c5701cf950d2137467a2d178833, but e560adef213301318dcc13d4db155624846e0420 already had the requestOperation() problem. Pick-to: 6.7 Change-Id: I2ab5eadb35625393f274e3391d7b7c393ed8f08a Reviewed-by: Volker Hilsheimer (cherry picked from commit 11725d46344105325a1dec8628708ab88483ca12) Reviewed-by: Qt Cherry-pick Bot --- src/network/access/qrestaccessmanager.cpp | 4 ++-- src/network/access/qrestaccessmanager_p.h | 12 +++++++----- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/network/access/qrestaccessmanager.cpp b/src/network/access/qrestaccessmanager.cpp index 9e0182c7cb0..e56aee1e5d1 100644 --- a/src/network/access/qrestaccessmanager.cpp +++ b/src/network/access/qrestaccessmanager.cpp @@ -755,11 +755,11 @@ QNetworkReply *QRestAccessManager::customWithDataImpl(const QNetworkRequest &req QNetworkReply *QRestAccessManagerPrivate::createActiveRequest(QNetworkReply *reply, const QObject *contextObject, - QtPrivate::QSlotObjectBase *slot) + QtPrivate::SlotObjUniquePtr slot) { Q_Q(QRestAccessManager); Q_ASSERT(reply); - QtPrivate::SlotObjSharedPtr slotPtr(QtPrivate::SlotObjUniquePtr{slot}); // adopts + QtPrivate::SlotObjSharedPtr slotPtr(std::move(slot)); // adopts activeRequests.insert(reply, CallerInfo{contextObject, slotPtr}); // The signal connections below are made to 'q' to avoid stray signal // handling upon its destruction while requests were still in progress diff --git a/src/network/access/qrestaccessmanager_p.h b/src/network/access/qrestaccessmanager_p.h index 2e6c1afb907..dbe360f2ccd 100644 --- a/src/network/access/qrestaccessmanager_p.h +++ b/src/network/access/qrestaccessmanager_p.h @@ -36,18 +36,19 @@ public: ~QRestAccessManagerPrivate() override; QNetworkReply* createActiveRequest(QNetworkReply *reply, const QObject *contextObject, - QtPrivate::QSlotObjectBase *slot); + QtPrivate::SlotObjUniquePtr slot); void handleReplyFinished(QNetworkReply *reply); using ReqOpRef = qxp::function_ref; QNetworkReply *executeRequest(ReqOpRef requestOperation, - const QObject *context, QtPrivate::QSlotObjectBase *slot) + const QObject *context, QtPrivate::QSlotObjectBase *rawSlot) { + QtPrivate::SlotObjUniquePtr slot(rawSlot); if (!qnam) return warnNoAccessManager(); verifyThreadAffinity(context); QNetworkReply *reply = requestOperation(qnam); - return createActiveRequest(reply, context, slot); + return createActiveRequest(reply, context, std::move(slot)); } using ReqOpRefJson = qxp::function_ref; QNetworkReply *executeRequest(ReqOpRefJson requestOperation, const QJsonDocument &jsonDoc, const QNetworkRequest &request, - const QObject *context, QtPrivate::QSlotObjectBase *slot) + const QObject *context, QtPrivate::QSlotObjectBase *rawSlot) { + QtPrivate::SlotObjUniquePtr slot(rawSlot); if (!qnam) return warnNoAccessManager(); verifyThreadAffinity(context); @@ -68,7 +70,7 @@ public: } req.setHeaders(std::move(h)); QNetworkReply *reply = requestOperation(qnam, req, jsonDoc.toJson(QJsonDocument::Compact)); - return createActiveRequest(reply, context, slot); + return createActiveRequest(reply, context, std::move(slot)); } void verifyThreadAffinity(const QObject *contextObject);