child_process: spawnSync crashes trying to terminate setuid child because of maxBuffer exceeded

[kolontsov]

• Version: v13.8.0, v12.16.0
• Platform: macOS 10.15.3, Ubuntu 16.04.6 (4.15.0-70-generic)
• Subsystem: child_process, src/spawn_sync.c

What steps will reproduce the bug?

$ cat test.c
#include <unistd.h>
#include <stdio.h>
int main() { setuid(0); while (1) printf("hello"); }

$ gcc -o test test.c; sudo chown root test; sudo chmod 4755 test

$ node -e "require('child_process').spawnSync('./test')"

OR

$ node -e "require('child_process').spawnSync('sudo', ['bash', '-c', 'ls -lR /'])"

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

I expect to get ENOBUF (because of maxBuffer reached) and/or EPERM (because child process cannot be killed), so I can handle it somehow - but not crash.

What do you see instead?

node[68984]: ../src/spawn_sync.cc:611:void node::SyncProcessRunner::Kill(): Assertion `r >= 0 || r == UV_ESRCH' failed.
 1: 0x100ba0c4a node::Abort() (.cold.1) [/usr/local/bin/node]
 2: 0x100084961 node::FatalError(char const*, char const*) [/usr/local/bin/node]
 3: 0x100084719 node::AppendExceptionLine(node::Environment*, v8::Local<v8::Value>, v8::Local<v8::Message>, node::ErrorHandlingMode) [/usr/local/bin/node]
 4: 0x10010657d node::SyncProcessRunner::Kill() [/usr/local/bin/node]
 5: 0x1006cb116 uv__stream_io [/usr/local/bin/node]
 6: 0x1006d23a8 uv__io_poll [/usr/local/bin/node]
 7: 0x1006c2fa2 uv_run [/usr/local/bin/node]
 8: 0x10010598f node::SyncProcessRunner::TryInitializeAndRunLoop(v8::Local<v8::Value>) [/usr/local/bin/node]
 9: 0x100105674 node::SyncProcessRunner::Run(v8::Local<v8::Value>) [/usr/local/bin/node]
10: 0x100105513 node::SyncProcessRunner::Spawn(v8::FunctionCallbackInfo<v8::Value> const&) [/usr/local/bin/node]
11: 0x1001cb578 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [/usr/local/bin/node]
12: 0x1001cac02 v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/usr/local/bin/node]
13: 0x1001ca40e v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/usr/local/bin/node]
14: 0x1007503d9 Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit [/usr/local/bin/node]
zsh: abort      node -e "require('child_process').spawnSync('./test')"

Additional information

According to documentation, child_process.spawnSync() terminates child process if it's output is larger than maxBuffer. If child process can't be killed (because of setuid call), node crashes in CHECK() below https://github.com/nodejs/node/blob/master/src/spawn_sync.cc#L611:

    // If uv_kill failed with an error that isn't ESRCH, the user probably
    // specified an invalid or unsupported signal. Signal this to the user as
    // and error and kill the process with SIGKILL instead.
    if (r < 0 && r != UV_ESRCH) {
      SetError(r);

      r = uv_process_kill(&uv_process_, SIGKILL);
      CHECK(r >= 0 || r == UV_ESRCH);
    }

Shouldn't we also check for UV_EPERM?

[bnoordhuis]

#31768