-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Get-Item directory -stream * now works #10570
#13650
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
Conversation
|
I would like to mention that none of the 24 issues of complexity that CodeFactor is complaining about are in the code that I modified. I hope that preexisting complexity will not prevent the acceptance of this patch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests in Get-Item.Tests.ps1 file so that cover all scenarios (positive, negative, file, directory and so on).
Please do not add style and formatting changes until maintainers ask - our policy to pull functional and style changes in different PRs.
src/System.Management.Automation/namespaces/FileSystemProvider.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/namespaces/FileSystemProvider.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/namespaces/FileSystemProvider.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/namespaces/FileSystemProvider.cs
Outdated
Show resolved
Hide resolved
|
@kyanha Please keep a commit history - don't rebase until maintainers ask you. Please address feedbacks adding new commits. At merge time maintainers will squash commits. |
Get-Item directory -stream * now works #10570Get-Item directory -stream * now works #10570
test/powershell/Modules/Microsoft.PowerShell.Management/Get-Item.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Management/Get-Item.Tests.ps1
Outdated
Show resolved
Hide resolved
|
Added a test for non-erroring globbing with wildcards. |
iSazonov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kyanha
I believe we should follow the rules for both files and directories:
Get-Item -Path * -Stream *orGet-Item -Path "exact_name" -Stream *shouldn't write errorsGet-Item -Path * -Stream "exact_name_without_wildcards"should write non-terminating errors if the stream is not found.
Currently second rule does not work.
A for tests, we could combine tests with -TestCases for files and directories because they have the same code.
src/System.Management.Automation/namespaces/FileSystemProvider.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/namespaces/FileSystemProvider.cs
Outdated
Show resolved
Hide resolved
This is the behavior that it follows right now. That test was part of my prior push in the tests.
I'm sorry, but I do not understand why you say that? I just ran it against my PowerShell code directory. This continues the run after not finding the stream on the src/ or test/ or docs/ directories. Is that not the definition of a non-terminating error? PS D:\Users\kyanha\work\github\Powershell\Powershell> get-item -Path * -Stream FTW
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\.devcontainer'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\.github'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\.poshchan'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\.vs'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\.vscode'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\.vsts-ci'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\assets'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\CHANGELOG'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\demos'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\docker'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\docs'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\Scripts'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\src'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\test'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\tools'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\.editorconfig'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\.gitattributes'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\.gitignore'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\.markdownlint.json'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\.spelling'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\ADOPTERS.md'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\build.psm1'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\CHANGELOG.md'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\CODE_OF_CONDUCT.md'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\codecov.yml'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\DotnetRuntimeMetadata.json'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\global.json'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\LICENSE.txt'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\nuget.config'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\pester-tests.xml'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\PowerShell.Common.props'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\PowerShell.sln'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\README.md'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\Settings.StyleCop'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\stylecop.json'.
Get-Item: Could not open the alternate data stream 'FTW' of the file 'D:\Users\kyanha\work\github\Powershell\Powershell\ThirdPartyNotices.txt'.
PS D:\Users\kyanha\work\github\Powershell\Powershell>The test I wrote for directories (Get-Item.Tests.ps1:140) precisely mirrors the test that had already existed for files (Get-Item.Tests.ps1:131). If there's a deficiency in the test I copied from, then another issue needs to be opened to revisit all of the tests in that file to verify terminating versus non-terminating errors. I'm not certain, but I think that's out of scope of this issue and PR.
I need a lot more information on the testing system, and it's not available in Get-Help It/Get-Help Should. Is there documentation anywhere that you can point me to? (Is there external documentation that should be pointed to in the PowerShell contribution documentation?) The issues about the test framework that I am ignorant of and need to educate myself on are:
|
Sorry, I skipped one test.
I can find many examples in our tests. Search "-TestCases".
You use right pattern in new tests ( |
test/powershell/Modules/Microsoft.PowerShell.Management/Get-Item.Tests.ps1
Outdated
Show resolved
Hide resolved
…em.Tests.ps1 Co-authored-by: Ilya <darpa@yandex.ru>
|
How can the CI system be forced to rebuild? I don't want to push a new commit that does nothing. (The MacOS tests failed apparently because of a WebListener, and none of the code in this patch touches a WebListener so it's likely a temporary environmental thing.) |
…em.Tests.ps1 Additional positive type checking on the result. Co-authored-by: Ilya <darpa@yandex.ru>
…Content.Tests.ps1 Be precise in our key comparisons. Co-authored-by: Ilya <darpa@yandex.ru>
Get-Item directory -stream * now works #10570Get-Item directory -stream * now works #10570
|
@kyanha if (handle.IsInvalid) throw new Win32Exception();Do you say only about new NotSupportedException or about Win32Exception too? |
When GetLastWin32Error() returns ERROR_HANDLE_EOF, it returns the existing list (because it literally means that there's nothing more to be added to the list). For directories, this means that the list can be empty without an error condition. This case is handled correctly. When GetLastWin33Error() returns ERROR_INVALID_PARAMETER, AlternateDataStreamUtilities throws a NotSupportedException. This is uncaught above, which means it gets caught by the general exception handler which turns it into a terminating error. (This is the situation which caused me to block committing this PR.) If GetLastWin32Error() returns anything else (which is possible, since filesystems can return anything they want without the system filtering them), there's nothing we can do in AlternateDataStreamUtilities -- we have to throw Win32Exception because the driver isn't behaving according to the protocol we understand, so we have no means of interpreting it into a user-friendly error message. This is also uncaught above, and also turns into a terminating error. I had been thinking of leaving the Win32Exception as terminating, but had not yet made up my mind -- but I think the impact analyses between this and the NotSupportedException case are the same, and suggest that both should be made nonterminating. I mean, the same issue of -Path being an array applies, and it would definitely be a per-filesystem behavior that might not apply to later entries in the array. The same logic which could apply to NotSupportedException could easily also apply to Win32Exception. So, I'll do that, unless you can suggest a reason not to? (Also, regardless of whether it's an edge-case, I really think we should try to find a way to code some tests on a filesystem that doesn't support alternate data streams. ReFS doesn't support them, and many administrators are going to run scripts on systems with both NTFS and ReFS. Because stream-unsupported errors on one filesystem must not terminate processing of a -Path array, I think it's important enough to put into CI regression tests. I only caught it when I was doing manual tests locally, and it would be really easy to miss otherwise. I just have no idea who to ask how to do so; can you suggest somebody?) |
|
We have not complains about "not support" scenario. So no need to do anything in the PR. |
This patch currently processes ERROR_HANDLE_EOF correctly. I've filed issue #13766 about the "terminating error should be non-terminating error" problem. Rereleasing for review and potential merge. |
Get-Item directory -stream * now works #10570Get-Item directory -stream * now works #10570
src/System.Management.Automation/namespaces/FileSystemProvider.cs
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Management/Clear-Content.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Management/Clear-Content.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Management/Remove-Item.Tests.ps1
Outdated
Show resolved
Hide resolved
| Get-Content -Path ${altStreamPath2} -Stream $streamName | Should -BeExactly $stringData | ||
| } | ||
| It "Should create a new data stream on a directory" -Skip:(-Not $IsWindows) { | ||
| { Set-Content -Path $altStreamDirectory -Stream $streamName -Value $stringData } | Should -Not -Throw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the request.
|
@kyanha The PR contains too many commits and a lot of comments. I suggest to close the PR and open new PR with one initial commit. This will simplify follow reviews from MSFT team. Thanks! |
|
Superseded by #13795, closing. |
|
I've spent a while thinking about this, @iSazonov, and I feel I have to call out your poor behavior. From #13650 (comment):
Huh. Would you look at that. This directly contradicts your earlier statement that you posted after I force-pushed squashed updates to the branch in accordance with the PR guidance in the project introduction: From #13650 (comment):
You said that they'd squash them. Now I'm stuck with a repository and history that's polluted by two separate branches, adding to the administrative overhead. Why did you change your guidance? Why did you demand the commit history? When did you realize that your demand was going to lead to another PR? What made you feel this was acceptable? |
|
@kyanha
As result I asked you open new PR. |
PR Summary
Supports
Get-Item -streamfor NTFS alternate data streams on directories, not merely files. Fixes #10570. Fixes #13656.PR Context
#10570 has been open for almost a year. NTFS supports what are called "Alternate Data Streams" on both files and directories (multiple named discrete blobs of data which are associated with a single directory entry). PowerShell supports enumeration of these Alternate Data Streams, using the '-stream' parameter to 'Get-Item'.
Unfortunately, the initial implementation of PowerShell only supported alternate data streams on files, not on directories. This makes an entire facility of the OS's file system invisible, and if an administration team is relying on PowerShell it makes an attractive place for a red team to store data to exfiltrate. (This is not an invitation to destroy the capability to store alternate data streams on directories, as they are useful for many purposes. It is merely a rationale for making their existence visible through PowerShell.)
To create and see an alternate data stream on a directory, use cmd.exe to run the following commands:
The output is something like:
To see the failure of PowerShell being able to see the stream on the file, but not the directory:
With this change,This has been fixed in the AlternateDataStreamUtilities.GetStream() method, by making ERROR_HANDLE_EOF returned from an enumeration of streams on a directory no longer throw an exception.Get-Item directory -stream *will throw "Get-Item: End of file reached." when there are no alternate data streams on a directory.Things not done:
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.