Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[v6.x] test: check reading zero-length env vars #19484

Closed
wants to merge 1 commit into from
Closed

[v6.x] test: check reading zero-length env vars #19484

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Mar 20, 2018 •
edited
Loading

Backport the test from #18463 (6acb1a3) to make sure nothing accidentally introduces this bug to Node 6. Backport #18463

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

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. v6.x labels Mar 20, 2018
@MylesBorins
Copy link
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/13789/

@MylesBorins
Copy link
Contributor

This test is failing
https://ci.nodejs.org/job/node-test-binary-windows/15856/COMPILED_BY=vs2015-x86,RUNNER=win2012r2,RUN_SUBSET=1/console

not ok 208 parallel/test-process-env-windows-error-reset
  ---
  duration_ms: 0.256
  severity: fail
  stack: |-
    
    assert.js:84
      throw new assert.AssertionError({
      ^
    AssertionError: undefined === ''
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-process-env-windows-error-reset.js:13:10)
        at Module._compile (module.js:577:32)
        at Object.Module._extensions..js (module.js:586:10)
        at Module.load (module.js:494:32)
        at tryModuleLoad (module.js:453:12)
        at Function.Module._load (module.js:445:3)
        at Module.runMain (module.js:611:10)
        at run (bootstrap_node.js:387:7)
        at startup (bootstrap_node.js:153:9)
        at bootstrap_node.js:500:3

@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]>
@addaleax addaleax force-pushed the backport-v6.x-18463 branch from bc48d19 to cec44be Compare March 21, 2018 11:11
@addaleax
Copy link
Member Author

addaleax commented Mar 21, 2018 •
edited
Loading

Huh. I was under the impression that Node 6 wasn’t affected, but sure, did a full backport now.

CI: https://ci.nodejs.org/job/node-test-pull-request/13805/

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
Copy link
Contributor

landed in d94fedc

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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@XadillaX XadillaX XadillaX approved these changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@addaleax @MylesBorins @XadillaX @richardlau @nodejs-github-bot