From 748e0b930ec7fe5be9b8e54dc2be1cf97e0a209c Mon Sep 17 00:00:00 2001 From: Michael Klement Date: Sat, 24 Mar 2018 14:41:27 -0400 Subject: [PATCH 1/4] fix error in windows provider when the environment as an existing set of variables name that only differs by case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - make the provider storage for the environment on windows ignore duplicates
 - add tests to verify existing environment get-item behavior fixes #6305 and supersedes #6320 based on discussion in #6460 --- .../namespaces/EnvironmentProvider.cs | 36 ++++++++++++++----- .../Get-Item.Tests.ps1 | 19 ++++++++++ 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/src/System.Management.Automation/namespaces/EnvironmentProvider.cs b/src/System.Management.Automation/namespaces/EnvironmentProvider.cs index fc5952b6459..e2691e62f43 100644 --- a/src/System.Management.Automation/namespaces/EnvironmentProvider.cs +++ b/src/System.Management.Automation/namespaces/EnvironmentProvider.cs @@ -198,15 +198,33 @@ internal override IDictionary GetSessionStateTable() IDictionary environmentTable = Environment.GetEnvironmentVariables(); foreach (DictionaryEntry entry in environmentTable) { - // 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); + 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 as the only entry, because for a given key 'foo' (and all its case variations), + // that is guaranteed to match what $env:FOO and [environment]::GetEnvironmentVariable('foo') return. + // (If, by contrast, we just used `entry` as-is every time a duplicate is encountered, + // it could - intermittently - represent a value *other* than the effective one.) + string effectiveValue = Environment.GetEnvironmentVariable((string)entry.Key); + if (((string)entry.Value).Equals(effectiveValue, StringComparison.Ordinal)) { // We've found the effective definition. + // Note: We *recreate* the entry so that the specific name casing of the + // effective definition is also reflected. However, if the case variants + // define the same value, it is unspecified which name variant is reflected + // in Get-Item env: output; given the always case-insensitive nature of the retrieval, + // that shouldn't matter. + providerTable.Remove((string)entry.Key); + providerTable.Add((string)entry.Key, entry); + } + } } return providerTable; diff --git a/test/powershell/Modules/Microsoft.PowerShell.Management/Get-Item.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Management/Get-Item.Tests.ps1 index 65f8256c263..970a7b7853d 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Management/Get-Item.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Management/Get-Item.Tests.ps1 @@ -141,5 +141,24 @@ Describe "Get-Item" -Tags "CI" { (get-item env:\testvar).Value | Should -BeExactly $expectedValue } + + It "get-item reports the effective value among accidental case-variant duplicates on Windows" -skip:(-not $isWindows) { + if (-not (Get-Command -ErrorAction Ignore node.exe)) { + Write-Warning "Test skipped, because Node.js is required to run it." + } else { + $valDirect, $valGetItem, $unused = node.exe -pe @" + env = {} + env.testVar = process.env.testVar // include the original case variant with its original value. + env.TESTVAR = 'c' // redefine with a case variant name and different value + // Note: Which value will win is not deterministic(!); what matters, however, is that both + // $env:testvar and Get-Item env:testvar report the same value. + // The nondeterministic behavior makes it hard to prove that the values are *always* the + // same, however. + require('child_process').execSync(\"\\\"$($PSHOME -replace '\\', '/')/pwsh.exe\\\" -noprofile -command `$env:testvar, (Get-Item env:testvar).Value\", { env: env }).toString() +"@ + $valGetItem | Should -BeExactly $valDirect + + } + } } } From a808e8395a5e1609f46dcf3832cd099faacdbfbc Mon Sep 17 00:00:00 2001 From: Michael Klement Date: Sat, 24 Mar 2018 15:23:19 -0400 Subject: [PATCH 2/4] switched from .Add() with exception handling to .TryAdd() with `if` --- .../namespaces/EnvironmentProvider.cs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/System.Management.Automation/namespaces/EnvironmentProvider.cs b/src/System.Management.Automation/namespaces/EnvironmentProvider.cs index e2691e62f43..8abb7fe9247 100644 --- a/src/System.Management.Automation/namespaces/EnvironmentProvider.cs +++ b/src/System.Management.Automation/namespaces/EnvironmentProvider.cs @@ -198,11 +198,7 @@ internal override IDictionary GetSessionStateTable() IDictionary environmentTable = Environment.GetEnvironmentVariables(); foreach (DictionaryEntry entry in environmentTable) { - try - { - providerTable.Add((string)entry.Key, entry); - } - catch (System.ArgumentException) + if (!providerTable.TryAdd((string)entry.Key, entry)) { // 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 - From 51f468f9a02c666ede51a2ed4d95d05ba52b2221 Mon Sep 17 00:00:00 2001 From: Michael Klement Date: Mon, 26 Mar 2018 16:12:29 -0400 Subject: [PATCH 3/4] Make the test a separate, scenario test. --- .../Get-Item.Tests.ps1 | 41 +++++++++++-------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/test/powershell/Modules/Microsoft.PowerShell.Management/Get-Item.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Management/Get-Item.Tests.ps1 index 970a7b7853d..8b4b8b283d0 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Management/Get-Item.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Management/Get-Item.Tests.ps1 @@ -142,23 +142,32 @@ Describe "Get-Item" -Tags "CI" { (get-item env:\testvar).Value | Should -BeExactly $expectedValue } - It "get-item reports the effective value among accidental case-variant duplicates on Windows" -skip:(-not $isWindows) { - if (-not (Get-Command -ErrorAction Ignore node.exe)) { - Write-Warning "Test skipped, because Node.js is required to run it." - } else { - $valDirect, $valGetItem, $unused = node.exe -pe @" - env = {} - env.testVar = process.env.testVar // include the original case variant with its original value. - env.TESTVAR = 'c' // redefine with a case variant name and different value - // Note: Which value will win is not deterministic(!); what matters, however, is that both - // $env:testvar and Get-Item env:testvar report the same value. - // The nondeterministic behavior makes it hard to prove that the values are *always* the - // same, however. - require('child_process').execSync(\"\\\"$($PSHOME -replace '\\', '/')/pwsh.exe\\\" -noprofile -command `$env:testvar, (Get-Item env:testvar).Value\", { env: env }).toString() -"@ - $valGetItem | Should -BeExactly $valDirect + } - } +} + +Describe "Get-Item environment provider on Windows with accidental case-variant duplicates" -Tags "Scenario" { + BeforeAll { + $env:testVar = 'a' # Note: Even though PSScriptAnalyzer can't detect it, this variable *is* used below, namely via Node.js. + } + AfterAll { + $env:testVar = $null + } + It "Reports the effective value among accidental case-variant duplicates on Windows" -skip:$skipNotWindows { + if (-not (Get-Command -ErrorAction Ignore node.exe)) { + Write-Warning "Test skipped, because prerequisite Node.js is not installed." + } else { + $valDirect, $valGetItem, $unused = node.exe -pe @" + env = {} + env.testVar = process.env.testVar // include the original case variant with its original value. + env.TESTVAR = 'b' // redefine with a case variant name and different value + // Note: Which value will win is not deterministic(!); what matters, however, is that both + // $env:testvar and Get-Item env:testvar report the same value. + // The nondeterministic behavior makes it hard to prove that the values are *always* the + // same, however. + require('child_process').execSync(\"\\\"$($PSHOME -replace '\\', '/')/pwsh.exe\\\" -noprofile -command `$env:testvar, (Get-Item env:testvar).Value\", { env: env }).toString() +"@ + $valGetItem | Should -BeExactly $valDirect } } } From 317138414bb80152c7c70f2c15ca1a0c809d267e Mon Sep 17 00:00:00 2001 From: Michael Klement Date: Tue, 27 Mar 2018 09:04:26 -0400 Subject: [PATCH 4/4] Remove extraneous empty lines. --- .../Modules/Microsoft.PowerShell.Management/Get-Item.Tests.ps1 | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/powershell/Modules/Microsoft.PowerShell.Management/Get-Item.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Management/Get-Item.Tests.ps1 index 8b4b8b283d0..c66ba603afb 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Management/Get-Item.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Management/Get-Item.Tests.ps1 @@ -141,9 +141,7 @@ Describe "Get-Item" -Tags "CI" { (get-item env:\testvar).Value | Should -BeExactly $expectedValue } - } - } Describe "Get-Item environment provider on Windows with accidental case-variant duplicates" -Tags "Scenario" {