Node: No error was thrown when spawn command was existed but non-executable

[chinesedfan]

• Version:

10.2.0/11.12.0

• Platform:

Darwin localhost 16.7.0 Darwin Kernel Version 16.7.0: Thu Jun 15 17:36:27 PDT 2017; root:xnu-3789.70.16~2/RELEASE_X86_64 x86_64

• Details:

Suppose we have following test script and file notexe exist in the same folder. But notexe has no executable permission.

const spawn = require('child_process').spawn;
const path = require('path');

function test(name) {
    console.log('===', name);

    try {
        const child = spawn(name);
        console.log('spawn.returned - child.stdin', !!child.stdin);
    } catch (e) {
        console.log('spawn.error', e.message);
    }
}

// always spawn with child.stdin equals true, and crash with ENOENT
test('notexe');  // can't find in PATH
test('/notexe'); // can't find in root folder

// in Node 8.9.0, spawn throws EACCES and no crash
// in Node 10.2.0/11.12.0, spawn throws no error with child.stdin equals false, and crash
test('./notexe');
test(path.resolve(__dirname, './notexe'));

So, why did Node 10+ change spawn behaviors? On purpose or regressions?

[targos]

IMO going from a throw to an emitted error is a good thing, since spawn is supposed to be async.

Let me show what I think is a real bug in Node.js.

With this script located in /tmp/child_process, and an empty, non-executable file /tmp/child_process/y:

'use strict';

const { spawn } = require('child_process');
const { existsSync } = require('fs');
const { resolve } = require('path');

function test(path) {
  const exists = existsSync(path);
  const child = spawn(path);
  const hasStdin = !!child.stdin;
  child.on('error', (e) => {
    console.log(`Path: "${path}".\nExists: ${exists}.\nHas stdin: ${hasStdin}\n\n`);
  });
}

test('x');
test('/tmp/child_process/x');
test('./x');
test('y');
test('/tmp/child_process/y');
test('./y');

Path: "x".
Exists: false.
Has stdin: true


Path: "/tmp/child_process/x".
Exists: false.
Has stdin: true


Path: "./x".
Exists: false.
Has stdin: true


Path: "y".
Exists: true.
Has stdin: true


Path: "/tmp/child_process/y".
Exists: true.
Has stdin: false


Path: "./y".
Exists: true.
Has stdin: false

We have a clear inconsistency. If the file does not exist, the object returned by spawn always has the stdin property. If the file exists but is not executable, stdin is only present if the path doesn't start with ./ or /.

/cc @nodejs/child_process

[targos]

@bnoordhuis What do you think?

[cjihrig]

So, why did Node 10+ change spawn behaviors? On purpose or regressions?

The behavior change comes from https://github.com/nodejs/node/pull/19294, which began classifying EACCES as a runtime error (emitting vs. throwing).

If the file exists but is not executable, stdin is only present if the path doesn't start with ./ or /

I think this is more about the error that's occurring. The two cases where "has stdin" is false are both EACCES, while the rest are ENOENT.

ENOENT gets special treatment here. Since that comment was originally written, we've started handling UV_EAGAIN and UV_EACCES too, but they should be treated the same as UV_ENOENT in my opinion.

We can:

• Remove the special handling of UV_ENOENT, and return all runtime errors. This is definitely a breaking change, as it makes one test fail. I prefer this option because that comment acknowledges its own silliness, and it's kind of wasteful to set up stdio for no reason. I'm willing to PR this change.
• Include UV_EACCES and UV_EAGAIN in the special handling with UV_ENOENT. I didn't test this variation. It's also a breaking change, even if our test suite doesn't catch it.
• Do something else, including nothing.

[cjihrig]

Actually, on second thought, we probably do want to setup stdio to prevent situations like https://github.com/nodejs/help/issues/1769.

[gireeshpunathil]

having stdio setup in all runtime error scenarios (though am unable to figure out what needs to be done in the code to meet this) looks to be the right way forward for me. That way, from programmer's perspective, they could meaningfully tap both stdio and error streams of the child without breaking the program.