Server should not crash on assetion errors from the underlying library

[zenlor]

As the title. I took the ws example changing the protocol to pub/sub:

var ws = new WebSocket('ws://127.0.0.1:7789', [
    'pub.sp.nanomsg.org'
]);

On the server:

var pub = nano.socket('pub');
pub.on('error', (err) => console.error(err));
...

After a ws.send('') from the client:

node server.js
Assertion failed: 0 (../deps/nanomsg/src/protocols/pubsub/xpub.c:138)
[1]    4925 abort (core dumped)  node server.js

Now I know the client shouldn't send anything over a pub/sub socket but it should never crash the server.

[yoshuawuyts]

👍 I tend to agree; valid server code should not crash due to invalid client behavior. In this case an error should have been emitted instead.

[nickdesaulniers]

oops, yep we shouldn't try to send strings/buffers of length 0. 😨

[nickdesaulniers]

oh: https://github.com/nanomsg/nanomsg/blob/dd23bfb44fd76418cf8f3b897698d210572c4221/src/protocols/pubsub/xpub.c#L138 ??

[nickdesaulniers]

@reqshark this looks like a bug upstream. There's no error handling, just an always failing assertion that we can't handle.

[nickdesaulniers]

we should hook this up to gdb and get a back trace of the failed assertion. Maybe we can avoid passing anything to that code path if we're a sub socket.

[reqshark]

@nickdesaulniers as I mentioned to you earlier tonight at PeninsulaJS (which was awesome 🎉 ), I couldn't get ws transport to work with pub/sub after the libnanomsg 0.8-beta update.

@aliem, I had some success using ws transport with a pub/sub on libnanomsg around 0.7-beta or thereabouts, maybe it was 0.6-beta. If you need ws://pubsub you might want to try those versions using the system flag with npm install

npm install nanomsg --use_system_libnanomsg=true

btw the transport is still experimental and in development, nanomsg website says dont use yet:

DESCRIPTION
THIS IS AN EXPERIMENTAL FEATURE. DO NOT USE. THE FUNCTIONALITY IS SUBJECT TO CHANGE WITHOUT PRIOR NOTICE.

When calling either nn_bind() or nn_connect(), omitting the port defaults to the RFC 6455 default port 80 for HTTP. Example:

ws://127.0.0.1 is equivalent to ws://127.0.0.1:80

also @aliem if you end up trying stuff in the 0.6-beta 0.7-beta range, please report your results

[reqshark]

in your example code you are setting both endpoints to pub, one of which has to be sub (err.. that being said i wasn't able to get it to work when configured differently)

another thought, something maybe in addition to getting a gdb backtrace on that assertion, might have to do with what we're not exposing visa vie WS socket options at the transport layer.

for example, related to #139, see usage in this test: https://github.com/nanomsg/nanomsg/blob/master/tests/ws.c#L54-L56

we're not setting anything like that before trying to send stuff (though PAIR seems to be satisfied)

[zenlor]

I'm following the spec: pub.sp.nanomsg.org (NN_PUB server, NN_SUB client)

The pattern itself works nicely but the client can still send messages to the server crashing it. For now I'm using it only for an internal UI, so, security does not matter in my constrained environment but it's definitely an issue for different environments.

In any case it seems more an upstream issue. I'll see if I have time to reproduce the crash in plain C this weekend.

[reqshark]

nice job and thanks for pointing that out!

your example code is correct here, my mistake about those endpoints 👍

feel free to PR other working ws browser examples/patterns, like msgs over pubsub.. also areas of the ws spec like what you mentioned should be documented here and added to the readme

cheers! 🍻 to following the spec!

[nickdesaulniers]

please reopen if this is still a problem.