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

[retrohacker]

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

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

[targos]

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]

I'm testing a fix

[targos]

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]

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]

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]

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