Network (HTTPS): prevent recursion among ->close() methods

We observed a stack-trace in which, while handling an error,
QHttpNetworkConnectionChannel::close()'s call to its socket->close()
triggered (when the socket was a QSslSocket) a flush() which asked its
backend to transmit() which tripped over the original error, which
duly triggered endless recursion.  Transiently clear the socket
member, during its ->close(), to prevent this; do the same in abort(),
to preserve its structural correspondence to close().  Restructure
both so that any recursive call's setting of state is overwritten by
the top-level call's, while this still uses the prior socket state
(not the state after close() or abort() and any recursion) to
determine final state.

Task-number: QTBUG-56476
Change-Id: If69e97f7a77a729bf2338ed14214c65aa95f8b05
Reviewed-by: Timur Pocheptsov <timur.pocheptsov@qt.io>

1 files changed, 23 insertions, 21 deletions

diff --git a/src/network/access/qhttpnetworkconnectionchannel.cpp b/src/network/access/qhttpnetworkconnectionchannel.cpp
index 646ede0022..d8ce28a7b0 100644
--- a/src/network/access/qhttpnetworkconnectionchannel.cpp
+++ b/src/network/access/qhttpnetworkconnectionchannel.cpp
@@ -44,6 +44,7 @@
 
 #include <qpair.h>
 #include <qdebug.h>
+#include <QScopedValueRollback>
 
 #ifndef QT_NO_HTTP
 
@@ -196,41 +197,42 @@ void QHttpNetworkConnectionChannel::init()
 
 void QHttpNetworkConnectionChannel::close()
 {
-    if (!socket)
-        state = QHttpNetworkConnectionChannel::IdleState;
-    else if (socket->state() == QAbstractSocket::UnconnectedState)
-        state = QHttpNetworkConnectionChannel::IdleState;
-    else
-        state = QHttpNetworkConnectionChannel::ClosingState;
-
+    // socket can be 0 since the host lookup is done from qhttpnetworkconnection.cpp while
+    // there is no socket yet; we may also clear it, below or in abort, to avoid recursion.
+    const bool idle = !socket || socket->state() == QAbstractSocket::UnconnectedState;
+    const ChannelState final = idle ? IdleState : ClosingState;
     // pendingEncrypt must only be true in between connected and encrypted states
     pendingEncrypt = false;
 
     if (socket) {
-        // socket can be 0 since the host lookup is done from qhttpnetworkconnection.cpp while
-        // there is no socket yet.
-        socket->close();
+        QAbstractSocket *const detached = socket;
+        QScopedValueRollback<QAbstractSocket *> rollback(socket, nullptr);
+        state = final; // Needed during close(), which may over-write it.
+        detached->close(); // Error states can cause recursion back to this method.
     }
-}
 
+    // Set state *after* close(), to override any recursive call's idle setting:
+    state = final;
+}
 
 void QHttpNetworkConnectionChannel::abort()
 {
-    if (!socket)
-        state = QHttpNetworkConnectionChannel::IdleState;
-    else if (socket->state() == QAbstractSocket::UnconnectedState)
-        state = QHttpNetworkConnectionChannel::IdleState;
-    else
-        state = QHttpNetworkConnectionChannel::ClosingState;
-
+    // socket can be 0 since the host lookup is done from qhttpnetworkconnection.cpp while
+    // there is no socket yet; we may also clear it, below or in close, to avoid recursion.
+    const bool idle = !socket || socket->state() == QAbstractSocket::UnconnectedState;
+    const ChannelState final = idle ? IdleState : ClosingState;
     // pendingEncrypt must only be true in between connected and encrypted states
     pendingEncrypt = false;
 
     if (socket) {
-        // socket can be 0 since the host lookup is done from qhttpnetworkconnection.cpp while
-        // there is no socket yet.
-        socket->abort();
+        QAbstractSocket *const detached = socket;
+        QScopedValueRollback<QAbstractSocket *> rollback(socket, nullptr);
+        state = final;
+        detached->abort();
     }
+
+    // Set state *after* abort(), to override any recursive call's idle setting:
+    state = final;
 }