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.

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

Bug: can not handle "/" correctly when reading registry item #5536

Closed
ZSkycat opened this issue Nov 24, 2017 · 22 comments
Closed

Bug: can not handle "/" correctly when reading registry item #5536

ZSkycat opened this issue Nov 24, 2017 · 22 comments
Labels
Committee-Reviewed PS-Committee has reviewed this and made a decision Issue-Bug Issue has been identified as a bug in the product Resolution-No Activity Issue has had no activity for 6 months or more Up-for-Grabs Up-for-grabs issues are not high priorities, and may be opportunities for external contributors WG-Cmdlets-Management cmdlets in the Microsoft.PowerShell.Management module WG-Engine-Providers built-in PowerShell providers such as FileSystem, Certificates, Registry, etc.

Comments

@ZSkycat
Copy link

ZSkycat commented Nov 24, 2017

Steps to reproduce

# ** careful "\" and "/" **
# initialization
New-Item 'registry::HKEY_CURRENT_USER/test'
New-ItemProperty 'registry::HKEY_CURRENT_USER\test' -Name test1 -Value test1

# read item.
Get-Item 'registry::HKEY_CURRENT_USER/test'
Set-ItemProperty 'registry::HKEY_CURRENT_USER/test' -Name test2 -Value abcde

# read and create item
Copy-Item 'registry::HKEY_CURRENT_USER/test' 'registry::HKEY_CURRENT_USER/test/t1'
Copy-Item 'registry::HKEY_CURRENT_USER\test' 'registry::HKEY_CURRENT_USER/test/t1'

# clear
Remove-Item 'registry::HKEY_CURRENT_USER\test' -Recurse -Force

Expected behavior

no errors

Actual behavior

Get-Item : Cannot find path 'HKEY_CURRENT_USER/test' because it does not exist.
At line:1 char:1
+ Get-Item 'registry::HKEY_CURRENT_USER/test'
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : ObjectNotFound: (HKEY_CURRENT_USER/test:String) [Get-Item], ItemNotFoundException
+ FullyQualifiedErrorId : PathNotFound,Microsoft.PowerShell.Commands.GetItemCommand

Set-ItemProperty : Cannot find path 'HKEY_CURRENT_USER/test' because it does not exist.
At line:1 char:1
+ Set-ItemProperty 'registry::HKEY_CURRENT_USER/test' -Name test2 -Valu ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : ObjectNotFound: (HKEY_CURRENT_USER/test:String) [Set-ItemProperty], ItemNotFoundException
+ FullyQualifiedErrorId : PathNotFound,Microsoft.PowerShell.Commands.SetItemPropertyCommand

Copy-Item : Cannot find path 'HKEY_CURRENT_USER/test' because it does not exist.
At line:1 char:1
+ Copy-Item 'registry::HKEY_CURRENT_USER/test' 'registry::HKEY_CURRENT_ ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : ObjectNotFound: (HKEY_CURRENT_USER/test:String) [Copy-Item], ItemNotFoundException
+ FullyQualifiedErrorId : PathNotFound,Microsoft.PowerShell.Commands.CopyItemCommand

Environment data

> $PSVersionTable
Name                           Value
----                           -----
PSVersion                      6.0.0-rc
PSEdition                      Core
GitCommitId                    v6.0.0-rc
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

Summary

after trying, I found the law.

  1. can handle "/" correctly when creating item.
  2. can not handle "/" correctly when reading item.

related cmdlets

New-Item
New-ItemPropert
Get-ChildItem
Get-Item
Get-ItemProperty
Get-ItemPropertyValue
Set-Item
Set-ItemProperty
Clear-Item
Clear-ItemProperty
Remove-Item
Remove-ItemProperty
Rename-Itemre
Rename-ItemProperty
Copy-Item
Copy-ItemProperty
Move-Item
Move-ItemProperty
@ZSkycat
Copy link
Author

ZSkycat commented Nov 24, 2017

This bug also exists in Windows PowerShell (no core)

@iSazonov
Copy link
Collaborator

'/' is allowed in registry key names. Can you to try escape '/'?

@iSazonov iSazonov added WG-Engine-Providers built-in PowerShell providers such as FileSystem, Certificates, Registry, etc. Issue-Discussion the issue may not have a clear classification yet. The issue may generate an RFC or may be reclassif labels Nov 24, 2017
@ZSkycat
Copy link
Author

ZSkycat commented Nov 24, 2017

@iSazonov
I am using \ to avoid this bug.

but I like automatic processing slash in powershell. (like file path)

@mklement0
Copy link
Contributor

To be clear: The real bug is that / is supported at all as a path separator with the registry provider (even though the desire to use / and \ interchangeably, as with the filesystem provider, is understandable).

As @iSazonov points out, / is a valid character in a registry key (or value) name; therefore, it cannot function as the path separator, which in the registry provider's case should only be \.

(Unfortunately, the objects returned by Get-PSProvider have no property that indicates a provider's path separator(s).)

Using one of your examples:

That New-Item 'registry::HKEY_CURRENT_USER/test' succeeds and functions the same as New-Item 'registry::HKEY_CURRENT_USER\test' is a manifestation of the bug.

@ZSkycat
Copy link
Author

ZSkycat commented Nov 25, 2017

@mklement0 I understand.

item name allow use '/' is too bad

😂😂😂

@iSazonov
Copy link
Collaborator

(Unfortunately, the objects returned by Get-PSProvider have no property that indicates a provider's path separator(s).)

That's why I'm more inclined to think that we need to optimize providers and use types instead of strings.

@mklement0
Copy link
Contributor

@iSazonov: What do you mean by using types instead of strings?

@iSazonov
Copy link
Collaborator

Providers work with strings and do parsing its on every operation. This results in poor performance and problems like this.

@SteveL-MSFT SteveL-MSFT added this to the 6.1.0-Consider milestone Feb 12, 2018
@ZSkycat ZSkycat closed this as completed Feb 28, 2018
@ZSkycat ZSkycat reopened this Feb 28, 2018
@rjmholt rjmholt self-assigned this Mar 7, 2018
@rjmholt
Copy link
Collaborator

rjmholt commented Mar 7, 2018

Tracing this, it looks like there has already been some consideration of this in the NavigationProvider base class here.

The comment seems to indicate that unless both back and forwards slashes are used, we want to treat forward slashes as back slashes. That seems a bit strange to me, but looking for input.

@rjmholt
Copy link
Collaborator

rjmholt commented Mar 7, 2018

Running New-Item 'registry::HKEY_CURRENT_USER\test\/', it turns out that the path that gets to the NavigationProvider is still 'registry::HKEY_CURRENT_USER\\test', since the SessionStateContainer already normalizes the path.

@rjmholt rjmholt removed their assignment Mar 7, 2018
@iSazonov
Copy link
Collaborator

iSazonov commented May 8, 2018

Related #6833 - seems it is really problem.

@rjmholt
Copy link
Collaborator

rjmholt commented May 8, 2018

It seems like the fix for this would be to make providers responsible for path handling and manage their own directory char separators.

So that way a registry provider can recognise / as an ordinary name char and a filesystem provider can recognise it as a path separator. It might also provide some ability to differentiate UNIX filesystems and Windows ones, although that's less clear.

While there's some consideration about ease of use for users, I think that fidelity to the native environment takes priority there.

My concern is that this could constitute a spiritual breaking change in PowerShell, whereby it's no longer "PowerShell handles cross-platform paths" but "providers are responsible for their paths and the filesystem provider treats / and \ equally". There might not be any significant change in user experience, so maybe it's not worth worrying about. But since SessionStateContainer does path normalisation before hitting any providers, I thought it might be worth discussing.

@BrucePay
Copy link
Collaborator

BrucePay commented May 8, 2018

@rjmholt From MSDN on the structure of the registry

Each key has a name consisting of one or more printable characters. Key names are not case sensitive. Key names cannot include the backslash character (), but any other printable character can be used. Value names and data can include the backslash character

So the natural path separator is \ (and it's what reg.exe uses exclusively) like everywhere else in Windows. However, for the convenience of the people coming to PowerShell from a *nix background (and the idea that we'd port to *nix someday), we chose to also allow / in paths and then normalize as appropriate.

@rjmholt
Copy link
Collaborator

rjmholt commented May 8, 2018

Right, with the tricky bit being "as appropriate".

The definite bug above is that reading a registry key behaves inconsistently with creating a registry key with respect to path normalisation.

But the question remains: do we always do the normalisation and possibly hide some native corner cases (and ideally provide a well-documented way to escape paths), or do we only normalise paths when it doesn't collide with the native implementation?

UNIX filenames allow (ASCII) \ and the Windows registry allows / in keys, so a single path-normalisation policy won't fit everywhere. (A brief look at wsman: and cert: suggests they don't have this problem, but worth checking).

@rjmholt rjmholt added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label May 11, 2018
@rjmholt
Copy link
Collaborator

rjmholt commented May 11, 2018

Tagging this for committee review so that it can be discussed alongside #6833.

@SteveL-MSFT
Copy link
Member

@PowerShell/powershell-committee reviewed this and similar to #6833, path normalization should happen within the provider. If the registry provider is inappropriately normalizing the path, that is a bug that should be fixed.

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels May 23, 2018
@SteveL-MSFT SteveL-MSFT modified the milestones: 6.1.0-Consider, Future Jun 20, 2018
@iSazonov
Copy link
Collaborator

In Registry provider we need

  • to define an override for protected virtual string MakePath(string parent, string child)
  • make and call there customized protected string MakePath(string parent, string child, bool childIsLeaf) (from NavigationProviderBase.cs)
  • make and call there customized private string NormalizePath(string path) (from NavigationProviderBase.cs)

I am not sure that it is all that we should make to fix the bug.

@iSazonov iSazonov added Issue-Bug Issue has been identified as a bug in the product Up-for-Grabs Up-for-grabs issues are not high priorities, and may be opportunities for external contributors and removed Issue-Discussion the issue may not have a clear classification yet. The issue may generate an RFC or may be reclassif labels Oct 14, 2019
@iSazonov
Copy link
Collaborator

@SteveL-MSFT Taking in account #10215 (PowerShell crash) please weigh the issue.

@SteveL-MSFT
Copy link
Member

@iSazonov since the recursion issue repros with 5.1, this is not a regression. It's also an unlikely user scenario and I haven't seen many reports of this. I would like to have this fixed, but I don't think it changes the priority.

@vanillaSprinkles
Copy link

@PowerShell/powershell-committee reviewed this and similar to #6833, path normalization should happen within the provider. If the registry provider is inappropriately normalizing the path, that is a bug that should be fixed.

It absolutely is inappropriately normalizing the path and causes a few other problems, I realize now I should've posted to this thread a fix, but I believe result = path.Replace(StringLiterals.AlternatePathSeparator, StringLiterals.DefaultPathSeparator); is the culprit and would likely remove quite a few issues (issue above references the correct line number).

The registry allows slashes / in key names, plain and simple. Take a look at the EdgeExtensions within and look at those beautiful key-names:
HKEY_CURRENT_USER\Software\Classes\Local Settings\Software\Microsoft\Windows\CurrentVersion\AppContainer\Storage\microsoft.microsoftedge_8wekyb3d8bbwe\EdgeExtensions\Configuration\EdgeExtensions\ConfigurationStore\Extensions

@Deadooshka
Copy link

got almost a same bug. Create some registry key 'Some_key' and create a subkey named '/' (slash only). Run Get-ChildItem -LiteralPath 'Registry::Some_key' -Recurse. You will see eternal repeating.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Resolution-No Activity Issue has had no activity for 6 months or more label Jan 10, 2024
Copy link
Contributor

This issue has not had any activity in 6 months, if there is no further activity in 7 days, the issue will be closed automatically.

Activity in this case refers only to comments on the issue. If the issue is closed and you are the author, you can re-open the issue using the button below. Please add more information to be considered during retriage. If you are not the author but the issue is impacting you after it has been closed, please submit a new issue with updated details and a link to this issue and the original.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committee-Reviewed PS-Committee has reviewed this and made a decision Issue-Bug Issue has been identified as a bug in the product Resolution-No Activity Issue has had no activity for 6 months or more Up-for-Grabs Up-for-grabs issues are not high priorities, and may be opportunities for external contributors WG-Cmdlets-Management cmdlets in the Microsoft.PowerShell.Management module WG-Engine-Providers built-in PowerShell providers such as FileSystem, Certificates, Registry, etc.
Projects
None yet
Development

No branches or pull requests

8 participants