Node: Accidentally writing garbage to the IPC channel blows up parent in debug build

[seishun]

• Version: master
• Platform: Windows
• Subsystem: child_process

index.js

'use strict'
const spawn = require('child_process').spawn
const path = require('path')

let dir = path.join(__dirname, 'child.js')
let server = spawn(process.argv0, [`"${dir}"`], {
  stdio: ['pipe', 'ipc', 'pipe'],
  shell: true,
})

child.js

// choose one to your liking
// option 1
console.log('here bomb');
// option 2
console.log('here bomb hahahahahaha');
// option 3
console.log('\u0000\u0000\u0000\u0000here bomb hahahahahaha');

Outputs for 1,2,3 respectively:

Assertion failed: avail >= sizeof(ipc_frame.header), file src\win\pipe.c, line 1590
Assertion failed: ipc_frame.header.flags <= (UV_IPC_TCP_SERVER | UV_IPC_RAW_DATA | UV_IPC_TCP_CONNECTION), file src\win\pipe.c, line 1604
Assertion failed: handle->pipe.conn.remaining_ipc_rawdata_bytes >= bytes, file src\win\pipe.c, line 1655

At first I thought it's a bug in libuv, but now I'm not sure. From all the asserts, it seems it assumes the child will only write valid data. In that case, I think Node.js should either disallow using console.log when stdout is used for IPC, or disallow using stdout for IPC.

[bnoordhuis]

IPC on Windows uses an ad hoc protocol that parent and child are expected to adhere to. So yes, working as intended as far as libuv is concerned. :-)

There are a few (non-node.js) programs out there that speak libuv's protocol. Although unlikely, prohibiting stdio from being used as an IPC channel might break such programs.

[seishun]

Assertion errors are acceptable when you violate an API contract in C, but not in Node.js. So Node.js should prevent this from happening.

The following also reproduces the issue, so it's not limited to stdout.

index.js

const spawn = require('child_process').spawn
const path = require('path')

let dir = path.join(__dirname, 'child.js')
let server = spawn(process.argv0, [dir], {
  stdio: [0, 1, 2, 'ipc']
})

child.js

const fs = require('fs');
fs.writeSync(3, 'asdfasdfasdf');

Output:

Assertion failed: avail >= sizeof(ipc_frame.header), file src\win\pipe.c, line 1590

My proposal:

• On the child side, somehow disallow directly writing to an fd that is being used for IPC.
• On the parent side, disallow using 'ipc' if the child isn't Node.js, or state that it's undefined behavior if the child doesn't adhere to libuv's protocol (although it seems the latter is generally avoided, see https://github.com/nodejs/node/issues/8724).

[seishun]

17460 doesn't fix this issue.

[bzoz]

How about: https://github.com/nodejs/node/pull/17545 ?

I don't think we can disallow using IPC in the code. We would have to add a lot of asserts. This IPC is not reliable anyway, see my comment here: https://github.com/nodejs/node/issues/17405#issuecomment-349026019. I say we document it is undefined.

[apapirovski]

@seishun @bzoz can we close this out? Can anything else be done?

[seishun]

I still believe that it shouldn't be possible to cause Node.js to crash so easily, and it's possible to prevent it (see my proposal in https://github.com/nodejs/node/issues/16491#issuecomment-340026902). But if the general opinion is that this is expected behavior, then I'm not going to oppose it.

[apapirovski]

@seishun thanks. I'll mark as help wanted for now since this gives us a bit of a lead for anyone that wants to take it up and see if anything comes of it in the near future.

[jasnell]

Given that there's been no further discussion here and no clear path forward, closing... but, I'm putting this on the futures project board so that it does not get lost.