Fixes NODE_OPTIONS when spaces are escaped

[arcanis]

This PR fixes #12971. It adds a way to properly escape spaces when found within NODE_OPTIONS. The previous behavior was problematic because there was no way to use --require with paths containing spaces.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[addaleax]

Can you maybe also add tests for what happens when a backslash is not followed by a space character? In particular, I’m thinking about Windows-style paths with \ in them

[arcanis]

@addaleax will add a test - with this PR such paths would have to be escaped (C:\foo will be interpreted as C:foo), would that be acceptable? There's unfortunately a slight risk to break something.

[addaleax]

@arcanis Yes, that would make this a breaking change in its current form…

[arcanis]

@addaleax would it be semver-minor and acceptable if I was instead to add support for double quotes? Basically, doing the following:

NODE_OPTIONS='--require foo bar'
# execArgv=['--require', 'foo', 'bar']

NODE_OPTIONS='--require "foo bar"'
# execArgv=['--require', 'foo bar']

NODE_OPTIONS='--require foo\ bar'
# execArgv=['--require', 'foo\\', 'bar']

NODE_OPTIONS='--require C:\a\b\c'
# execArgv=['--require', 'C:\\a\\b\\c']

NODE_OPTIONS='--require "foo\ bar"'
# execArgv=['--require', 'foo bar']

NODE_OPTIONS='--require "foo\\ bar"'
# execArgv=['--require', 'foo\\ bar']

[addaleax]

@arcanis I guess the question is whether people currently (successfully) use quotes for NODE_OPTIONS – I can’t really imagine why, but there could be cases where it gets tricky?

(I’m also kinda surprised by your first example, I’d probably expect '--require "foo bar"' to work like shell quoting, i.e. foo bar as a single argument to --require…)

In any case, I’m not more of an authority on “how people use node features” than anybody else, and other people can weigh in too.

[arcanis]

(I’m also kinda surprised by your first example, I’d probably expect '--require "foo bar"' to work like shell quoting, i.e. foo bar as a single argument to --require…)

My bad, it's a typo 😄

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/18346/

[Trott]

This change appears to break the test suite on Windows:

https://ci.nodejs.org/job/node-test-binary-windows/21273/COMPILED_BY=vs2017,RUNNER=win2008r2-vs2017,RUN_SUBSET=2/console:

20:11:12 not ok 75 parallel/test-cli-node-options
20:11:12   ---
20:11:12   duration_ms: 0.499
20:11:12   severity: fail
20:11:12   exitcode: 1
20:11:12   stack: |-
20:11:12     assert.js:753
20:11:12         throw newErr;
20:11:12         ^
20:11:12     
20:11:12     AssertionError [ERR_ASSERTION]: ifError got unwanted exception: Command failed: c:\workspace\node-test-binary-windows\Release\node.exe -e console.log("B")
20:11:12     internal/modules/cjs/loader.js:605
20:11:12         throw err;
20:11:12         ^
20:11:12     
20:11:12     Error: Cannot find module 'c:workspacenode-test-binary-windowstestfixturesprintA.js'
20:11:12         at Function.Module._resolveFilename (internal/modules/cjs/loader.js:603:15)
20:11:12         at Function.Module._load (internal/modules/cjs/loader.js:529:25)
20:11:12         at Module.require (internal/modules/cjs/loader.js:658:17)
20:11:12         at Module._preloadModules (internal/modules/cjs/loader.js:843:12)
20:11:12         at preloadModules (internal/bootstrap/node.js:731:7)
20:11:12         at startup (internal/bootstrap/node.js:282:9)
20:11:12         at bootstrapNodeJSCore (internal/bootstrap/node.js:876:3)
20:11:12     
20:11:12         at ChildProcess.exithandler (child_process.js:294:12)
20:11:12         at ChildProcess.emit (events.js:182:13)
20:11:12         at maybeClose (internal/child_process.js:969:16)
20:11:12         at Process.ChildProcess._handle.onexit (internal/child_process.js:257:5)
20:11:12   ...

[arcanis]

@Trott This is related to the comment left by @addaleax here #24065 (review). There are three possible solutions:

• fix the tests to double-escape the path (replace c:\foo\bar by c:\\foo\\bar)
• remove support for escaping non-space and non-backslash characters (meaning that \a would stay as \a, and \\ would be collapsed into \)
• make the parsing logic more complex to support pseudo-strings (c:\foo\bar would be parsed as-is because not quoted, and "c:\foo\bar" would be interpreted as c:foobar because quoted)

I think I'd lean towards either the second or third solutions .. they're not perfect, but changing behavior like this even in a major might be potentially painful otherwise.

[jdalton]

I made my way here because I tried to use NODE_OPTIONS with--require with a quoted path and noticed Node didn't handle it. Quotes where the thing I reached for.

[arcanis]

I'd be willing to make extra changes, I just need to know what the consensus is. Which one of the three options described above look the best?

[jdalton]

My preference is for seeing if the following is doable:

make the parsing logic more complex to support pseudo-strings

[arcanis]

The latest commit changes the implementation to parse a JSON-like string (JSON-like because it really only take " and \ into consideration). It has a better backward compatibility (the only break would be if someone was using " within NODE_OPTIONS), and fixes #26521.

[himself65]

we have SplitString(), why don't just fix then use it?

[arcanis]

I'm not sure that SplitString should contain an actual parsing logic, or double quote/backslash support, does it?

[arcanis]

For reference this is the other place where SplitString is used - while it wouldn't be broken by using the same logic, in this case it sounds like it would be slightly overkill:

node/src/node_v8_platform-inl.h

Line 122 in 617f055

SplitString(per_process::cli_options->trace_event_categories, ',');

[himself65]

But I think it is necessary to abstract common functions like SplitString and we should maintain these functions instead of hacking code everywhere

[arcanis]

If they have the same semantical requirements I'm all for it, but in this case it just doesn't seem the case? I can abstract this behavior in a separate function (SmartSplitString), but it'll eventually be used in a single place.

[mhdawson]

@arcanis I think having a common function is probably a good idea. How about SplitString check's the delimiter specified and does the new logic only if it is a space? In that way we still have a common function which will do the right thing if you use a space, but otherwise we don't have extra overhead?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/22528/

[addaleax]

Landed in 3ef1512, thanks for the PR and sorry it took so long to get it landed!

[rickyes]

@addaleax This PR doesn't seem to be backported to v10.x, can we still do this, related issues microsoft/vscode-js-debug#769

/cc @nodejs/releasers

[addaleax]

@rickyes Maybe you could open a manual backport PR and ping the release team there?

[rickyes]

backport in #35342