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.

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

Fix client Websockets (broken) #2749

Merged
merged 16 commits into from
Apr 3, 2024
Merged

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Mar 27, 2024

This PR addresses several issues discovered in the client websocket stack. A simple local websocket echo server has been added for testing, which can be run using make wsserver.

Memory leaks

Running HttpServer_Websockets sample with valgrind shows a memory leak (details below, fig 1).
I can see from the logic of WebsocketConnection::send that there are multiple reasons the call could fail, but the source stream is only destroyed in one of them.

Failed connection

Testing with the local server failed with websockets.exceptions.InvalidHeaderValue: invalid Sec-WebSocket-Key header.
The key was 17 bytes instead of 16.

utf-8 decoding errors

Turns out message was getting corrupted because mask value passed to XorStream is on stack, which then gets overwritten before message has been sent out. Fixed by taking a copy of the value.

CLOSE message not being sent

Tested with Websocket_Client sample (running local echo server) to confirm correct behaviour, noticed a Streams without known size are not supported message when closing the connection. This blocked sending 'CLOSE' notifications which have no payload.

Issues during CLOSE

The TCP socket was being closed too soon, causing additional errors. Increasing timeout to 2 seconds fixes this. Also included a status code in the CLOSE message indicating normal closure; this is optional, but seems like a good thing to do.

RFC 6455 states: The application MUST NOT send any more data frames after sending a Close frame. If an endpoint receives a Close frame and did not previously send a Close frame, the endpoint MUST send a Close frame in response.

Therefore, the close() logic has been changed so that a CLOSE message is always sent, either in response to a previous incoming request (in which case the received status is echoed back) or as notification that we're closing (status 1000 - normal closure). Checked server operation with HttpServer_Websockets sample

Simplify packet generation

It's not necessary to pre-calculate the packet length as it's never more than 14 bytes in length.

WebsocketConnection not getting destroyed

HttpConnection objects are not 'auto-released' so leaks memory every time a WebsocketConnection is destroyed (512+ bytes). Simplest fix for this is to add a setAutoSelfDestruct method to TcpConnection so this can be changed.

Messages not being received

Connection is activated OK, but HttpClientConnection then calls init in its onConnected handler which resets the new receive delegate. This causes incoming websocket frames to be passed to the http parser, instead of the WS parser, hence the HTTP parser error: HPE_INVALID_CONSTANT message.

====

Fig 1: Initial memory leak reported by valgrind

==1291918== 
==1291918== HEAP SUMMARY:
==1291918==     in use at exit: 4,133 bytes in 16 blocks
==1291918==   total heap usage: 573 allocs, 557 frees, 71,139 bytes allocated
==1291918== 
==1291918== 64 bytes in 2 blocks are definitely lost in loss record 10 of 13
==1291918==    at 0x4041D7D: operator new(unsigned int) (vg_replace_malloc.c:476)
==1291918==    by 0x8075BC5: WebsocketConnection::send(char const*, unsigned int, ws_frame_type_t) (WebsocketConnection.cpp:180)
==1291918==    by 0x804EB65: send (WebsocketConnection.h:107)
==1291918==    by 0x804EB65: sendString (WebsocketConnection.h:145)
==1291918==    by 0x804EB65: wsCommandReceived(WebsocketConnection&, String const&) (application.cpp:88)
==1291918==    by 0x807575D: operator() (std_function.h:591)
==1291918==    by 0x807575D: WebsocketConnection::staticOnDataPayload(void*, char const*, unsigned int) (WebsocketConnection.cpp:128)
==1291918==    by 0x8081E5C: ws_parser_execute (ws_parser.c:263)
==1291918==    by 0x80756C6: WebsocketConnection::processFrame(TcpClient&, char*, int) (WebsocketConnection.cpp:103)
==1291918==    by 0x8079305: operator() (std_function.h:591)
==1291918==    by 0x8079305: TcpClient::onReceive(pbuf*) (TcpClient.cpp:150)
==1291918==    by 0x8078A8E: TcpConnection::internalOnReceive(pbuf*, signed char) (TcpConnection.cpp:484)
==1291918==    by 0x8058560: tcp_input (in /stripe/sandboxes/sming-dev/samples/HttpServer_WebSockets/out/Host/debug/firmware/app)
==1291918==    by 0x80627F3: ip4_input (in /stripe/sandboxes/sming-dev/samples/HttpServer_WebSockets/out/Host/debug/firmware/app)
==1291918==    by 0x8063C89: ethernet_input (in /stripe/sandboxes/sming-dev/samples/HttpServer_WebSockets/out/Host/debug/firmware/app)
==1291918==    by 0x8064245: tapif_select (in /stripe/sandboxes/sming-dev/samples/HttpServer_WebSockets/out/Host/debug/firmware/app)
==1291918== 
==1291918== LEAK SUMMARY:
==1291918==    definitely lost: 64 bytes in 2 blocks
==1291918==    indirectly lost: 0 bytes in 0 blocks
==1291918==      possibly lost: 0 bytes in 0 blocks
==1291918==    still reachable: 4,069 bytes in 14 blocks
==1291918==         suppressed: 0 bytes in 0 blocks
==1291918== Reachable blocks (those to which a pointer was found) are not shown.
==1291918== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==1291918== 
==1291918== For lists of detected and suppressed errors, rerun with: -s
==1291918== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

mikee47 pushed a commit to mikee47/Sming that referenced this pull request Mar 27, 2024
(Discovered memory leak in `WebsocketConnection::send` - SmingHub#2749)
@mikee47 mikee47 changed the title Ensure WebsocketConnection::send releases source stream on failure Fix WebsocketConnection::send leak and close failure Mar 28, 2024
@mikee47 mikee47 changed the title Fix WebsocketConnection::send leak and close failure [WIP] Fix WebsocketConnection::send leak and close failure Mar 28, 2024
@mikee47 mikee47 changed the title [WIP] Fix WebsocketConnection::send leak and close failure Fix WebsocketConnection::send leak and close failure Mar 28, 2024
@mikee47 mikee47 changed the title Fix WebsocketConnection::send leak and close failure [WIPFix WebsocketConnection::send leak and close failure Mar 28, 2024
@mikee47 mikee47 changed the title [WIPFix WebsocketConnection::send leak and close failure [WIP] Fix WebsocketConnection::send leak and close failure Mar 28, 2024
@mikee47
Copy link
Contributor Author

mikee47 commented Mar 28, 2024

Changing to WIP as there appear to be further issues with client websockets. Am testing with local websocket server and it's thrown up some additional problems I'm resolving.

@mikee47 mikee47 changed the title [WIP] Fix WebsocketConnection::send leak and close failure Fix WebsocketConnection::send leak and close failure Mar 28, 2024
@mikee47
Copy link
Contributor Author

mikee47 commented Mar 28, 2024

OK, seems to behave itself much better now.

@mikee47 mikee47 changed the title Fix WebsocketConnection::send leak and close failure Fix client Websocket bugs Mar 28, 2024
@slaff slaff added this to the 5.2.0 milestone Mar 29, 2024
Copy link
Contributor Author

@mikee47 mikee47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also needs rebase. And running through valgrind again...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mask must be unique_ptr.

@mikee47 mikee47 changed the title Fix client Websocket bugs [WIP] Fix client Websocket bugs Mar 29, 2024
@mikee47
Copy link
Contributor Author

mikee47 commented Mar 29, 2024

Yet more bugs... this is driving me potty! Two things:

  1. If sample MESSAGES_TO_SEND is an even number, server throws exceptions when closing. This only happens running on Host, on esp8266 seems fine. Increasing time between messages to 2 seconds and the problem goes away.
  2. No websocket messages are received by the client!
  3. Getting HTTP parser error: HPE_INVALID_CONSTANT, cause as yet unknown

I suspect 2 and 3 are related. Something off in ws_parser library? Or http-parser? Or how they're used?

@mikee47 mikee47 changed the title [WIP] Fix client Websocket bugs Fix client Websocket (broken) Mar 29, 2024
@mikee47
Copy link
Contributor Author

mikee47 commented Mar 29, 2024

@slaff OK, reception now working. This has turned out to be a larger PR than I first anticipated. Also broken in 4.2.2 LTS.
There may be a neater solution to the receive-packets issue - any thoughts?

@mikee47 mikee47 changed the title Fix client Websocket (broken) Fix client Websockets (broken) Mar 29, 2024
@mikee47 mikee47 changed the title Fix client Websockets (broken) [WIP] Fix client Websockets (broken) Mar 30, 2024
@mikee47
Copy link
Contributor Author

mikee47 commented Mar 30, 2024

Testing with websocket server to make sure that's not broken...

mikee47 added 13 commits March 31, 2024 09:50
Add `wsserver` build target with simple echo server
Use local server by default (remote website no longer exists)
Add `test.py` script to check connection protocol
Update sample to send both text and binary messages
Websocket client passes local mask data which can get corrupted, leading to unreliable messages.
HttpConnection is NOT auto-self-destruct so we need to change that during closure.
`HttpClientConnection` resets our `receive` delegate
Can either be in response to a received CLOSE, or as an outgoing notification.
Only difference is in the status code sent.
@mikee47 mikee47 changed the title [WIP] Fix client Websockets (broken) Fix client Websockets (broken) Apr 1, 2024
@slaff slaff merged commit c487f91 into SmingHub:develop Apr 3, 2024
46 checks passed
@mikee47 mikee47 deleted the fix/websocket-memleak branch April 3, 2024 08:53
@slaff slaff mentioned this pull request May 8, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants