GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — process: fix reading zero-length env vars on win32 by addaleax · Pull Request #18463 · nodejs/node

Conversation

@addaleax

@addaleax addaleax commented Jan 30, 2018

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sokra, connorjclark, payload, gjasny, and freme reacted with hooray emoji
All reactions
  • 🎉 5 reactions
@addaleax 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 nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 30, 2018
@addaleax
process: fix reading zero-length env vars on win32
52d4e3b 
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.
@addaleax
addaleax force-pushed the windows-env-var-fix branch from 43801ea to 52d4e3b Compare January 30, 2018 19:32
bnoordhuis
bnoordhuis approved these changes Jan 30, 2018
View reviewed changes

@bnoordhuis bnoordhuis left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and appreciate the good commit log.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@addaleax addaleax mentioned this pull request Jan 30, 2018
Avoid relying on Node’s internals webpack/webpack#6398
Merged
Fishrock123
Fishrock123 approved these changes Jan 30, 2018
View reviewed changes

@Fishrock123 Fishrock123 left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
indutny
indutny approved these changes Jan 30, 2018
View reviewed changes

@indutny indutny left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
addaleax added a commit to addaleax/webpack that referenced this pull request Jan 30, 2018
@addaleax
Work around Node environment variable bug
6193e8c 
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.
addaleax added a commit to addaleax/webpack that referenced this pull request Jan 30, 2018
@addaleax
Work around Node environment variable bug
ba26e61 
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.
addaleax added a commit to addaleax/webpack that referenced this pull request Jan 30, 2018
@addaleax
Work around Node environment variable bug
8da8b93 
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.
tniessen
tniessen approved these changes Jan 31, 2018
View reviewed changes
cjihrig
cjihrig approved these changes Jan 31, 2018
View reviewed changes
@targos targos added dont-land-on-v4.x labels Jan 31, 2018
targos
targos approved these changes Jan 31, 2018
View reviewed changes
@targos targos removed dont-land-on-v4.x labels Jan 31, 2018
jasnell
jasnell approved these changes Jan 31, 2018
View reviewed changes
TimothyGu
TimothyGu approved these changes Feb 1, 2018
View reviewed changes
thefourtheye
thefourtheye approved these changes Feb 1, 2018
View reviewed changes
BridgeAR
BridgeAR approved these changes Feb 1, 2018
View reviewed changes
@BridgeAR BridgeAR added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Feb 1, 2018
jasnell pushed a commit that referenced this pull request Feb 2, 2018
@addaleax @jasnell
process: fix reading zero-length env vars on win32
6acb1a3 
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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@jasnell

jasnell commented Feb 2, 2018

Copy link
Copy Markdown
Member

Landed in 6acb1a3
addaleax reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this Feb 2, 2018
@addaleax
addaleax deleted the windows-env-var-fix branch February 2, 2018 18:56
@addaleax addaleax removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Feb 4, 2018
@richardlau richardlau mentioned this pull request Feb 12, 2018
Passing empty environment variables to child processes convert to 'undefined' using ConEmu + Node #14593
Closed
@gibfahn

gibfahn commented Feb 12, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@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.
addaleax, BridgeAR, and richardlau reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@richardlau

richardlau commented Feb 12, 2018

Copy link
Copy Markdown
Member

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gibfahn gibfahn added land-on-v8.x and removed lts-watch-v8.x labels Feb 18, 2018
gibfahn pushed a commit that referenced this pull request Feb 18, 2018
@addaleax @gibfahn
process: fix reading zero-length env vars on win32
f99aba1 
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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@addaleax addaleax mentioned this pull request Feb 18, 2018
test: add env undefined regression test #18529
Closed
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
@addaleax @MylesBorins
process: fix reading zero-length env vars on win32
582fa6a 
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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@addaleax @MylesBorins
process: fix reading zero-length env vars on win32
0f2a522 
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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@addaleax @MylesBorins
process: fix reading zero-length env vars on win32
cec3d1e 
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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
v9.6.0 proposal #18902
Merged
@addaleax 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
gibfahn added a commit that referenced this pull request Mar 6, 2018
@gibfahn
2018-03-06 Version 8.10.0 'Carbon' (LTS)
2fce636 
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
gibfahn added a commit that referenced this pull request Mar 7, 2018
@gibfahn
2018-03-06 Version 8.10.0 'Carbon' (LTS)
fc9ddd9 
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 MylesBorins added the backport-requested-v6.x label Mar 20, 2018
@MylesBorins

MylesBorins commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax added backported-to-v6.x and removed backport-requested-v6.x labels Mar 20, 2018
@addaleax addaleax mentioned this pull request Mar 20, 2018
[v6.x] test: check reading zero-length env vars #19484
Closed
3 tasks done
addaleax added a commit to addaleax/node that referenced this pull request Mar 21, 2018
@addaleax
process: fix reading zero-length env vars on win32
cec44be 
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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2018
@addaleax @MylesBorins
process: fix reading zero-length env vars on win32
d94fedc 
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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
@addaleax @MylesBorins
process: fix reading zero-length env vars on win32
0a4c79b 
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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
@addaleax @MylesBorins
process: fix reading zero-length env vars on win32
988cca8 
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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 13, 2018
v6.14.2 proposal #19996
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@addaleax @MayaLekova
process: fix reading zero-length env vars on win32
b8f94e2 
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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@gibfahn @MayaLekova
2018-03-06 Version 8.10.0 'Carbon' (LTS)
185ac06 
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 addaleax mentioned this pull request Jun 18, 2018
Path.join, argument[0] is coming back undefined when explicitly passed in #21392
Closed
@alexeykuzmin 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes
@jasnell jasnell jasnell approved these changes
@cjihrig cjihrig cjihrig approved these changes
@tniessen tniessen tniessen approved these changes
@BridgeAR BridgeAR BridgeAR approved these changes
+5 more reviewers
@indutny indutny indutny approved these changes
@thefourtheye thefourtheye thefourtheye approved these changes
@Fishrock123 Fishrock123 Fishrock123 approved these changes
@TimothyGu TimothyGu TimothyGu approved these changes
@targos targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. windows Issues and PRs related to the Windows platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

15 participants

@addaleax @jasnell @gibfahn @richardlau @MylesBorins @indutny @bnoordhuis @thefourtheye @Fishrock123 @TimothyGu @targos @cjihrig @tniessen @BridgeAR @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.