From f99aba1f808d1ea083c75241d0bf2455b671f298 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Tue, 30 Jan 2018 20:27:17 +0100
Subject: [PATCH] process: fix reading zero-length env vars on win32
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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
1aa595e5bd64241451b3884d3029b9b9aa97c42d 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.

PR-URL: https://github.com/nodejs/node/pull/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>
---
 src/node.cc                                   |  2 ++
 .../test-process-env-windows-error-reset.js   | 22 +++++++++++++++++++
 2 files changed, 24 insertions(+)
 create mode 100644 test/parallel/test-process-env-windows-error-reset.js

diff --git a/src/node.cc b/src/node.cc
index cebcf100c80306..49657b0deedd11 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -3029,6 +3029,7 @@ static void EnvGetter(Local<Name> property,
 #else  // _WIN32
   node::TwoByteValue key(isolate, property);
   WCHAR buffer[32767];  // The maximum size allowed for environment variables.
+  SetLastError(ERROR_SUCCESS);
   DWORD result = GetEnvironmentVariableW(reinterpret_cast<WCHAR*>(*key),
                                          buffer,
                                          arraysize(buffer));
@@ -3077,6 +3078,7 @@ static void EnvQuery(Local<Name> property,
 #else  // _WIN32
     node::TwoByteValue key(info.GetIsolate(), 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
new file mode 100644
index 00000000000000..59e5f287d82346
--- /dev/null
+++ 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);
+}