qlcplus icon indicating copy to clipboard operation
qlcplus copied to clipboard

Pull in QT WebSocket library for parsing

Open awwright opened this issue 2 years ago • 9 comments

This avoids crashes when frames arrive in separate TCP packets.


This is an alternative to #1335 and closes #1308. It pulls in the official Qt parser, which is otherwise a private API.

This is the output from a websocket test suite, index.html.pdf. The failed cases 1.1.6/1.1.7/1.1.8/1.2.6/1.2.7/1.2.8 are testing packet lengths that are too big, and the Qt code responds appropriately with response code 1009.

(To get it to work with the test suite, you have to slightly modify the code to echo back complete messages, see the following patch.)

diff --git a/webaccess/src/qhttpserver/qhttpconnection.cpp b/webaccess/src/qhttpserver/qhttpconnection.cpp
index 806a8f219..2549dde70 100644
--- a/webaccess/src/qhttpserver/qhttpconnection.cpp
+++ b/webaccess/src/qhttpserver/qhttpconnection.cpp
@@ -362,6 +362,8 @@ QHttpConnection *QHttpConnection::enableWebSocket()
         this, SLOT(slot_pingReceived(const QByteArray&)));
     connect(m_webSocketParser, SIGNAL(textMessageReceived(const QString&)),
         this, SLOT(slot_textMessageReceived(const QString&)));
+    connect(m_webSocketParser, SIGNAL(binaryMessageReceived(const QByteArray&)),
+        this, SLOT(slot_binaryMessageReceived(const QByteArray&)));
     connect(m_webSocketParser, SIGNAL(errorEncountered(QWebSocketProtocol::CloseCode, const QString)),
         this, SLOT(slot_closeReceived(QWebSocketProtocol::CloseCode)));
     connect(m_webSocketParser, SIGNAL(closeReceived(QWebSocketProtocol::CloseCode, const QString)),
@@ -398,6 +400,11 @@ void QHttpConnection::webSocketWrite(WebSocketOpCode opCode, const QByteArray &d
     }
 }
 
+void QHttpConnection::slot_binaryMessageReceived(const QByteArray &data)
+{
+    webSocketWrite(BinaryFrame, data);
+}
+
 void QHttpConnection::slot_pingReceived(const QByteArray &data)
 {
     webSocketWrite(Pong, data);
@@ -405,6 +412,7 @@ void QHttpConnection::slot_pingReceived(const QByteArray &data)
 
 void QHttpConnection::slot_textMessageReceived(const QString &data)
 {
+    webSocketWrite(TextFrame, data.toUtf8());
     Q_EMIT webSocketDataReady(this, data);
 }
 
diff --git a/webaccess/src/qhttpserver/qhttpconnection.h b/webaccess/src/qhttpserver/qhttpconnection.h
index cca596a59..100c40c64 100644
--- a/webaccess/src/qhttpserver/qhttpconnection.h
+++ b/webaccess/src/qhttpserver/qhttpconnection.h
@@ -109,6 +109,7 @@ Q_SIGNALS:
 private Q_SLOTS:
     void slotWebSocketPollTimeout();
     void slot_pingReceived(const QByteArray &data);
+    void slot_binaryMessageReceived(const QByteArray &data);
     void slot_textMessageReceived(const QString &data);
     void slot_closeReceived(QWebSocketProtocol::CloseCode closeCode);

awwright avatar May 26 '22 23:05 awwright

3da306527 is pulled from Qt 5.15.2 and appears to use some API features too new for 5.14.2... Let's try downgrading to 5.15.0 before QStringView::toInt was introduced and see if that fixes the build (commit 3404c9d)

awwright avatar May 27 '22 18:05 awwright

@awwright please see my last comment on #1308 What you're experiencing with this build is exactly what I'm talking about. Let's avoid it...

mcallegari avatar May 27 '22 18:05 mcallegari

@mcallegari I see—sorry, I thought I was trying something new by copying in the sources. However, I was able to get much further in just a couple hours of work, compared to #1335. I tried this because when I was running the tests on #1335 there were still more than a few failures that could plausibly cause data corruption or some other problems in the future. I can provide the test results if you're interested.

Maybe you can still take a look at this, I seem to have everything working. There's just the errors from the code analysis tool that can probably be ignored.

awwright avatar May 28 '22 01:05 awwright

@mcallegari There seems to be an error with the AppVeyor builds in general (was there a system change?), but otherwise it should pass, and I think this is ready for your consideration. The Codacy analysis shows one type of error ("Check buffer boundaries") which can be ignored: in all the cases the code has already checked the buffer boundary using bytesAvailable. Please see https://github.com/mcallegari/qlcplus/issues/1308#issuecomment-1140605478 for a comparison of the solutions I've tried.

awwright avatar Jun 06 '22 06:06 awwright

@mcallegari This PR is ready for your consideration, could you please take a look? The only remaining error is the code analysis wants read() calls to check the return value, which it is doing.

awwright avatar Jun 21 '22 03:06 awwright

As I mentioned here a month ago, I'm not sure if I want to drag in another Qt dependency to the project. I'd prefer to fix the current implementation as, thanks to your analysis, it seems the issue to fix is a corner case reported basically only by you. I could spend some time to reproduce the issue and attempt a fix myself without extra dependencies.

mcallegari avatar Jul 01 '22 06:07 mcallegari

@mcallegari I'm hoping to submit a number of other improvements to the Web interface, especially around tempo, and even with the simple fixes that prevents crashing, the test suite still flagged a number of other issues that will likely cause problems, see qlc+-websocket-parsing.pdf.

We could write code that fixes all of those problems, but it would not be as quick to implement, or easy to maintain, as copying in an existing parser. No dependencies have been added, it uses the same public Qt API that the rest of QLC+ uses.

I spent several days on #1335; I was able to finish this PR in a couple hours. In my testing, everyone who uses Wi-Fi is affected.

awwright avatar Jul 01 '22 18:07 awwright

@mcallegari Can you please merge this? It doesn't add any dependencies.

awwright avatar Dec 15 '22 06:12 awwright