Repository navigation
#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:foomay 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
TravisEz13 commentedon Mar 23, 2018
I see. Thank for the detail repro and explanation. Do you want to submit a PR?
fix error in windows provider when the environment as an existing set…
mklement0 commentedon Mar 24, 2018
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.
Fix error in windows provider when the environment has accidental dup…
mklement0 commentedon Mar 30, 2018
Fixed by #6489.
Keep uptodate (#1)
Fix error in windows provider when the environment has accidental dup…