Skip to content

#6320 (fix for #6305) is flawed, because it may expose incorrect environment-variable values via Get-Item #6460

Description

A follow-up from #6305 (comment), as requested by Travis Plunk (@TravisEz13):

#6320 attempts to fix #6305, but only does so superficially:

  • while it fixes the immediate problem by preventing an error from surfacing when you use Get-item env: while case-variant environment variables are unexpectedly defined,

  • it can end up exposing an effectively hidden value that neither $env:<name> nor [environment]::GetEnvironmentVariable('<name>') see intermittently; in other words: Get-Item env:foo may intermittently report an incorrect value for $env:foo

Steps to reproduce

Note: Run this in PowerShell Core on Windows, with Node.js installed and #6320 applied (without it, Get-Item will fail, complaining about a duplicate key).

$env:FOO = 'A'
$tries = 0
do {
  Write-Host '.' -NoNewline
  ++$tries
  $res = node -pe @'
    env = {}
    env.fOo = 'B' // redefine with a case variant name and different value
    env.FOO = process.env.FOO // also include the original case variant with its original value.
    // Contrast $env:foo with Get-Item env:foo
    require('child_process').execSync(\"pwsh -nop -command $env:foo; (Get-Item env:foo).Value\", { env: env }).toString()
'@
} while ($res[0] -eq $res[1])

"
Values differed after $tries tries: '$($res[0])' vs. '$($res[1])'"

Expected behavior

The loop should never exit, because $env:foo and Get-Item env:foo should always report the same value.

Actual behavior

Because Get-Item env:foo intermittently reports B rather than A, the loop exits after a few tries with, e.g.:

Values differed after 3 tries: 'A' vs. 'B'

Fix

As proposed in #6305 (comment), in https://github.com/PowerShell/PowerShell/pull/6320/files#diff-1bee4b279bb56aca25fb479994641529, replace:

  // Windows only: duplicate key (variable name that differs only in case)
  // NOTE: Even though this shouldn't happen, it can, e.g. when npm
  //       creates duplicate environment variables that differ only in case -
  //       see https://github.com/PowerShell/PowerShell/issues/6305.
  //       However, because retrieval *by name* later is invariably
  //       case-Insensitive, in effect only a *single* variable exists.
  //       We simply ask Environment.GetEnvironmentVariable() which value is
  //       the effective one, and use that.
providerTable.TryAdd((string)entry.Key, entry);

(Note: the comments, taken from my original proposal, which still stands below, don't match the code - the comments explain what the code should do, but doesn't - see below.)

with:

try {
    providerTable.Add((string)entry.Key, entry);
} catch (System.ArgumentException) { // Windows only: duplicate key (variable name that differs only in case)
    // NOTE: Even though this shouldn't happen, it can, e.g. when npm
    //       creates duplicate environment variables that differ only in case -
    //       see https://github.com/PowerShell/PowerShell/issues/6305.
    //       However, because retrieval *by name* later is invariably
    //       case-INsensitive, in effect only a *single* variable exists.
    //       We simply ask Environment.GetEnvironmentVariable() for the effective value 
    //       and use that.
    providerTable[(string)entry.Key] = new DictionaryEntry((string)entry.Key, Environment.GetEnvironmentVariable((string)entry.Key));
}

Environment data

PowerShell Core post-v6.0.2 with #6320 applied.

Activity

  1. TravisEz13 commented on Mar 23, 2018

    @TravisEz13
    Member

    I see. Thank for the detail repro and explanation. Do you want to submit a PR?

  2. mklement0 commented on Mar 24, 2018

    @mklement0
    ContributorAuthor

    Travis Plunk (@TravisEz13): Sure; please see #6489. Note that I've tweaked the fix slightly to recreate the entry when(ever) the effective value is encountered, so that the name casing reflects the duplicate that took effect; however, if duplicates define the very same value, it is unspecified which casing is reflected, but in practice that shouldn't matter.

  3. mklement0 commented on Mar 30, 2018

    @mklement0
    ContributorAuthor

    Fixed by #6489.

  4. added a commit that references this issue on Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

Issue-BugIssue has been identified as a bug in the productResolution-FixedThe issue is fixed.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions