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.