Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pull in QT WebSocket library for parsing #1338

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Pull in QT WebSocket library for parsing #1338

wants to merge 1 commit into from

Conversation

awwright
Copy link
Contributor

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
Copy link
Contributor Author

awwright commented May 27, 2022 •
edited

3da3065 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)

@mcallegari
Copy link
Owner

@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...

@awwright awwright force-pushed the issue-1308-b branch 4 times, most recently from bf8238b to c6f3d7c Compare May 28, 2022 01:39
@awwright
Copy link
Contributor Author

awwright commented May 28, 2022 •
edited

@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 awwright force-pushed the issue-1308-b branch 3 times, most recently from 14a42bf to 97ab647 Compare May 30, 2022 02:00
@awwright awwright force-pushed the issue-1308-b branch 2 times, most recently from e23e3a1 to 97b2860 Compare June 6, 2022 05:54
@awwright
Copy link
Contributor Author

awwright commented Jun 6, 2022

@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 #1308 (comment) for a comparison of the solutions I've tried.

@awwright
Pull in Qt WebSocket library for parsing
8d4e883 
This avoids crashes when frames arrive in separate TCP packets.
@awwright
Copy link
Contributor Author

@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.

@mcallegari
Copy link
Owner

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.

@awwright
Copy link
Contributor Author

awwright commented Jul 1, 2022

@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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

A crash when using the Web interface
2 participants
@awwright @mcallegari