Skip to content

Recognize CONOUT$ and CONIN$ as reserved device names #13508

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

Merged
merged 3 commits into from
Sep 14, 2020

Conversation

davidreis97
Copy link
Contributor

PR Summary

Fixes #13046.

Adds CONOUT$ and CONIN$ to the list of reserved device names. The existing range of filename lengths to be checked for reserved names excluded CONOUT$ since it only checked names of up to 6 characters. To include CONOUT$, this range has been bumped to 7 characters.

With these changes, attempting to execute filesystem commands with these names as targets will now raise an exception, as defined in the expected behaviour section of the linked issue.

PR Checklist

@ghost ghost assigned anmenaga Aug 23, 2020
@iSazonov
Copy link
Collaborator

@DHowett Have you any thoughts about blocking CONIN$ and CONOUT$ in PowerShell file operations?
Copy-Item <text file> CONOUT$ works like Get-Content and this looks good and we could expect copy-item CONIN$ <file path> works too but no. Also we have New-Item and others and this force us to think that we should follow the best practice and block these reserved names.
In my opinion, we should ban these names in order to eliminate misunderstandings and unjustified expectations.

@iSazonov iSazonov requested a review from SteveL-MSFT August 24, 2020 07:09
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Aug 24, 2020
@vexx32
Copy link
Collaborator

vexx32 commented Aug 24, 2020

Yeah, we're already blocking the majority of them, so probably it would make sense to block all of them.

Maybe a subset of them could be useful for specific provider cmdlets, but I don't think any of them would be universally useful for all the provider cmdlets to support.

@DHowett
Copy link

DHowett commented Aug 24, 2020

So, here's the thing that worries me about the windows reserved names.

I don't know how we handle them in a world where pwsh also runs on Linux (and can access /dev/sda1, or /dev/fd/0 which is a lot like CONOUT$) and macOS (/dev/rdisk0, /dev/tty).

Do we have a holistic story that covers how powershell handles devicelike file handles in general? If not, is this just putting a band-aid over a problem?

@DHowett
Copy link

DHowett commented Aug 24, 2020

But, here: We should block these names.

There are better ways to write output to the console or read input from the console. It is generally a bad thing that we would allow PowerShell users -- users who have opted into a higher-level abstraction -- direct unmanaged access to the raw I/O handles.

CMD should be able to do it. Perhaps PowerShell should bend users towards using the object model instead.

@vexx32
Copy link
Collaborator

vexx32 commented Aug 24, 2020

/cc @SteveL-MSFT not sure who best to answer @DHowett's question regarding similarly reserved names on MacOS/Linux, but maybe worth opening an issue for? 🤔

Otherwise... yep, agreed. 🙂

@iSazonov
Copy link
Collaborator

iSazonov commented Aug 24, 2020

Do we have a holistic story that covers how powershell handles devicelike file handles in general? If not, is this just putting a band-aid over a problem?

We follow the naming convention https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file#naming-conventions but CONIN$/CONOUT$ is not documented there.

I don't know how we handle them in a world where pwsh also runs on Linux (and can access /dev/sda1, or /dev/fd/0 which is a lot like CONOUT$) and macOS (/dev/rdisk0, /dev/tty).

In Unix-s they are files. Users could do cd ~; mkdir /dev/sda1 and access this by cat ./dev/sda1. But in Windows ./ prefix optional and users can do type CONOUT$ and it is not clear whether this a file or a device.

All file systems supported by Windows use the concept of files and directories to access data stored on a disk or device.

@ghost ghost added the Review - Needed The PR is being reviewed label Sep 1, 2020
@ghost
Copy link

ghost commented Sep 1, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

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.

Perhaps IsReservedDeviceName() is on hot path and we could measure and optimize its performance in follow PR.

if (((compareName.Length < 3) || (compareName.Length > 6)) &&
((noExtensionCompareName.Length < 3) || (noExtensionCompareName.Length > 6)))
if (((compareName.Length < 3) || (compareName.Length > 7)) &&
((noExtensionCompareName.Length < 3) || (noExtensionCompareName.Length > 7)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is not blocking. It looks to me we only need to check on noExtensionCompareName. The checking on compareName seems not needed. /cc @anmenaga @iSazonov

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I open new tracking issue #13618.

@iSazonov
Copy link
Collaborator

@anmenaga The PR is ready to merge.

@ghost ghost removed the Review - Needed The PR is being reviewed label Sep 11, 2020
@anmenaga anmenaga merged commit 3effa20 into PowerShell:master Sep 14, 2020
@iSazonov
Copy link
Collaborator

@davidreis97 Thanks for your contribution!

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.

IsReservedDeviceName does not recognize CONIN$, CONOUT$
6 participants