WebSocket server dies on multiple connection attempts from same client #227
Comments
rafalsk
changed the title
|
The server does not expect the socket to be closed after calling onConnectionCallback. You should close the socket in the onMessageCallback. We are doing this in our application without any problems. |
|
Please do read-up on the updated version. Even if we do not do anything; (i.e. return from setOnConnectionCallback) the sever dies. Since we do Live-programming sessions it's all here: https://www.youtube.com/gridnetproject @marcelkauf also, just referring to what you said what if a client does not send any message? Is he allowed to use up all the resources on a computer by forming new connections? Also, here it's not the case, the server dies after less than a dozen of attempts. |
rafalsk
changed the title
|
|
|
During the consecuritve faulty, invocation _onMessageCallback points to uninitialized memory-heap. Anyway this should be easy to reproduce. Just create WebSockets in a loop from JavaScript in browser. |
|
Another thing; Say Socket's events are handled by object's internal method. Now, when there's a call made to ->close() one could expect that events related to the socket would not fire anymore (since the object representing the conversation and thus encapsulating the web-socket) might had been deleted by then. So the event calls point to non-initialized memory regions. It's a separate dilemma; how to unsure than no more callbacks will be made so that the parental object can be deleted safely? The above we've managed to solve by giving an internal grace-period after which the conversation is considered for deletion, still, the problem is worthwhile to consider. |
|
The situation exists even if client connects to Web-Socket, closes the connection by himself and attempts to reconnect. |
|
All in all, during testing, if there's a situation in which the C++ app is debugged in any way(execution halted) and we have the other end (WebSocket JavaScript app running in Chrome) trying to establish a couple of new connections (even if only one or two additional), the server would throw at the same lines. the WebSocketInitResult Status has success true http_status 200 however the _ws object has _close_reason equal to "internal error" close_code 1011. This happens every time. is this going to be looked into into anytime soon, or are we on our own when it comes to this issue? In our case this requires restart of full-node software (minutes) after each time something goes on on the web-app's end. |
|
Maybe you reached the max connections allowed. Try to change the argument 4, which used to default to 32 but is now 128.
WebSocketServer(int port = SocketServer::kDefaultPort,
const std::string& host = SocketServer::kDefaultHost,
int backlog = SocketServer::kDefaultTcpBacklog,
size_t maxConnections = SocketServer::kDefaultMaxConnections,
int handshakeTimeoutSecs = WebSocketServer::kDefaultHandShakeTimeoutSecs,
int addressFamily = SocketServer::kDefaultAddressFamily);
… On Jul 26, 2020, at 12:27 AM, rafalsk ***@***.***> wrote:
All in all, during testing, if there's a situation in which the C++ app is debugged in any way(execution halted) and we have the other end (WebSocket JavaScript app trying to establish a couple of new connections (even if one or two additional), the server would throw at the same lines.
WebSocketInitResult status = _ws.connectToSocket(std::move(socket), timeoutSecs);
if (!status.success)
{
return status;
}
_onMessageCallback(
std::make_unique<WebSocketMessage>(WebSocketMessageType::Open,
"",
0,
WebSocketErrorInfo(),
WebSocketOpenInfo(status.uri, status.headers),
WebSocketCloseInfo()));
the WebSocketInitResult Status has success true http_status 200 however the _ws object has _close_reason equal to "internal error" close_code 1011
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#227 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC2O6UNWAQ2GNGYRQZERRELR5PLGVANCNFSM4PDPI3ZQ>.
|
|
@bsergean, did you at least read what I wrote and try to reproduce the error? Even if it did I would not expect the library to throw on me and internal variables to represent errors of sort 'invalid internal state' |
|
Just tried to use wscat (a javascript command line echo tool) to talk to an echo server, to reproduce your problem. I can create 5 connections as you can see in the previous email.
… On Jul 27, 2020, at 12:14 AM, rafalsk ***@***.***> wrote:
@bsergean <https://github.com/bsergean>, did you at least read what I wrote and try to reproduce the error?
Does spawning a 2nd , 3rd connection come close to reaching a default conn limit equal to 128?
Even if it did I would not expect the library to throw on me and internal variables to represent errors of sort 'invalid internal state'
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#227 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC2O6UPINQMWEM44I2366XLR5USMFANCNFSM4PDPI3ZQ>.
|
|
|
|
We shall provide a 'working' minimal reproducable example. |
|
|
|
we'll be getting back to this soon.. sorry for delay |
|
Please see the attachment; we've provided sample 'exploit' and exploitable server implementation. |
|
If I use ws echo_server as the server, and open your html file in Firefox things works fine (I could see logs for more that 128 connections). I haven't looked at your C++ code yet. |
|
I was able to compile your sample on macOS, with a small compile tweak so that it works with latest ixwebsocket. I made a gist out of it. https://gist.github.com/bsergean/08387e2e19601f7df19f74321baebb52/revisions I can reproduce a crash but it looks like incorrect usage. (ps: there is a lot of commented code in your repro, it's better if you remove it to create a minimal repro) You need to register a callback for the ws/webSocket object, as described here in the docs -> https://machinezone.github.io/IXWebSocket/usage/#websocket-server-api |
|
I changed the library to display a better error message when used improperly. |
|
You seem to be very fast at closing fresh issues 👍 The fact that something works with external Linux utilities does not imply that the library does not have internal issues when used through seemingly /intuitively valid invocations that are not stated to be invalid by docs. Should a fact of not registering for a callback be considered as in improper use of library is a delicate matter. I would rather say that the lib docs would need to explicitly state that registering for a given callback is a MUST. Still, it's irrelevant here. Do you really think we have not signed up for onMessage callbacks and were not receiving data in the first place to begin with? We did. First things first -> for a library to go wild if a callback was not registered, is at least not expected. So we've just discovered that as a side effect of the provided example. In the new API you've just briefly released (it's not part of release branch yet I can see), you seem to be checking for Null pointer, still it is not enough. What about a case when a pointer gets invalidated later on due to socket destruction since the callback function is passed simply as a reference. And we're now getting to the heart of matters.. Second, yes we did have the callback registered of course, since otherwise we wouldn't be able to receive any data in the first place and we did. It's the reproducible example which we've provided to be missing it indeed. The code might have contained comments sorry if these disturbed you. We saw the reproducible return same error so we stopped at that and provided. Now to the main part we were signing up for onMessageCallback within another object representing the particular conversation. Now, the Convesation object embedding the WebSocket has an internal subroutine handling the callback. When the CConversation object got deleted it made the internal subroutine no longer pointing to a valid region in memory. Now the main question is.. (which I suggested earlier) is the callback unregistered on socket destruction? Is it guarantied not to fire after the socket got destroyed? If not, then how does one unregister explicitly? I saw the new API; you just made available. The new API says the callback would not fire when the callback is NULL. Questions:
To sum up: The problem does not stem from our lack of registering for a callback but rather from the fact that the callback's pointer became invalid due to the object holding the socket being destroyed and thus invalidating pointer to the callback subroutine. The callback subroutine continued to be fired after the socket was released (we called close() on the socket before 'forgetting it'), from the viewpoint of our code (no instances) to its shared-ptr held by us. From our end everything was handled properly, socket supposedly closed, shared_ptr to WebSocket released, yet the library was still trying to execute the registered callback which belonged to a killed socket. Thus, leading to a situation which we've just stumbled upon, by providing the reproducible without a registered callback at all. Kindly do not call this an invalid use of the library. Additionally we suppose to be able to handle events per-socket basis (as it used to be) rather than per-global server. The 'new API' requires one to register for Messages callbacks per-server basis... wait a sec.. looking at you examples, the callback-registration routine does not take pointer to another function (the actual callback) but rather takes/provided immediate params? How are we supposed to handle events at per-connection basis? There are no connection-specific callbacks anymore? Kindly thank you for your assistance, let's make it better. |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Major conclustion: The situation is worse than we thought Initially.. I thought this to be a result of trying to close the socket within the setOnConnectionCallback() method. The reality is that if we have a client constantly trying to open new sockets the server throws and dies within a few seconds.
What we initially thought:
Situation: the client (Chrome) is invoking multiple socket instances. When we decide to close within setOnConnectionCallback() the server 'explodes' at the above line.
It occurs within setOnConnectionCallback() i.e. we want to limit the number of active connections from a single IP thus send error MSG and close the socket right within the onConnectionCallback
Update: I read up (by looking at an example) within documentation that stop() is used is used instead of close(), still close is a public method.What's the difference? When using stop() the result is the same.
end()/close() executes, then webSocketServer's handleConnection() kicks in, and throws at connectToSocket(), the socket param is null and timout -3.
The text was updated successfully, but these errors were encountered: