unix: catch some cases of watching fd twice #1851

bnoordhuis · 2018-05-28T14:51:58Z

Libuv does not support multiple handles watching the same file
descriptor. That condition is caught by an assert but it's detached
from the call site and therefore not always trivial to track down.

This commit turns cases where we can easily detect duplicates into
runtime UV_EEXIST errors. More work is needed to catch all cases.

Partially addresses #1172.

cc @gireeshpunathil

bnoordhuis · 2018-05-28T14:53:47Z

src/unix/pipe.c

@@ -134,6 +134,9 @@ void uv__pipe_close(uv_pipe_t* handle) {
 int uv_pipe_open(uv_pipe_t* handle, uv_file fd) {
  int err;

+  if (uv__fd_exists(handle->loop, fd))
+    return UV_EEXIST;


Are runtime errors what we want or should they be asserts? It's ultimately a programmer bug because said programmer is trying to do something that libuv doesn't support - but if it's an assert, we can't test it easily. :-)

I think Node.js would prefer if it errored rather than asserted so we can choose what action to take. :D

I think a runtime error would be better.

gireeshpunathil · 2018-05-29T07:44:03Z

I tested this patch with the reproduce (in mac) and got a better in-context error. I guess this is clean.

without patch:
#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

with patch:
#node -e "new require('net').Socket({fd: 8}).write('g')"

net.js:264
    this._handle.open(fd);
                 ^

Error: EEXIST: file already exists, uv_pipe_open
    at new Socket (net.js:264:18)
    at Object.Socket (net.js:226:41)
    at [eval]:1:20
    at Script.runInThisContext (vm.js:89:20)
    at Object.runInThisContext (vm.js:286:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at evalScript (internal/bootstrap/node.js:540:27)
    at startup (internal/bootstrap/node.js:214:9)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:573:3)
#

gireeshpunathil · 2018-05-29T07:44:12Z

LGTM

santigimeno

LGTM with a question

santigimeno · 2018-05-29T16:06:40Z

src/unix/core.c

@@ -927,6 +927,11 @@ int uv__io_active(const uv__io_t* w, unsigned int events) {
 }


+int uv__fd_exists(uv_loop_t* loop, int fd) {
+  return (unsigned) fd < loop->nwatchers && loop->watchers[fd] != NULL;


why the (unsigned)?

Avoids an explicit check for values < 0, the cast wraps them around to a value > INT_MAX (which no fd can be and hence loop->nwatchers can't be either.)

santigimeno · 2018-05-29T16:19:43Z

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/885/

Libuv does not support multiple handles watching the same file descriptor. That condition is caught by an assert but it's detached from the call site and therefore not always trivial to track down. This commit turns cases where we can easily detect duplicates into runtime `UV_EEXIST` errors. More work is needed to catch _all_ cases. Partially addresses libuv#1172. PR-URL: libuv#1851 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

bnoordhuis · 2018-05-30T11:27:21Z

Landed in 2256be0. Opened #1856 to address the (not new, not caused by this PR) getaddrinfo_fail_sync failures.

There is already a check in place to prevent stdin and the IPC channel from sharing a file descriptor. This commit adds a similar check to stdout and stderr. Refs: libuv/libuv#1851 Refs: libuv/libuv#1897

There is already a check in place to prevent stdin and the IPC channel from sharing a file descriptor. This commit adds a similar check to stdout and stderr. Refs: libuv/libuv#1851 Refs: libuv/libuv#1897 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>

So it doesn't fail when creating a socket whose `fd` is already being watched. Test that functionality now inside a cluster worker. Refs: libuv/libuv#1851 Refs: libuv/libuv#1897 PR-URL: nodejs#21466 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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>

There is already a check in place to prevent stdin and the IPC channel from sharing a file descriptor. This commit adds a similar check to stdout and stderr. Refs: libuv/libuv#1851 Refs: libuv/libuv#1897 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>

So it doesn't fail when creating a socket whose `fd` is already being watched. Test that functionality now inside a cluster worker. Refs: libuv/libuv#1851 Refs: libuv/libuv#1897 PR-URL: #21466 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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>

There is already a check in place to prevent stdin and the IPC channel from sharing a file descriptor. This commit adds a similar check to stdout and stderr. Refs: libuv/libuv#1851 Refs: libuv/libuv#1897 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>

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>

There is already a check in place to prevent stdin and the IPC channel from sharing a file descriptor. This commit adds a similar check to stdout and stderr. Backport-PR-URL: #24103 Refs: libuv/libuv#1851 Refs: libuv/libuv#1897 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>

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>

bnoordhuis commented May 28, 2018

View reviewed changes

cjihrig approved these changes May 28, 2018

View reviewed changes

santigimeno approved these changes May 29, 2018

View reviewed changes

bnoordhuis force-pushed the bug1172 branch from 92638d2 to 2256be0 Compare May 30, 2018 11:26

bnoordhuis closed this May 30, 2018

bnoordhuis deleted the bug1172 branch May 30, 2018 11:26

bnoordhuis merged commit 2256be0 into libuv:v1.x May 30, 2018

gireeshpunathil mentioned this pull request Sep 22, 2018

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

Closed

theanarkh mentioned this pull request Jun 15, 2022

Wrapping existing net.Socket does not work nodejs/node#37873

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unix: catch some cases of watching fd twice #1851

unix: catch some cases of watching fd twice #1851

bnoordhuis commented May 28, 2018

bnoordhuis May 28, 2018

Fishrock123 May 28, 2018

cjihrig May 28, 2018

gireeshpunathil commented May 29, 2018

gireeshpunathil commented May 29, 2018

santigimeno left a comment

santigimeno May 29, 2018

bnoordhuis May 29, 2018

santigimeno commented May 29, 2018

bnoordhuis commented May 30, 2018

unix: catch some cases of watching fd twice #1851

unix: catch some cases of watching fd twice #1851

Conversation

bnoordhuis commented May 28, 2018

bnoordhuis May 28, 2018

Choose a reason for hiding this comment

Fishrock123 May 28, 2018

Choose a reason for hiding this comment

cjihrig May 28, 2018

Choose a reason for hiding this comment

gireeshpunathil commented May 29, 2018

gireeshpunathil commented May 29, 2018

santigimeno left a comment

Choose a reason for hiding this comment

santigimeno May 29, 2018

Choose a reason for hiding this comment

bnoordhuis May 29, 2018

Choose a reason for hiding this comment

santigimeno commented May 29, 2018

bnoordhuis commented May 30, 2018