Details
-
Bug
-
Resolution: Done
-
P1: Critical
-
5.11.2, 5.12.0, 5.12.1, 5.12.2, 5.12.3, 5.12.4, 5.13.0 Alpha 1, 5.13.0 RC 1, 5.13.0 RC 2, 5.13.0 RC 3, 5.13.0
-
None
-
-
b14f5f59a3ae96949e6a33302385a751d6448182
Description
The qt websocket implements the frame parser in the QWebSocketDataProcessor/QWebSocketFrame classes.
Every time that the tcpsocket emits a readyRead() signal, the function QWebsocketDataProcess:: process(...) is called, which calls itself the QWebSocketFrame::readFrame(..), here's an extract:
QWebSocketFrame QWebSocketFrame::readFrame(QIODevice *pIoDevice) { bool isDone = false; qint64 bytesRead = 0; QWebSocketFrame frame; quint64 dataWaitSize = 0; Q_UNUSED(dataWaitSize); // value is used in MACRO, Q_UNUSED to avoid compiler warnings ProcessingState processingState = PS_READ_HEADER; ProcessingState returnState = PS_READ_HEADER; bool hasMask = false; quint64 payloadLength = 0; while (!isDone) { switch (processingState) { case PS_WAIT_FOR_MORE_DATA: //TODO: waitForReadyRead should really be changed //now, when a WebSocket is used in a GUI thread //the GUI will hang for at most 5 seconds //maybe, a QStateMachine should be used if (!pIoDevice->waitForReadyRead(5000)) { frame.setError(QWebSocketProtocol::CloseCodeGoingAway, tr("Timeout when reading data from socket.")); processingState = PS_DISPATCH_RESULT; } else { processingState = returnState; } break;
The person who wrote the code apparently knew himself, that this was incorrect seeing the comment "TODO".
Indeed, the code here, is blocking (for maximum 5sec). As a result:
1) This will block the event loop, therefore we can't use qwebsocket in gui thread unlike other socket class for Qt.
We also can't use it in thread where the event loop cannot be blocked (for example thread where we have qt socket classes (they rely on event loop)).
2) Waiting for only 5sec might be not enough, as the data from tcp network could come later, this would result in a incorrect websocket connection close.
3) An attacker could use this to block the thread of the qwebsocket (which is the same as the qwebsocketserver). Indeed he could send small chunks of data continuously. This would cause a denial of service: as the event loop wouldn't be run, the qwebsocketserver wouldn't be able to process other clients.
This class should be asynchronous, as the other qt socket classes (qtcpsocket, qudpsocket, qnetworkaccessmanager, etc)
Attachments
Issue Links
- is duplicated by
-
QTBUG-63719 Incoming QML WebSocket messages block main event loop against network
-
- Closed
-