process: fix reading zero-length env vars on win32 by addaleax · Pull Request #18463 · nodejs/node

addaleax · 2018-01-30T19:31:15Z

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.

Checklist

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

Affected core subsystem(s)

process

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.

bnoordhuis

LGTM and appreciate the good commit log.

Fishrock123

LGTM with CI: https://ci.nodejs.org/job/node-test-pull-request/12818/

indutny

LGTM, 🤦‍♀️ 🤦‍♂️ (we had similar errors in node_crypto.cc)

Work around nodejs/node#18463 by detecting the conditions under which the bug occurs and performing a simple operation that resets the error state if necessary.

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>

jasnell · 2018-02-02T17:51:37Z

Landed in 6acb1a3

gibfahn · 2018-02-12T16:32:28Z

@nodejs/lts see #14593 (comment), I'm inclined to include the fix into 8.10.0 (it's not needed for 6.x).

Please weigh in.

richardlau · 2018-02-12T17:47:56Z

@nodejs/lts see #14593 (comment), I'm inclined to include the fix into 8.10.0 (it's not needed for 6.x).
Please weigh in.

SGTM in light of discussions in nodejs/Release#308

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>

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [#16413](#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [#16413](#16413) * upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260) * re land npm 5.6.0 (Myles Borins) [#18625](#18625) * ICU 60 bump (Steven R. Loomis) [#16876](#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [#16130](#16130) * warn on invalid authentication tag length (Tobias Nießen) [#17566](#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [#18004](#18004) * use typed array stack as fast path (Anna Henningsen) [#17780](#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [#17273](#17273) * separate missing from default context (Andreas Madsen) [#17273](#17273) * rename initTriggerId (Andreas Madsen) [#17273](#17273) * deprecate undocumented API (Andreas Madsen) [#16972](#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [#16998](#16998) * add trace events to async_hooks (Andreas Madsen) [#15538](#15538) * set HTTPParser trigger to socket (Andreas Madsen) [#18003](#18003) * add provider types for net server (Andreas Madsen) [#17157](#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [#16495](#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [#17033](#17033) * module: * add builtinModules (Jon Moss) [#16386](#16386) * replace default paths in require.resolve() (cjihrig) [#17113](#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * add process.ppid (cjihrig) [#16839](#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [#16267](#16267) * add rawPacket in err of `clientError` event (XadillaX) [#17672](#17672) * better support for IPv6 addresses (Mattias Holmlund) [#14772](#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [#17662](#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [#18463](#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [#17478](#17478) * process: * improve unhandled rejection message (Madara Uchiha) [#17158](#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [#12860](#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [#17196](#17196) PR-URL: #18336

MylesBorins · 2018-03-20T16:18:40Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

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: nodejs#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>

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>

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: nodejs#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>

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [nodejs#16413](nodejs#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [nodejs#16413](nodejs#16413) * upgrade libuv to 1.19.1 (cjihrig) [nodejs#18260](nodejs#18260) * re land npm 5.6.0 (Myles Borins) [nodejs#18625](nodejs#18625) * ICU 60 bump (Steven R. Loomis) [nodejs#16876](nodejs#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [nodejs#16130](nodejs#16130) * warn on invalid authentication tag length (Tobias Nießen) [nodejs#17566](nodejs#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [nodejs#18004](nodejs#18004) * use typed array stack as fast path (Anna Henningsen) [nodejs#17780](nodejs#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [nodejs#17273](nodejs#17273) * separate missing from default context (Andreas Madsen) [nodejs#17273](nodejs#17273) * rename initTriggerId (Andreas Madsen) [nodejs#17273](nodejs#17273) * deprecate undocumented API (Andreas Madsen) [nodejs#16972](nodejs#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [nodejs#16998](nodejs#16998) * add trace events to async_hooks (Andreas Madsen) [nodejs#15538](nodejs#15538) * set HTTPParser trigger to socket (Andreas Madsen) [nodejs#18003](nodejs#18003) * add provider types for net server (Andreas Madsen) [nodejs#17157](nodejs#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [nodejs#16495](nodejs#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [nodejs#17033](nodejs#17033) * module: * add builtinModules (Jon Moss) [nodejs#16386](nodejs#16386) * replace default paths in require.resolve() (cjihrig) [nodejs#17113](nodejs#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * add process.ppid (cjihrig) [nodejs#16839](nodejs#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [nodejs#16267](nodejs#16267) * add rawPacket in err of `clientError` event (XadillaX) [nodejs#17672](nodejs#17672) * better support for IPv6 addresses (Mattias Holmlund) [nodejs#14772](nodejs#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [nodejs#17662](nodejs#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [nodejs#18463](nodejs#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [nodejs#17478](nodejs#17478) * process: * improve unhandled rejection message (Madara Uchiha) [nodejs#17158](nodejs#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [nodejs#12860](nodejs#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [nodejs#17196](nodejs#17196) PR-URL: nodejs#18336

addaleax added windows Issues and PRs related to the Windows platform. process Issues and PRs related to the process subsystem. lts-watch-v8.x labels Jan 30, 2018

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 30, 2018

addaleax force-pushed the windows-env-var-fix branch from 43801ea to 52d4e3bCompare January 30, 2018 19:32

bnoordhuis approved these changes Jan 30, 2018
View reviewed changes

addaleax mentioned this pull request Jan 30, 2018
Avoid relying on Node’s internals webpack/webpack#6398
Merged

Fishrock123 approved these changes Jan 30, 2018
View reviewed changes

indutny approved these changes Jan 30, 2018
View reviewed changes

tniessen approved these changes Jan 31, 2018
View reviewed changes

cjihrig approved these changes Jan 31, 2018
View reviewed changes

targos added dont-land-on-v4.x labels Jan 31, 2018

targos approved these changes Jan 31, 2018
View reviewed changes

targos removed dont-land-on-v4.x labels Jan 31, 2018

jasnell approved these changes Jan 31, 2018
View reviewed changes

TimothyGu approved these changes Feb 1, 2018
View reviewed changes

thefourtheye approved these changes Feb 1, 2018
View reviewed changes

BridgeAR approved these changes Feb 1, 2018
View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2018

jasnell closed this Feb 2, 2018

addaleax deleted the windows-env-var-fix branch February 2, 2018 18:56

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 4, 2018

richardlau mentioned this pull request Feb 12, 2018
Passing empty environment variables to child processes convert to 'undefined' using ConEmu + Node #14593
Closed

gibfahn added land-on-v8.x and removed lts-watch-v8.x labels Feb 18, 2018

addaleax mentioned this pull request Feb 18, 2018
test: add env undefined regression test #18529
Closed

MylesBorins mentioned this pull request Feb 21, 2018
v9.6.0 proposal #18902
Merged

addaleax mentioned this pull request Feb 27, 2018
The value of process.env.foo can sometimes be observed to be undefined after being assigned the value of '' #19048
Closed

MylesBorins added the backport-requested-v6.x label Mar 20, 2018

addaleax added backported-to-v6.x and removed backport-requested-v6.x labels Mar 20, 2018

addaleax mentioned this pull request Mar 20, 2018
[v6.x] test: check reading zero-length env vars #19484
Closed
3 tasks

MylesBorins mentioned this pull request Apr 13, 2018
v6.14.2 proposal #19996
Merged

addaleax mentioned this pull request Jun 18, 2018
Path.join, argument[0] is coming back undefined when explicitly passed in #21392
Closed

alexeykuzmin mentioned this pull request Jun 19, 2018
Node path.join fails on Windows 7 & Electron 2.0.2 - Possibly due to bug in Node 8.9.3, fixed in 8.10.0 electron/electron#13287
Closed

Navigation Menu

Uh oh!

Uh oh!

process: fix reading zero-length env vars on win32 - #18463

process: fix reading zero-length env vars on win32#18463
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:windows-env-var-fix

addaleax commented Jan 30, 2018

Uh oh!

bnoordhuis left a comment

Uh oh!

Fishrock123 left a comment

Uh oh!

indutny left a comment

Uh oh!

jasnell commented Feb 2, 2018

Uh oh!

gibfahn commented Feb 12, 2018•
edited
Loading

Uh oh!

richardlau commented Feb 12, 2018

Uh oh!

MylesBorins commented Mar 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

Uh oh!

Uh oh!

Conversation

addaleax commented Jan 30, 2018

Checklist

Affected core subsystem(s)

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Feb 2, 2018

Uh oh!

gibfahn commented Feb 12, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardlau commented Feb 12, 2018

Uh oh!

MylesBorins commented Mar 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

gibfahn commented Feb 12, 2018•
edited
Loading