Skip to content

Commit

Permalink
process: fix reading zero-length env vars on win32
Browse files Browse the repository at this point in the history
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.

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>
  • Loading branch information
addaleax authored and gibfahn committed Feb 18, 2018
1 parent 2a2c881 commit f99aba1
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 0 deletions.
2 changes: 2 additions & 0 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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;
Expand Down
22 changes: 22 additions & 0 deletions test/parallel/test-process-env-windows-error-reset.js
Original file line number Diff line number Diff line change
@@ -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);
}

0 comments on commit f99aba1

Please sign in to comment.