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

uv_close: Assertion `0' failed on child_process.execSync w/ Infinite maxBuffer #8096

Closed
retrohacker opened this issue Aug 13, 2016 · 7 comments · Fixed by #8312
Closed

uv_close: Assertion `0' failed on child_process.execSync w/ Infinite maxBuffer #8096

retrohacker opened this issue Aug 13, 2016 · 7 comments · Fixed by #8312
Labels
child_process Issues and PRs related to the child_process subsystem. confirmed-bug Issues with confirmed bugs.

Comments

@retrohacker
Copy link

  • Version: tested on 4.4.7 and 5.10.0
  • Platform: Linux 3.16.0-4-amd64 #1 SMP Debian 3.16.7-ckt25-2 (2016-04-08) x86_64 GNU/Linux
  • Subsystem: child_process and deps/uv/src/unix/core.c

Reproduce:

var cp = require('child_process')

cp.execSync('', { maxBuffer: Infinity })
node: ../deps/uv/src/unix/core.c:165: uv_close: Assertion `0' failed.
[1]    31628 abort      node index.js

Not that the async exec does not exert the same behaviour

var cp = require('child_process')

cp.exec('', { maxBuffer: Infinity })
@targos targos added the confirmed-bug Issues with confirmed bugs. label Aug 13, 2016
@targos
Copy link
Member

targos commented Aug 13, 2016

I can reproduce. This happens when invalid values are detected in ParseOptions.
For example: cp.exec('', {uid: 'abc'}) also aborts.

@targos
Copy link
Member

targos commented Aug 13, 2016

backtrace:

#0  0x00007fff95e71f06 in __pthread_kill () from /usr/lib/system/libsystem_kernel.dylib
#1  0x00007fff8b3a94ec in pthread_kill () from /usr/lib/system/libsystem_pthread.dylib
#2  0x00007fff8c4086df in abort () from /usr/lib/system/libsystem_c.dylib
#3  0x00007fff8c3cfdd8 in __assert_rtn () from /usr/lib/system/libsystem_c.dylib
#4  0x0000000100b92d42 in uv_close (handle=<optimized out>, close_cb=<optimized out>) at ../deps/uv/src/unix/core.c:168
#5  0x0000000100a23931 in node::SyncProcessRunner::CloseHandlesAndDeleteLoop (this=0x7fff5fbfdd10) at ../src/spawn_sync.cc:505
#6  0x0000000100a235ac in node::SyncProcessRunner::Run (this=0x7fff5fbfdd10, options=...) at ../src/spawn_sync.cc:426
#7  0x0000000100a23481 in node::SyncProcessRunner::Spawn (args=...) at ../src/spawn_sync.cc:354

@targos
Copy link
Member

targos commented Aug 13, 2016

I'm testing a fix

@targos targos added the child_process Issues and PRs related to the child_process subsystem. label Aug 13, 2016
@targos
Copy link
Member

targos commented Aug 13, 2016

diff --git a/src/spawn_sync.cc b/src/spawn_sync.cc
index 79f10a0..8cded31 100644
--- a/src/spawn_sync.cc
+++ b/src/spawn_sync.cc
@@ -501,7 +501,7 @@ void SyncProcessRunner::CloseHandlesAndDeleteLoop() {
     // Close the process handle when ExitCallback was not called.
     uv_handle_t* uv_process_handle =
         reinterpret_cast<uv_handle_t*>(&uv_process_);
-    if (!uv_is_closing(uv_process_handle))
+    if (!uv_is_closing(uv_process_handle) && uv_process_handle->type != 0)
       uv_close(uv_process_handle, nullptr);

     // Give closing watchers a chance to finish closing and get their close

This prevents the problem but it is probably not the correct fix.

cc @bnoordhuis @cjihrig

@targos
Copy link
Member

targos commented Aug 13, 2016

BTW without the abort, the thrown exception is not very informative:

> cp.execSync('', {maxBuffer:Infinity})
Error: spawnSync /bin/sh EINVAL
    at exports._errnoException (util.js:1026:11)
    at spawnSync (child_process.js:466:20)
    at Object.execSync (child_process.js:522:13)
    at repl:1:4
    at sigintHandlersWrap (vm.js:22:35)
    at sigintHandlersWrap (vm.js:96:12)
    at ContextifyScript.Script.runInThisContext (vm.js:21:12)
    at REPLServer.defaultEval (repl.js:313:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)

Is there any reason we don't have a preliminary check of the options in JS ?

@bnoordhuis
Copy link
Member

The issue is that CloseHandlesAndDeleteLoop() tries to close handles that TryInitializeAndRunLoop() doesn't initialize when ParseOptions() fails.

Untested, but a quick fix would look like this:

diff --git a/src/spawn_sync.cc b/src/spawn_sync.cc
index 79f10a0..9a9ec4a 100644
--- a/src/spawn_sync.cc
+++ b/src/spawn_sync.cc
@@ -422,8 +422,13 @@ Local<Object> SyncProcessRunner::Run(Local<Value> options) {

   CHECK_EQ(lifecycle_, kUninitialized);

-  TryInitializeAndRunLoop(options);
-  CloseHandlesAndDeleteLoop();
+  int r = ParseOptions(options);
+  if (r < 0) {
+    SetError(r);
+  } else {
+    TryInitializeAndRunLoop(options);
+    CloseHandlesAndDeleteLoop();
+  }

   Local<Object> result = BuildResultObject();

@@ -444,10 +449,6 @@ void SyncProcessRunner::TryInitializeAndRunLoop(Local<Value> options) {
     return SetError(UV_ENOMEM);
   CHECK_EQ(uv_loop_init(uv_loop_), 0);

-  r = ParseOptions(options);
-  if (r < 0)
-    return SetError(r);
-
   if (timeout_ > 0) {
     r = uv_timer_init(uv_loop_, &uv_timer_);
     if (r < 0)

That produces an unhelpful EINVAL error though, and it doesn't address the wider issue that SyncProcessRunner is sloppy with its bookkeeping. TryInitializeAndRunLoop() can still fail for other reasons.

@targos
Copy link
Member

targos commented Aug 14, 2016

@bnoordhuis It's more complex. AddStdioPipe (used in ParseOptions) needs uv_loop_ to be initialized.

cjihrig added a commit to cjihrig/node that referenced this issue Dec 25, 2016
This commit verifies that the child process handle is of the
correct type before trying to close it in
CloseHandlesAndDeleteLoop(). This catches the case where input
validation failed, and the child process was never actually
spawned.

Fixes: nodejs#8096
Fixes: nodejs#8539
Refs: nodejs#9722
PR-URL: nodejs#8312
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
cjihrig added a commit to cjihrig/node that referenced this issue Dec 25, 2016
This commit applies stricter input validation in
normalizeSpawnArguments(), which is run by all of the
child_process methods. Additional checks are added for spawnSync()
specific inputs.

Fixes: nodejs#8096
Fixes: nodejs#8539
Refs: nodejs#9722
PR-URL: nodejs#8312
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
cjihrig added a commit to cjihrig/node that referenced this issue Dec 25, 2016
This commit removes C++ checks from spawn() and spawnSync()
that are duplicates of the JavaScript type checking.

Fixes: nodejs#8096
Fixes: nodejs#8539
Refs: nodejs#9722
PR-URL: nodejs#8312
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 18, 2017
This commit verifies that the child process handle is of the
correct type before trying to close it in
CloseHandlesAndDeleteLoop(). This catches the case where input
validation failed, and the child process was never actually
spawned.

Fixes: nodejs#8096
Fixes: nodejs#8539
Refs: nodejs#9722
PR-URL: nodejs#8312
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 18, 2017
This commit applies stricter input validation in
normalizeSpawnArguments(), which is run by all of the
child_process methods. Additional checks are added for spawnSync()
specific inputs.

Fixes: nodejs#8096
Fixes: nodejs#8539
Refs: nodejs#9722
PR-URL: nodejs#8312
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 18, 2017
This commit removes C++ checks from spawn() and spawnSync()
that are duplicates of the JavaScript type checking.

Fixes: nodejs#8096
Fixes: nodejs#8539
Refs: nodejs#9722
PR-URL: nodejs#8312
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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. confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants