Skip to content

Conversation

@kyanha
Copy link
Contributor

@kyanha kyanha commented Sep 17, 2020

PR Summary

Supports Get-Item -stream for 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:

> mkdir 10570demo
> cd 1057demo
> echo "This is a file." > 10570demo.txt
> echo "This is an alternate data stream on the file." > 10570demo.txt:datastream
> mkdir bug10570
> echo "This is an alternate data stream on the directory." > bug10570:datastream
> dir /r

The output is something like:

D:\10570demo>dir /r
 Volume in drive D is DATA
 Volume Serial Number is 8FD3-BD69

 Directory of D:\10570demo

09/17/2020  02:59 PM    <DIR>          .
09/17/2020  02:59 PM    <DIR>          ..
09/17/2020  02:58 PM                20 10570demo.txt
                                    50 10570demo.txt:datastream:$DATA
09/17/2020  02:59 PM    <DIR>          bug10570
                                    55 bug10570:datastream:$DATA
               1 File(s)             20 bytes
               3 Dir(s)  88,185,401,344 bytes free

To see the failure of PowerShell being able to see the stream on the file, but not the directory:

> pwsh
PS > Get-Item *


    Directory: D:\10570demo

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
da---           9/17/2020  2:59 PM                bug10570
-a---           9/17/2020  2:58 PM             20 10570demo.txt

PS > Get-Item * -stream *

PSPath        : Microsoft.PowerShell.Core\FileSystem::D:\10570demo\10570demo.txt::$DATA
PSParentPath  : Microsoft.PowerShell.Core\FileSystem::D:\10570demo
PSChildName   : 10570demo.txt::$DATA
PSDrive       : D
PSProvider    : Microsoft.PowerShell.Core\FileSystem
PSIsContainer : False
FileName      : D:\10570demo\10570demo.txt
Stream        : :$DATA
Length        : 20

PSPath        : Microsoft.PowerShell.Core\FileSystem::D:\10570demo\10570demo.txt:datastream
PSParentPath  : Microsoft.PowerShell.Core\FileSystem::D:\10570demo
PSChildName   : 10570demo.txt:datastream
PSDrive       : D
PSProvider    : Microsoft.PowerShell.Core\FileSystem
PSIsContainer : False
FileName      : D:\10570demo\10570demo.txt
Stream        : datastream
Length        : 50

PS > Get-Item bug10570 -stream *
PS >

With this change, Get-Item directory -stream * will throw "Get-Item: End of file reached." when there are no alternate data streams on a directory. 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.

Things not done:

PR Checklist

@kyanha kyanha requested a review from anmenaga as a code owner September 17, 2020 20:18
@ghost ghost assigned anmenaga Sep 17, 2020
@ghost
Copy link

ghost commented Sep 17, 2020

CLA assistant check
All CLA requirements met.

@kyanha
Copy link
Contributor Author

kyanha commented Sep 18, 2020

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?

Copy link
Collaborator

@iSazonov iSazonov left a 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.

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Sep 18, 2020
@iSazonov
Copy link
Collaborator

@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.

@kyanha kyanha changed the title WIP: Get-Item directory -stream * now works #10570 Get-Item directory -stream * now works #10570 Sep 18, 2020
@kyanha kyanha requested review from iSazonov and vexx32 September 18, 2020 19:14
@kyanha kyanha requested a review from iSazonov September 20, 2020 07:36
@kyanha
Copy link
Contributor Author

kyanha commented Sep 20, 2020

Added a test for non-erroring globbing with wildcards.

Copy link
Collaborator

@iSazonov iSazonov left a 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 * or Get-Item -Path "exact_name" -Stream * shouldn't write errors
  • Get-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.

@kyanha
Copy link
Contributor Author

kyanha commented Sep 20, 2020

I believe we should follow the rules for both files and directories:

* `Get-Item -Path * -Stream *` or `Get-Item -Path "exact_name" -Stream *` shouldn't write errors

This is the behavior that it follows right now. That test was part of my prior push in the tests.

* `Get-Item -Path * -Stream "exact_name_without_wildcards"` should write non-terminating errors if the stream is not found.

Currently second rule does not work.

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.

A for tests, we could combine tests with -TestCases for files and directories because they have the same code.

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:

  1. I'm not sure how to write a test for a non-terminating error. All the other examples I found for errors that are normally non-terminating used -ErrorAction Stop to cause them to throw. (see Get-Item.Tests.ps1:131 for the file case that was already there when I started, and Get-Item.Tests.ps1:140 for the directory case that I copied the form of line 131 for). Is there an example you can find for non-terminating errors that I can write it around to test that it's non-terminating? (It actually is a non-terminating error, exactly the same as for files, but the preexisting test for the file error turns it into a terminating error for the test. I copied that style.)

  2. I don't understand your reference to -TestCases as a separate command or option, and I need documentation to understand.

  • Tests for specific stream names against files versus directories can be consolidated, as there is no difference in behavior.
  • Tests for wildcard stream names against files versus directories cannot be consolidated, if the wildcard is '*', because a file will always have at least one stream of type $DATA (the unnamed one), while directories by default have no data streams (and can never have an unnamed one). As a result, the output will always differ between a file and a directory, and the tests need to be different as a result.

@iSazonov
Copy link
Collaborator

I do not understand why you say that?

Sorry, I skipped one test.

TestCases

I can find many examples in our tests. Search "-TestCases".

I'm not sure how to write a test for a non-terminating error.

You use right pattern in new tests (-ErrorAction Stop)

…em.Tests.ps1

Co-authored-by: Ilya <darpa@yandex.ru>
@kyanha kyanha requested a review from iSazonov September 26, 2020 22:50
@kyanha
Copy link
Contributor Author

kyanha commented Sep 28, 2020

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.)

kyanha and others added 2 commits October 8, 2020 15:18
…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>
@kyanha kyanha changed the title Get-Item directory -stream * now works #10570 WIP: Get-Item directory -stream * now works #10570 Oct 8, 2020
@kyanha kyanha marked this pull request as draft October 8, 2020 20:21
@iSazonov
Copy link
Collaborator

iSazonov commented Oct 9, 2020

@kyanha
Original code is:

if (handle.IsInvalid) throw new Win32Exception();

Do you say only about new NotSupportedException or about Win32Exception too?

@kyanha
Copy link
Contributor Author

kyanha commented Oct 9, 2020

@kyanha
Original code is:

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?)

@iSazonov
Copy link
Collaborator

iSazonov commented Oct 9, 2020

We have not complains about "not support" scenario. So no need to do anything in the PR.
Let's address only directory scenario related to the PR and ignore other scenarios.
I mean to process only ERROR_HANDLE_EOF and throw else (new Win32Exception()).

@kyanha
Copy link
Contributor Author

kyanha commented Oct 10, 2020

We have not complains about "not support" scenario. So no need to do anything in the PR.
Let's address only directory scenario related to the PR and ignore other scenarios.
I mean to process only ERROR_HANDLE_EOF and throw else (new Win32Exception()).

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.

@kyanha kyanha marked this pull request as ready for review October 10, 2020 06:24
@kyanha kyanha changed the title WIP: Get-Item directory -stream * now works #10570 Get-Item directory -stream * now works #10570 Oct 10, 2020
@kyanha kyanha requested a review from iSazonov October 10, 2020 06:24
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
Copy link
Collaborator

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 kyanha requested a review from iSazonov October 11, 2020 05:57
@iSazonov
Copy link
Collaborator

iSazonov commented Oct 12, 2020

@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!

@kyanha
Copy link
Contributor Author

kyanha commented Oct 17, 2020

Superseded by #13795, closing.

@kyanha kyanha closed this Oct 17, 2020
@kyanha
Copy link
Contributor Author

kyanha commented Oct 23, 2020

I've spent a while thinking about this, @iSazonov, and I feel I have to call out your poor behavior.

From #13650 (comment):

@kyanha The PR contains too many commits and a lot of comments. I suggest to close the PR and one new PR with one initial commit. This will simplify follow reviews from MSFT team. Thanks!

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):

@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.

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?

@iSazonov
Copy link
Collaborator

iSazonov commented Oct 24, 2020

@kyanha
After a contributor pulled a PR we ask to do not force-push and keep commit history because it simplify the code review - sometimes it is easy to review commit by commit or review only last change.
Maintainers can ask to rebase to resolve some problems, to get latest codes and tests, CI changes and so on.
In context the PR:

  • we have many conversations - it slow down GitHub and it is unlikely that anybody will read all the comments - it is more easy to look and review code on clean page.
  • the PR contains many commits. MSFT team uses a commercial tool for code review, the tool has limitation 30 commits

As result I asked you open new PR.
I'm sorry that this forces you to spend more time, but it is inevitable.
Thanks for understanding!

@kyanha kyanha deleted the issue10570 branch October 30, 2020 07:23
@kyanha kyanha restored the issue10570 branch December 2, 2020 11:45
@kyanha kyanha deleted the issue10570 branch December 3, 2020 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Get-Content, Set-Content, and Clear-Content should work with datastreams set on directories Get-Item P:\ath\To\A\Directory -Streams * does not work

4 participants