maxPayload crash: "this.onerror is not a function" in Receiver

[rkaw92]

The server code below can easily be crashed by sending a message that is over the size limit of 100 bytes (on master):

var ws = require('ws');

var websocketServer = new ws.Server({
    port: 7717,
    disableHixie: true,
    clientTracking: false,
    perMessageDeflate: false,
    maxPayload: 100
});

websocketServer.on('connection', function(socket) {
    setInterval(function() {
        socket.send('some notification');
    }, 1000);

    socket.on('error', function() {});
});

The output:

/home/thewanderer/Devel/Node/WebSocket/bugs/maxPayload/node_modules/ws/lib/Receiver.js:331
  this.onerror(reason, protocolErrorCode);
       ^

TypeError: this.onerror is not a function
    at Receiver.error (/home/thewanderer/Devel/Node/WebSocket/bugs/maxPayload/node_modules/ws/lib/Receiver.js:331:8)
    at Receiver.<anonymous> (/home/thewanderer/Devel/Node/WebSocket/bugs/maxPayload/node_modules/ws/lib/Receiver.js:483:18)
    at Receiver.add (/home/thewanderer/Devel/Node/WebSocket/bugs/maxPayload/node_modules/ws/lib/Receiver.js:102:24)
    at Socket.realHandler (/home/thewanderer/Devel/Node/WebSocket/bugs/maxPayload/node_modules/ws/lib/WebSocket.js:801:20)
    at emitOne (events.js:90:13)
    at Socket.emit (events.js:182:7)
    at readableAddChunk (_stream_readable.js:147:16)
    at Socket.Readable.push (_stream_readable.js:111:10)
    at TCP.onread (net.js:525:20)

Specifically, I am using a very talkative test client, which keeps sending a 100KB-long "lorem ipsum" as fast as possible.

My guess is that lib/Receiver.js:122 is the part that deletes the function.

[LordMajestros]

@rkaw92 the socket is closed (line 849 in https://github.com/websockets/ws/blob/master/lib/WebSocket.js) after receiving a payload that exceeds the maxPayload (the assumption is a rogue client attempting a DOS) so it should be invalid after that. My guess is there is a race condition i.e. it receives another message before the cleanup is complete but after deleting the function.
What happens if you add a delay to your client?
I will see if I can create a PR with a fix.

[rkaw92]

It appears that even a single message can crash the server: gist.
For reference, the original client code is here.

No more messages are required. Turning off the maxPayload option (passing zero or undefined) makes the crash go away.

[LordMajestros]

Doing a PR right now to fix

[LordMajestros]

@rkaw92 could you please test this and confirm if the issue is resolved? #681

[LordMajestros]

@rkaw92 you may need to catch the 'not opened' error in thrown in lib/Websocket.js:218:5 to prevent a crash but this crash is expected behaviour.
My modification of your server code:

var ws = require('ws');

var websocketServer = new ws.Server({
    port: 7717,
    disableHixie: true,
    clientTracking: false,
    perMessageDeflate: false,
    maxPayload: 100
});

websocketServer.on('connection', function(socket) {
    setInterval(function() {
        try{
            socket.send('some notification');
        }
        catch(exception){
        }
    }, 1000);

    socket.on('error', function(error) {
        console.log("An error occured",error);
    });
});

[rkaw92]

Indeed, the fix does work. Apparently, the try {} catch block is also required around the server's .send() - otherwise an "Error: not opened" is thrown.

[LordMajestros]

Thanks @rkaw92 cc @3rd-Eden