process: fix reading zero-length env vars on win32

addaleax · MylesBorins · commit 988cca841ed0 · 2018-03-29T23:25:38.000-04:00
Up until now, Node did not clear the current error code attempting to read environment variables on Windows. Since checking the error code is the way we distinguish between missing and zero-length environment variables, this could lead to a false positive when the error code was still tainted. In the simplest case, accessing a missing variable and then a zero-length one would lead Node to believe that both calls yielded an error. Before: > process.env.I=''; process.env.Q; process.env.I undefined > process.env.I=''; /*process.env.Q;*/ process.env.I '' After: > process.env.I=''; process.env.Q; process.env.I '' > process.env.I=''; /*process.env.Q;*/ process.env.I '' This only affects Node 8 and above, since before 1aa595e we always constructed a `v8::String::Value` instance for passing the lookup key to the OS, which in in turn always made a heap allocation and therefore reset the error code. Backport-PR-URL: #19484 PR-URL: #18463 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
diff --git a/src/node.cc b/src/node.cc
@@ -2799,6 +2799,7 @@ static void EnvGetter(Local<Name> property,
 #else  // _WIN32
   String::Value key(property);
   WCHAR buffer[32767];  // The maximum size allowed for environment variables.
+  SetLastError(ERROR_SUCCESS);
   DWORD result = GetEnvironmentVariableW(reinterpret_cast<WCHAR*>(*key),
                                          buffer,
                                          arraysize(buffer));
@@ -2846,6 +2847,7 @@ static void EnvQuery(Local<Name> property,
 #else  // _WIN32
   String::Value key(property);
   WCHAR* key_ptr = reinterpret_cast<WCHAR*>(*key);
+  SetLastError(ERROR_SUCCESS);
   if (GetEnvironmentVariableW(key_ptr, nullptr, 0) > 0 ||
       GetLastError() == ERROR_SUCCESS) {
     rc = 0;
diff --git a/test/parallel/test-process-env-windows-error-reset.js b/test/parallel/test-process-env-windows-error-reset.js
@@ -0,0 +1,22 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+
+// This checks that after accessing a missing env var, a subsequent
+// env read will succeed even for empty variables.
+
+{
+  process.env.FOO = '';
+  process.env.NONEXISTENT_ENV_VAR;
+  const foo = process.env.FOO;
+
+  assert.strictEqual(foo, '');
+}
+
+{
+  process.env.FOO = '';
+  process.env.NONEXISTENT_ENV_VAR;
+  const hasFoo = 'FOO' in process.env;
+
+  assert.strictEqual(hasFoo, true);
+}
