Ping() hard crashes Node.js

[pimterry]

• I've searched for any related issues and avoided creating a duplicate issue.

Description

I'm testing keep alives in my WS application. I've just tried setting them to be sent all the time every 100ms, just for testing, and node.js core dumps when ping() is called.

The relevant code looks like this:

setInterval(() => {
    wsServer.clients.forEach((client) => {
        if (client.readyState !== Ws.OPEN) return;
        client.ping();
    });
}, options.webSocketKeepAlive);

If I comment out the ping() line, everything works fine. There might be another underlying cause elsewhere, but it's definitely the ping that triggers this. The server and client are both running on localhost in the same node.js process (as part of my test suite) and they're both using ws.

This is triggered immediately, so it's possible that I'm pinging during connection setup, after teardown or some other edge case. Regardless though, this shouldn't happen.

It's difficult to isolate this further down to a minimal reproduction, but if I do manage to do so I'll share that here.

ws version

Tested on both 8.2.2 & 7.55

Node.js Version

Tested on 14.17.0 and 16.8.0

System

 System:
    OS: Linux 5.4 Ubuntu 20.04.3 LTS (Focal Fossa)
    CPU: (4) x64 Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz
    Memory: 676.45 MB / 15.40 GB
    Container: Yes
    Shell: 5.8 - /usr/bin/zsh

Expected result

client.ping() should either work, or raise a handleable error somewhere. It should never crash node.

Actual result

node[177127]: ../src/stream_base.cc:220:int node::StreamBase::WriteString(const v8::FunctionCallbackInfo<v8::Value>&) [with node::encoding enc = (node::encoding)1]: Assertion `args[1]->IsString()' failed.
 1: 0xb02d90 node::Abort() [node]
 2: 0xb02e0e  [node]
 3: 0xbe4a7e int node::StreamBase::WriteString<(node::encoding)1>(v8::FunctionCallbackInfo<v8::Value> const&) [node]
 4: 0xbe5700 void node::StreamBase::JSMethod<&(int node::StreamBase::WriteString<(node::encoding)1>(v8::FunctionCallbackInfo<v8::Value> const&))>(v8::FunctionCallbackInfo<v8::Value> const&) [node]
 5: 0xd471cb  [node]
 6: 0xd4844a  [node]
 7: 0xd48926 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [node]
 8: 0x15ce039  [node]
Aborted (core dumped)

(from v16.8.0)

Attachments

No response

[lpinca]

Sounds like a Node.js bug. Are you able to get a core dump?

[lpinca]

Also note that ws always write buffers so that stack trace is a bit surprising.

[pimterry]

Found it! This is what happens if you set { objectMode: true } on a websocket. In my case, everything else still worked fine (all my data is actually strings at this stage anyway), so it was only the client's pong() after a server ping that broke everything.

Repro:

const WebSocket = require('ws');

const wss = new WebSocket.Server({ port: 8080 });

wss.on('connection', function connection(ws) {
  ws.ping();
  ws.on('pong', () => console.log('got pong'));
});

const ws = new WebSocket(`ws://localhost:8080`, { objectMode: true }); // <--

ws.on('ping', () => console.log('got ping'));

With object mode set you get an immediate crash & node native stack trace. Without object mode you get:

got ping
got pong

This happened because I'm using websocket-stream to wrap WS as a node stream, since my WS client code needs to work in both browsers & node (while WS's createWebSocketStream is Node-only). I wanted object mode elsewhere in my pipeline, it snuck into the websocket-stream setup, and that option was then passed through to WS and broke everything.

I'll close for this now, since it's clearly my bug and it's now fixed.

It's an easy mistake to make though and hard to catch (since most other methods seem to all keep working just fine, if you don't actually write objects) so it might be nice to protect against this in WS. It would be easy to enforce objectMode: false in setup to ignore this option for example, as WS already does for createWebSocketStream. Raising an error immediately (maybe in both cases) would be even clearer - if you try to create a websocket in objectMode, you're definitely making a mistake, and you probably want to know about it.

[lpinca]

Keep in mind that websocket-stream does not properly handle backpressure. See https://gist.github.com/lpinca/114a348f6108acc1e4236fed05522f73.

[lpinca]

I think this should be addressed in Node.js core.

$ cat crash.js
'use strict';

const net = require('net');

const server = net.createServer();

server.on('connection', function (socket) {
  socket.write(Buffer.from('foo'));
  socket.resume();
});

server.on('listening', function () {
  const { port } = server.address();
  const socket = net.createConnection({ port, objectMode: true });

  socket.on('data', function (chunk) {
    console.log(chunk);
    socket.write(chunk);
  });
});

server.listen();

$ node crash.js
<Buffer 66 6f 6f>
node[235]: ../src/stream_base.cc:220:int node::StreamBase::WriteString(const v8::FunctionCallbackInfo<v8::Value>&) [with node::encoding enc = (node::encoding)1]: Assertion `args[1]->IsString()' failed.
 1: 0xafd010 node::Abort() [node]
 2: 0xafd08e  [node]
 3: 0xbdfd0e int node::StreamBase::WriteString<(node::encoding)1>(v8::FunctionCallbackInfo<v8::Value> const&) [node]
 4: 0xbe0990 void node::StreamBase::JSMethod<&(int node::StreamBase::WriteString<(node::encoding)1>(v8::FunctionCallbackInfo<v8::Value> const&))>(v8::FunctionCallbackInfo<v8::Value> const&) [node]
 5: 0xd4339b  [node]
 6: 0xd4462c  [node]
 7: 0xd44b06 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [node]
 8: 0x15e8099  [node]
Aborted