Unicode characters are not properly parsed from NODE_OPTIONS on Windows

[connor4312]

• Version: verified on 14.5.0 / 12.14.1 / 10.15.2
• Platform: Windows 1909
• Subsystem:

What steps will reproduce the bug?

1. Create a file foó.js containing console.log("in foó");, and an bar.js with console.log("in bar");
2. Create a file index.js containing

require('child_process').spawn('node', ['bar.js'], {
  cwd: __dirname,
  env: { HELLO: 'cónnór', NODE_OPTIONS: '--require "./foó.js"' },
  stdio: 'inherit'
})

3. Run index.js

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

Every time

What is the expected behavior?

Output like:

> node index.js
in foo
in bar

What do you see instead?

> node index.js
internal/modules/cjs/loader.js:796
    throw err;
    ^

Error: Cannot find module './fo�.js'
Require stack:
- internal/preload
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:793:17)
    at Function.Module._load (internal/modules/cjs/loader.js:686:27)
    at Module.require (internal/modules/cjs/loader.js:848:19)
    at Module._preloadModules (internal/modules/cjs/loader.js:1133:12)
    at loadPreloadModules (internal/bootstrap/pre_execution.js:443:5)
    at prepareMainThreadExecution (internal/bootstrap/pre_execution.js:62:3)
    at internal/main/run_main_module.js:7:1 {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ 'internal/preload' ]
}

Additional information

• This does work Linux and OSX
• This happens irrespective of setting shell: true/false in spawn()
• I seem to be able to pass unicode characters through other variables and read them, e.g. console.log(process.env.HELLO) in the above example prints out fine
• For kicks I tried replacing the accented character with \\\\xF3, but it doesn't look like Node tries to parse these from the string.

References: microsoft/vscode-js-debug#563

[richardlau]

cc @nodejs/platform-windows

[bnoordhuis]

NODE_OPTIONS is parsed by credentials::SafeGetenv(), which calls getenv() from libc/msvcrt, and that function may or may not decode multi-byte sequences properly because... well, it's complicated.

Switching to uv_os_getenv() should fix it because that function always performs proper decoding. Untested, but the fix would look like this:

diff --git a/src/node_credentials.cc b/src/node_credentials.cc
index d552a50172..e77d0378b2 100644
--- a/src/node_credentials.cc
+++ b/src/node_credentials.cc
@@ -57,9 +57,23 @@ bool SafeGetenv(const char* key, std::string* text, Environment* env) {
   }
 
   {
-    Mutex::ScopedLock lock(per_process::env_var_mutex);
-    if (const char* value = getenv(key)) {
-      *text = value;
+    MaybeStackBuffer<char, 256> value;
+    size_t size = 256;  // TODO(bnoordhuis) DRY, re-use kStackStorageSize here.
+    int rc;
+
+    {
+      Mutex::ScopedLock lock(per_process::env_var_mutex);
+      rc = uv_os_getenv(key, *value, &size);
+    }
+
+    if (rc == UV_ENOBUFS) {
+      value.AllocateSufficientStorage(size);
+      Mutex::ScopedLock lock(per_process::env_var_mutex);
+      rc = uv_os_getenv(key, *value, &size);
+    }
+
+    if (rc >= 0) {
+      *text = *value;
       return true;
     }
   }

[ariccio]

I stumbled on this while opening an issue (facebook/create-react-app#13117) and digging to make sure I understood everything.
I'm just here to say I'm a bit surprised that you use a mutex for uv_os_getenv on all platforms. While the libuv docs say Warning This function is not thread safe., I do actually suspect the windows implementation is threadsafe!

If you compare the windows impl with other OSs, I can see the other impls use classic getenv, which is obviously unsafe. But the Windows implementation uses GetEnvironmentVariable, which (like getenv_s) is totally threadsafe. If you read through it, you may be a bit concerned with the SetLastError/GetLastError calls, which is a patch for the kind of thing we've all seen cause issues elsewhere, but that should be thread safe too, since the last error is per-thread (in the TEB, not PEB), not per-process. My windows machine is out of order right now, so I'm relying on ReactOS sources instead of GHIDRA, but it does look like there are locks internally, as I'd expect... so this should be redundant?

This probably makes little practical difference. But locks are locks, I bet someone, somewhere, would see a benefit from eliminating it.