Skip to content

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

Merged
anmenaga merged 3 commits into
PowerShell:masterfrom
davidreis97:13046-moreReservedNames
Sep 14, 2020
Merged

Recognize CONOUT$ and CONIN$ as reserved device names#13508
anmenaga merged 3 commits into
PowerShell:masterfrom
davidreis97:13046-moreReservedNames

Conversation

@davidreis97
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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