get-childitem env: fails on Windows when an environment variable is defined twice, cased differently

The problem:
get-childitem env: fails on Windows when an environment variable exists twice, each instance cased differently.

The scenario:
On Windows, set the environment variable NPM_CONFIG_PATH to control where the npm cache is created. And use npm to run unit tests, use powershell within a unit test, and from powershell invoke get-childitem env:. Note, it is common to use npm test to build and test a nodejs project.

It appears npm internally creates environment variables to manage state, and a lower-case variable npm_config_path is created. When powershell is invoked from the unit test, the environment variables NPM_CONFIG_PATH and npm_config_path are both set.

Steps to reproduce

On Windows, tested with npm v5.6.0 in my PATH.

Write-Host "******************** SETTING NPM_CONFIG_PATH ********************"
$null = mkdir env-test
cd env-test
$env:NPM_CONFIG_PATH = $PWD
Write-Host "******************** CREATING PACKAGE.JSON ********************"
& npm init --force --yes
Write-Host "******************** MODIFYING PACKAGE.JSON ********************"
$package = Get-Content package.json | ConvertFrom-Json
$package.scripts.test = "pwsh -command Get-ChildItem env:"
Set-Content -LiteralPath package.json -Encoding UTF8 -Value (ConvertTo-Json -InputObject $package -Depth 99)
Get-Content package.json
Write-Host "******************** TESTING ENV PS DRIVE, VIA NPM ********************"
& npm test

Expected behavior

PowerShell should gracefully handle the duplicate environment variable. The set builtin in cmd.exe handles it gracefully. .NET Environment.GetEnvironmentVariables() handles it gracefully.

Actual behavior

Get-ChildItem fails with "An item with the same key has already been added. Key: NPM_CONFIG_PATH"

Environment data

> $PSVersionTable
Name                           Value
----                           -----
PSVersion                      6.0.1
PSEdition                      Core
GitCommitId                    v6.0.1
OS                             Microsoft Windows 10.0.16299
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

Does not repro on macOS

Here is the fix we made for macOS/Linux:

PowerShell/src/System.Management.Automation/namespaces/EnvironmentProvider.cs

Line 185 in c1c5344

#if UNIX

Update: Linux/macOS actually support this scenario... Windows does not. This would only mask the issue NPM is creating.

We would need to verify that making this change for windows would not be breaking.

Travis Plunk (@TravisEz13)

Travis Plunk (@TravisEz13):

#6320 does fix the issue, but I suggest resolving it differently:

That npm even manages to create a separate variable that differs by case only is unfortunate - given that environment variable names are case-insensitive (but case-preserving) in all other contexts I am aware of.

In effect, however, there's only one environment variable on retrieval by name, and - irrespective of what case variation in the name was used last - ~~the last value assigned wins - from what I can tell.~~

Thus, I suggest sticking with the case-insensitive dictionary and simply replacing the value of the existing dictionary entry if a case variation is encountered in the process of enumerating Environment.GetEnvironmentVariables()

There is no benefit in listing all case variations of a given name - all it creates is confusion, given that the values of ~~earlier~~ [other] case variants are unreachable by name.

Michael Klement (@mklement0)

Michael Klement (@mklement0)
I agreed that there is little to no value retrieving both values. I'm open to the change if the cost of the change is justified, but the cost of the change is increased complexity and therefore maintenance.

Our provider remains case-insensitive, case-preserving. You can never assign two variables with the same case (due to dotnet behavior and we have tests to verify this.) I chose this implementation over the other implementations because it was simpler with the only side effect occurring during the scenario we are fixing.

Fixing this while preserving the case insensitive dictionary would require additional specialized logic for windows, while this fix removed the specialized logic for windows. I believe the side-effects are reasonable unless I'm incorrect about them being limited to the scenario being fixed.

Travis Plunk (@TravisEz13)

Travis Plunk (@TravisEz13): Fixing this in a Windows-friendly manner would be simple, I believe:
The only change needed is to replace:

providerTable.Add((string)entry.Key, entry);

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() which value is
    //       the effective one, and use that.
    providerTable[(string)entry.Key] = new DictionaryEntry((string)entry.Key, Environment.GetEnvironmentVariable((string)entry.Key));
}

Note:

This assumes that the current distinction between a case-sensitive (Unix) and case-INsensitive dictionary (Windows) is retained.
No attempt is made to decide which particular case variant among the duplicate names is the "real" one associated with the "winning" value - I don't think we need to worry about that.

I've chosen the above approach because I've come to realize that it's not necessarily the definition order that determines the values that wins; here's a simplified reproduction of the original problem:

$env:FOO = 'first'
node -pe @'
    env = {}
    env.PATH = process.env.PATH
    env.FOO = process.env.FOO
    env.foo = 'last'
    require('child_process').execSync(\"pwsh -nop -command $env:fOo; [environment]::getenvironmentvariable('fOo'); '---'; [environment]::getenvironmentvariables(); \", { env: env }).toString()
'@

The above prints (note how the first definition remained in effect):

first
first
---

Name                           Value
----                           -----
foo                            last
USERPROFILE                    C:\Users\jdoe
PROMPT                         $P$G
PSModulePath                   C:\Users\jdoe\Documents\PowerShell\Modules;C:\Program Files\PowerShell\Modules;c:\program files\powershell\6.0.1\Modules
LOGONSERVER                    \\MK-W10
COMSPEC                        C:\WINDOWS\system32\cmd.exe
FOO                            first
SYSTEMROOT                     C:\WINDOWS
WINDIR                         C:\WINDOWS
SYSTEMDRIVE                    C:
HOMEPATH                       \Users\jdoe
PATHEXT                        .COM;.EXE;.BAT;.CMD;.VBS;.JS;.WS;.MSC
PATH                           C:\ProgramData\Oracle\Java\javapath;C:\Program Files\CMake\bin;C:\tools\ruby23\bin;C:\Python35\Scripts\;C:\Python35\;C:\WINDOWS\system32;C:\WINDO...
USERDOMAIN                     JDOE-W10
TEMP                           C:\Users\jdoe\AppData\Local\Temp
USERNAME                       jdoe
HOMEDRIVE                      C:

Michael Klement (@mklement0)

Michael Klement (@mklement0) The fix you were suggesting was easy enough to understand. The question is what is the benefit. Let us review the differences between the two changes:

	My fix	your fix
case sensitivity	Unaltered	Unaltered
what show up	Both items	One Item (potentially hiding a value)
complexity versus original	Reduced	Increased

So, is only seeing one item (hiding one value) in the environment provider really worth the increased complexity.

Your example also shows that one value could be hidden with your fix.

Travis Plunk (@TravisEz13)

Travis Plunk (@TravisEz13):

I hope we can agree that complexity of the implementation should not be the driving factor here, given that even my relatively more complex fix is, in the end, quite simple.

If we focus on the user experience:

My approach hides a value that for all intents and purposes doesn't exist, so hiding it is a good thing and indeed the main purpose of my fix.
Conversely, not hiding it is apt to create confusion - which value is in effect?
If someone truly wanted to get at the hidden value - and I don't see why - they can still use [Environment]::GetEnvironmentVariables().

Travis Plunk (@TravisEz13)

Travis Plunk (@TravisEz13):

Let's consider a concrete example of the side effects of not hiding the case variants:

Assume that a "rogue" application has created case variants FOO_bar, foo_bar and fOo_bar, with distinct values 1, 2, 3 (to recap: only one of which is actually visible if you acess $env:foo_bar in any case variation).
Additionally, assume the existence of (single) var. foo_other with value 4.
Now I want to get the values of these variables using a wildcard expression with Get-Item:

# With your fix, you'd get *4* matches
> Get-Item env:foo_* 
Name                           Value                                                                                                                                               
----                           -----                                                                                                                                               
FOO_bar                            1      
foo_bar                            2                                                                                                                                                  
fOo_bar                            3                
foo_other                          4

Now, which one is the foo_bar value I'm looking for, which one is actually in effect for a lookup by name?
Even if you know what the underlying problem is, you would (a) have to anticipate it every time and (b) work around it in a cumbersome fashion.

Michael Klement (@mklement0)

Michael Klement (@mklement0)
I found something that convinced me to make the change. Get-Item does become case sensitive. Thanks for your feedback.

PS > node -pe @'
>>     env = {}
>>     env.FOO = 'first'
>>     require('child_process').execSync(\"pwsh -nop -command get-item env:FOO; get-item env:foo; $env:foo \", { env: en
v }).toString()
>> '@

Name                           Value
----                           -----
FOO                            first
first

Michael Klement (@mklement0)

Michael Klement (@mklement0) I updated the PR. I didn't use try-catch, but the fix is equivalent. Assuming, we don't care which value ends up in the provider.

Travis Plunk (@TravisEz13)

Travis Plunk (@TravisEz13): While we needn't worry what case variant of the name ends up being reported by the provider, I think it is crucial that we make sure that the value is the effective one, i.e., the one that $env:foo returns - hence my use of Environment.GetEnvironmentVariable((string)entry.Key) to determine the value.

Travis Plunk (@TravisEz13)

Travis Plunk (@TravisEz13): To illustrate the previous point:

My first node snippet demonstrated that it is - regrettably - possible to create case-variant-only environment variables with distinct values; e.g., FOO with value first and foo with value last.

However, on retrieving the value, only one of them "wins", and we shouldn't speculate about which one it is, as that is an implementation detail (though it seems that whatever one is assigned first to the env JS object in my example is the winner).

Specifically, we want to avoid a scenario where Get-Item env:foo returns last, whereas $env:foo returns first.

By simply consulting Environment.GetEnvironmentVariable((string)entry.Key) whenever a case variant is encountered during the enumeration, we ensure that Get-Item too reflects the "winning" value.

The only goal here is to prevent the provider from crashing. This is beyond the scope of this issue. Please file a new issue to handle this feature request. I don't think your fix is as simple as it sounds.

The only goal here is to prevent the provider from crashing.

That being the only goal is regrettable, given that we already know that the fix you just merged introduces an insidious - if rare - inconsistency and that we've discussed a proper fix. (The rarity may actually heighten the insidiousness.)

Please file a new issue to handle this feature request.

Please see #6460

I don't think your fix is as simple as it sounds.

Please tell us why - over at #6460, which duplicates much of what we've discussed here.

All in all, not concluding the discussion here has ended up creating more work for everyone.

fix error in windows provider when the environment as an existing set…

Fix error in windows provider when the environment has accidental dup…

Keep uptodate (#1)

dump env vars; validate fix for dup env vars on win

Fix error in windows provider when the environment has accidental dup…

Navigation Menu

Repository navigation

get-childitem env: fails on Windows when an environment variable is defined twice, cased differently #6305

Steps to reproduce

Expected behavior

Actual behavior

Environment data

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

get-childitem env: fails on Windows when an environment variable is defined twice, cased differently #6305

Description

Steps to reproduce

Expected behavior

Actual behavior

Environment data

Activity

TravisEz13 commented on Mar 6, 2018

TravisEz13 commented on Mar 6, 2018

TravisEz13 commented on Mar 6, 2018

mklement0 commented on Mar 8, 2018

TravisEz13 commented on Mar 8, 2018

mklement0 commented on Mar 9, 2018

TravisEz13 commented on Mar 9, 2018

mklement0 commented on Mar 9, 2018

mklement0 commented on Mar 9, 2018

TravisEz13 commented on Mar 9, 2018

TravisEz13 commented on Mar 20, 2018

mklement0 commented on Mar 21, 2018

mklement0 commented on Mar 21, 2018

TravisEz13 commented on Mar 21, 2018

mklement0 commented on Mar 22, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions