Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accidentally writing garbage to the IPC channel blows up parent in debug build #16491

Closed
seishun opened this issue Oct 25, 2017 · 8 comments
Closed
Labels
child_process Issues and PRs related to the child_process subsystem. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. libuv Issues and PRs related to the libuv dependency or the uv binding.

Comments

@seishun
Copy link
Contributor

seishun commented Oct 25, 2017

  • 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.

@seishun seishun added child_process Issues and PRs related to the child_process subsystem. libuv Issues and PRs related to the libuv dependency or the uv binding. labels Oct 25, 2017
@bnoordhuis
Copy link
Member

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
Copy link
Contributor Author

seishun commented Oct 27, 2017

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 buffer: segfault writing values with noAssert=true #8724).

bzoz added a commit to JaneaSystems/node that referenced this issue Dec 4, 2017
IPC messages are more complicated than a simple pipe passing JSON
objects separated by new line. This removes inaccurate notes about
implementation from the documentation.

Fixes: nodejs#16491
Fixes: nodejs#17405
@seishun
Copy link
Contributor Author

seishun commented Dec 7, 2017

#17460 doesn't fix this issue.

@bzoz
Copy link
Contributor

bzoz commented Dec 8, 2017

How about: #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: #17405 (comment). I say we document it is undefined.

MylesBorins pushed a commit that referenced this issue Dec 12, 2017
IPC messages are more complicated than a simple pipe passing JSON
objects separated by new line. This removes inaccurate notes about
implementation from the documentation.

PR-URL: #17460
Fixes: #16491
Fixes: #17405
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
IPC messages are more complicated than a simple pipe passing JSON
objects separated by new line. This removes inaccurate notes about
implementation from the documentation.

PR-URL: #17460
Fixes: #16491
Fixes: #17405
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this issue Dec 20, 2017
IPC messages are more complicated than a simple pipe passing JSON
objects separated by new line. This removes inaccurate notes about
implementation from the documentation.

PR-URL: #17460
Fixes: #16491
Fixes: #17405
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this issue Dec 20, 2017
IPC messages are more complicated than a simple pipe passing JSON
objects separated by new line. This removes inaccurate notes about
implementation from the documentation.

PR-URL: #17460
Fixes: #16491
Fixes: #17405
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@apapirovski
Copy link
Member

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

@seishun
Copy link
Contributor Author

seishun commented Apr 13, 2018

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 #16491 (comment)). But if the general opinion is that this is expected behavior, then I'm not going to oppose it.

@apapirovski apapirovski added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Apr 13, 2018
@apapirovski
Copy link
Member

@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
Copy link
Member

jasnell commented Jun 25, 2020

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.

@jasnell jasnell closed this as completed Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. libuv Issues and PRs related to the libuv dependency or the uv binding.
Projects
None yet
5 participants