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

a simple script to crash Node.js with an assertion failure: (loop->watchers[w->fd] == w) #3604

Closed
pmq20 opened this issue Oct 30, 2015 · 25 comments
Assignees
Labels
confirmed-bug Issues with confirmed bugs. 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. net Issues and PRs related to the net subsystem.

Comments

@pmq20
Copy link
Contributor

pmq20 commented Oct 30, 2015

This crash was reported several times in [1] [2] [3] [4], I also devised a simple script to reproduce the crash as below.

var fd = 3;
while (fd<1000) {
  try {
    var stream = new net.Socket({
      fd: fd,
      readable: false,
      writable: true
    });
    stream.on('error', function() {});
    stream.write('might crash');
  } catch(e) {}
  fd += 1;
}

[1] joyent/libuv#838
[2] joyent/libuv#1348
[3] jeremycx/node-LDAP#33
[4] serialport/node-serialport#262

@pmq20 pmq20 changed the title a simple script to crash Node.js with an assertion failure a simple script to crash Node.js with (loop->watchers[w->fd] == w) Oct 30, 2015
@pmq20 pmq20 changed the title a simple script to crash Node.js with (loop->watchers[w->fd] == w) a simple script to crash Node.js with an assertion failure: (loop->watchers[w->fd] == w) Oct 30, 2015
@Fishrock123
Copy link
Contributor

@pmq20 What Node version and which OS? I can't repro on Node v4.2.1 and OS X 10.10.5

@pmq20
Copy link
Contributor Author

pmq20 commented Oct 30, 2015

@Fishrock123 I'm using Node.js v4.2.1 and Mac OS X 10.11 (15A284)

@Fishrock123
Copy link
Contributor

@pmq20 My machine has a Quad-core 2.6ghz i7 -- perhaps the test isn't race-y enough?

@pmq20
Copy link
Contributor Author

pmq20 commented Oct 30, 2015

@Fishrock123 I have revised the script to loop on fd, could you try it again?

@pmq20
Copy link
Contributor Author

pmq20 commented Oct 30, 2015

image

@Fishrock123
Copy link
Contributor

Reproed. Thanks.

Jeremiahs-MacBook-Pro:here-be-dragons Jeremiah$ node
> var fd = 3
undefined
> while (fd<1000) {
... try {
..... var stream = new net.Socket({
....... fd,
....... readable: false,
....... writable: true
....... })
..... stream.on('error', ()=>{})
..... stream.write('might crash')
..... } catch(e) {}
... fd++
... }
Assertion failed: (loop->watchers[w->fd] == w), function uv__io_stop, file ../deps/uv/src/unix/core.c, line 856.
Abort trap: 6

@Fishrock123 Fishrock123 added confirmed-bug Issues with confirmed bugs. net Issues and PRs related to the net subsystem. labels Oct 30, 2015
@indutny
Copy link
Member

indutny commented Oct 30, 2015

I'm not sure if this could be considered a bug. Libuv does not allow to watch the same file descriptors twice, and this is a manifestation of this behavior.

Most of the bugs that I have seen that was triggering loop->watchers[w->fd] == w in user-land, were due to poorly written C++ addons.

@pmq20
Copy link
Contributor Author

pmq20 commented Oct 30, 2015

@indutny I just thought pure JS code without any addons should not cause C-level panic under any circumstance

@indutny
Copy link
Member

indutny commented Oct 30, 2015

@pmq20 well, it probably should throw indeed. cc @bnoordhuis @saghul

pmq20 added a commit to pmq20/node that referenced this issue Nov 3, 2015
@saghul
Copy link
Member

saghul commented Nov 4, 2015

Ideally libuv itself should check if a fd is already being watched and scream with an error. Now, I remember this wasn't as straightforward as it sounds, but I'll take another look, since I might be remembering wrong...

@thefourtheye
Copy link
Contributor

Weird. I am able to reproduce this problem only in REPL.

@jupe
Copy link

jupe commented Mar 18, 2016

I've this same crash when using node-serialport. Is there any way to avoid this ?

@Fishrock123 Fishrock123 self-assigned this Jul 19, 2016
@Fishrock123 Fishrock123 added the libuv Issues and PRs related to the libuv dependency or the uv binding. label Jul 21, 2016
@Fishrock123
Copy link
Contributor

Ideally libuv itself should check if a fd is already being watched and scream with an error.

@saghul is there a libuv issue for this yet?

@saghul
Copy link
Member

saghul commented Jul 21, 2016

@Fishrock123 alas no. And it's not easy to write one (IIRC), that's why it hasn't been done yet.

@allbinmani
Copy link

FWIW, this is still reproducible with node 7.2.1 (OSX). I run into this assertion occasionally when using a custom nwjs-app that streams serial data, but since it's not deterministic I attribute my specific problems to a race condition elsewhere (not node/uv).
However, there were no mentions of node later than v4 here, so I thought it could be worth mentioning reproducibility on 7.2.1.

@sprajagopal
Copy link

sprajagopal commented Aug 8, 2017

I have this issue in node v4l2camera module
Node: v6.11.2

@Fishrock123 Fishrock123 added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Nov 23, 2017
@roberthartung
Copy link

Got the same bug in a webinterface using socket.io and express with ejs. As far as I can tell, it happens when I try to login from a different machine using firefore, but not from my computer using Chrome.

@mmjee
Copy link

mmjee commented Apr 10, 2018

This hit me today, I did nothing but run some HTTP requests one of which ended in a timeout, and then this showed up.

Is there a way to avoid this ?

Node v9.11.1

@gireeshpunathil
Copy link
Member

there are 2 issues with the (original) test case:

  • arbitrary file descriptors are fed to create streams.
  • some are bad, some are already in-use: either by Node.js or libuv

Smallest code to reproduce the issue:
#node -e "new require('net').Socket({fd: 8}).write('g')"

Assertion failed: (loop->watchers[w->fd] == w), function uv__io_stop, file ../deps/uv/src/unix/core.c, line 896.
Abort trap: 6

The fd # may not be 8 always, but in and around. In my MAC with a Node version it is 8, and in Linux it is 6. This fd is already opened and in use by Node, for signal watching.

A new stream object is created with this fd as the data source.
Attempt to insert the watcher into the watch queue did not succeed, as we index the watcher array with the fd number, and the signal watcher is already present at the index [8]
Then the write is attemted on the stream which erred - as the fd 6 is read only, the reading end of the signal pipe.
This caused the stream to be ended and the socket to be closed.
uv__io_stop is invoked that expects the watcher for this fd to be same as the watcher in the array at the index pointed to by fd, which is not, leading to the failure.

I believe we should ensure:

assert(loop->watchers[w->fd] == NULL)

while setting up a watcher too, to make the current assertion to be reasonable.

Doing so does not help the test case, but catches faults much earlier.

A standalone test case that does not involve arbitray fds in the picture:

$ cat 3604.js

var c = require('child_process')
var n = require('net')

if (process.argv[2] === 'c') {
var i = new n.Socket({fd: 0})
var o = new n.Socket({fd: 1})
i.on('data', (d) => {
  console.log(d.toString())
})
} else {
  var cp = c.spawn(process.execPath, [__filename, 'c'])
  cp.stdout.pipe(process.stdout);
  cp.stderr.pipe(process.stdout);
  cp.stdin.write('hello!\n')
}

$ node 3604

hello!

Assertion failed: (loop->watchers[w->fd] == w), function uv__io_stop, file ../deps/uv/src/unix/core.c, line 896.

@nodejs/libuv

@bnoordhuis
Copy link
Member

@gireeshpunathil There's already an open libuv issue about that, libuv/libuv#1172.

@gireeshpunathil
Copy link
Member

thanks @bnoordhuis - I missed that.

cjihrig added a commit to cjihrig/node that referenced this issue Jun 25, 2018
Notable changes:

- Building via cmake is now supported.
  PR-URL: libuv/libuv#1850
- Stricter checks have been added to prevent watching the same
  file descriptor multiple times.
  PR-URL: libuv/libuv#1851
  Refs: nodejs#3604
- An IPC deadlock on Windows has been fixed.
  PR-URL: libuv/libuv#1843
  Fixes: nodejs#9706
  Fixes: nodejs#7657
- uv_fs_lchown() has been added.
  PR-URL: libuv/libuv#1826
  Refs: nodejs#19868
- uv_fs_copyfile() sets errno on error.
  PR-URL: libuv/libuv#1881
  Fixes: nodejs#21329
- uv_fs_fchmod() supports -A files on Windows.
  PR-URL: libuv/libuv#1819
  Refs: nodejs#12803

PR-URL: nodejs#21466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Jun 25, 2018
Notable changes:

- Building via cmake is now supported.
  PR-URL: libuv/libuv#1850
- Stricter checks have been added to prevent watching the same
  file descriptor multiple times.
  PR-URL: libuv/libuv#1851
  Refs: #3604
- An IPC deadlock on Windows has been fixed.
  PR-URL: libuv/libuv#1843
  Fixes: #9706
  Fixes: #7657
- uv_fs_lchown() has been added.
  PR-URL: libuv/libuv#1826
  Refs: #19868
- uv_fs_copyfile() sets errno on error.
  PR-URL: libuv/libuv#1881
  Fixes: #21329
- uv_fs_fchmod() supports -A files on Windows.
  PR-URL: libuv/libuv#1819
  Refs: #12803

PR-URL: #21466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Sep 21, 2018

I am unable to recreate the crash in the current Node.js 10.9, 8.12, and master. Not exactly sure which commit fixed the issue.

I'm unable to reproduce on the most recent version of 6 also.

@jasnell
Copy link
Member

jasnell commented Sep 22, 2018

Closing. If someone is able to recreate this issue, on current versions, please weigh in and we can reopen.

@jasnell jasnell closed this as completed Sep 22, 2018
@gireeshpunathil
Copy link
Member

$ cat 3604.js

var net = require('net')
var fd = 3;
while (fd<1000) {
  try {
    var stream = new net.Socket({
      fd: fd,
      readable: false,
      writable: true
    });
    stream.on('error', function() {});
    stream.write('might crash');
  } catch(e) {console.log(e)}
  fd += 1;
}
$   node -v
v11.0.0-pre
$   node 3604.js
TypeError [ERR_INVALID_FD_TYPE]: Unsupported fd type: UNKNOWN
    at createHandle (net.js:113:9)
    at new Socket (net.js:264:20)
    at Object.<anonymous> (/home/gireesh/nr/upstream/node/3604.js:5:18)
    at Module._compile (internal/modules/cjs/loader.js:703:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:714:10)
    at Module.load (internal/modules/cjs/loader.js:613:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:552:12)
    at Function.Module._load (internal/modules/cjs/loader.js:544:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:756:12)
    at startup (internal/bootstrap/node.js:302:19)
{ Error: EEXIST: file already exists, uv_pipe_open
    at new Socket (net.js:266:24)
    at Object.<anonymous> (/home/gireesh/nr/upstream/node/3604.js:5:18)
    at Module._compile (internal/modules/cjs/loader.js:703:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:714:10)
    at Module.load (internal/modules/cjs/loader.js:613:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:552:12)
    at Function.Module._load (internal/modules/cjs/loader.js:544:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:756:12)
    at startup (internal/bootstrap/node.js:302:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:855:3) errno: -17, code: 'EEXIST', syscall: 'uv_pipe_open' }
...
Aborted (core dumped)

$ node -e "new require('net').Socket({fd: 8}).write('g')"
net.js:113
  throw new ERR_INVALID_FD_TYPE(type);
  ^

TypeError [ERR_INVALID_FD_TYPE]: Unsupported fd type: UNKNOWN
    at createHandle (net.js:113:9)
    at new Socket (net.js:264:20)
    at Object.Socket (net.js:225:41)
    at [eval]:1:20
    at Script.runInThisContext (vm.js:94:20)
    at Object.runInThisContext (vm.js:291:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (internal/modules/cjs/loader.js:703:30)
    at evalScript (internal/bootstrap/node.js:679:27)
    at startup (internal/bootstrap/node.js:284:9)

@jasnell - for the context, watching same fd twice now throws valid JS error instead of C land crash. The relevant change is libuv/libuv#1851 . The original test case was silently ignoring errors in the catch sink, and that is why probably it was not showing up anything.

Though the underlying issue is not resolved, this mitigation looks good in the context of current libuv design, and hence I agree this issue can be closed (other than hand-crafted fd situations, under normal JS programming cases this do not show up).

@harlentan
Copy link

I also get this crash in Nodejs v8.11.3.

MylesBorins pushed a commit to MylesBorins/node that referenced this issue Nov 5, 2018
Notable changes:

- Building via cmake is now supported.
  PR-URL: libuv/libuv#1850
- Stricter checks have been added to prevent watching the same
  file descriptor multiple times.
  PR-URL: libuv/libuv#1851
  Refs: nodejs#3604
- An IPC deadlock on Windows has been fixed.
  PR-URL: libuv/libuv#1843
  Fixes: nodejs#9706
  Fixes: nodejs#7657
- uv_fs_lchown() has been added.
  PR-URL: libuv/libuv#1826
  Refs: nodejs#19868
- uv_fs_copyfile() sets errno on error.
  PR-URL: libuv/libuv#1881
  Fixes: nodejs#21329
- uv_fs_fchmod() supports -A files on Windows.
  PR-URL: libuv/libuv#1819
  Refs: nodejs#12803

PR-URL: nodejs#21466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Nov 11, 2018
Notable changes:

- Building via cmake is now supported.
  PR-URL: libuv/libuv#1850
- Stricter checks have been added to prevent watching the same
  file descriptor multiple times.
  PR-URL: libuv/libuv#1851
  Refs: #3604
- An IPC deadlock on Windows has been fixed.
  PR-URL: libuv/libuv#1843
  Fixes: #9706
  Fixes: #7657
- uv_fs_lchown() has been added.
  PR-URL: libuv/libuv#1826
  Refs: #19868
- uv_fs_copyfile() sets errno on error.
  PR-URL: libuv/libuv#1881
  Fixes: #21329
- uv_fs_fchmod() supports -A files on Windows.
  PR-URL: libuv/libuv#1819
  Refs: #12803

Backport-PR-URL: #24103
PR-URL: #21466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. 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. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.