Windows path tweaks by JeremyKuhne · Pull Request #8669 · dotnet/corefx

JeremyKuhne · 2016-05-18T23:45:17Z

Simplify the path length check to match desktop
Tweak IsPartiallyQualified to match desktop
Update wildcards to actually match what Windows supports

It came as a surprise to me, but yes, there are more wildcard characters in Windows than "*" and "?".

https://msdn.microsoft.com/en-us/library/ff469270.aspx

You can use these other wildcards from the CMD prompt as long as you quote and escape the quote if you use it. (It is a FindFirstFile thing)

@ianhays, @ericstj, @stephentoub

stephentoub · 2016-05-19T13:11:14Z

src/Common/src/System/IO/PathInternal.Windows.cs


        internal static readonly char[] InvalidPathChars =
        {
-            '\"', '<', '>', '|', '\0',


These characters can be used in file paths passed to Win32 functions like CreateFile?

FindFirstFile is where the wildcards come into play- and is why we have the current ones (*, ?) broken out. We check all characters in GetFullPath.

What does or doesn't work with CreateFile is device/file system dependent. You can use most of these with pipes, for example. Null and backslash are the most "reserved" of the lot as the null terminates string logic and the backslash is the separator the object manager uses.

If this is changed, then I believe the function I recently modified (HasIllegalCharacters) needs to be changed as well to eliminate checks for these characters.

ianhays · 2016-05-20T19:20:52Z

Won't this affect the behavior of some FileSystem functions? They use PathInternal.InvalidPathChars. I don't think we have tests in place to see what happens when they pass a (potentially valid) path that contains one of these unusual search characters?

I'm particularly interested in whether a search pattern with one of these is allowed and passed to the win32 functions or if it throws an error before it gets the chance.

JeremyKuhne · 2016-05-20T22:25:04Z

@ianhays Only the ones that don't do the full check (which includes the wildcards). That behavior is by design so you can actually utilize wildcards where it makes sense.

Path.GetFullPath checks all characters.

jamesqo · 2016-05-21T18:33:40Z

src/Common/src/System/IO/PathInternal.Windows.cs

        internal unsafe static bool HasWildCardCharacters(string path)
        {
            // Question mark is part of extended syntax so we have to skip if we're extended
            int startIndex = PathInternal.IsExtended(path) ? ExtendedPathPrefix.Length : 0;


startIndex isn't being used anymore in the new code...

Whoops- thanks! I'll add some tests.

JeremyKuhne · 2016-05-24T20:00:30Z

Test Innerloop CentOS7.1 Debug Build and Test please

JeremyKuhne · 2016-05-26T18:54:05Z

@ianhays, @stephentoub Are you ok with this?

ianhays · 2016-05-26T19:10:36Z

stephentoub · 2016-06-03T13:30:06Z

Are the same wildcard changes being made to desktop?

JeremyKuhne · 2016-06-03T19:52:18Z

@stephentoub, that's the intent

stephentoub · 2016-08-19T22:49:50Z

@JeremyKuhne, what's the status of this PR?

JeremyKuhne · 2016-09-01T17:43:38Z

I'll update for #11293. @jamesqo

jamesqo · 2016-09-02T17:12:04Z

src/Common/src/System/IO/PathInternal.Windows.cs

        internal static readonly char[] InvalidPathChars =
        {
-            '\"', '<', '>', '|', '\0',
+             '|', '\0',


Nit: indentation is off here

JeremyKuhne · 2016-09-07T02:40:51Z

@stephentoub I've updated the PR, are you ok with this change?

stephentoub · 2016-09-07T02:43:20Z

src/Common/src/System/IO/PathInternal.Windows.cs

        internal static char[] GetInvalidPathChars() => new char[]
        {
-            '\"', '<', '>', '|', '\0',
+             '|', '\0',


Nit: extra space before the first single quote

stephentoub · 2016-09-07T02:45:39Z

@ericstj, @weshaggard, thoughts on the wildcard change?

JeremyKuhne · 2016-09-07T04:32:17Z

@weshaggard I thought a testproject pkgproj reference was supposed to pull in the local bits? I can't seem to get this to pull in the right binary any more- tried a direct reference to the src csproj as well but the Random and StringComparer tests can't compile.

weshaggard · 2016-09-07T04:56:41Z

@ericstj, @weshaggard, thoughts on the wildcard change?

I don't have much context on the actual change or the risk it has but I do worry a little about the compat on desktop. @JeremyKuhne what do you think the compat risk for this change will be?

@weshaggard I thought a testproject pkgproj reference was supposed to pull in the local bits? I can't seem to get this to pull in the right binary any more- tried a direct reference to the src csproj as well but the Random and StringComparer tests can't compile.

A pkgproj reference will pull in the most appropriate asset it might be the local built or not it really depends on what the test project is building against. Does the TargetGroup for the test project match what you want it to pull from the source project?

jamesqo · 2016-09-07T11:25:05Z

src/Common/src/System/IO/PathInternal.Windows.cs

@@ -210,8 +195,6 @@ internal static bool HasIllegalCharacters(string path)
                switch (c)
                {
                    case '"':


Is it intentional '"' is still kept here when it it removed from the array?

(And assuming it's not, seems like we're missing a test.)

Doh. Thanks- trying to code when sick- I did have a test and it is failing. 😷 I'm going to move the one remaining case up to the other if block.

ericstj · 2016-09-07T15:59:10Z

I imagine this will have the effect of changing an ArgumentException to an IOException in some cases, which would make it a breaking change on Desktop that likely requires a quirk.

JeremyKuhne · 2016-09-07T17:55:28Z

I don't have much context on the actual change or the risk it has but I do worry a little about the compat on desktop. @JeremyKuhne what do you think the compat risk for this change will be?

And...

I imagine this will have the effect of changing an ArgumentException to an IOException in some cases, which would make it a breaking change on Desktop that likely requires a quirk.

I think the risk is pretty low. Most places will still kick the path- the reason they're (bad and wildcard chars) separated is to allow enumerating. That said, I intend to put the change behind the existing normalization quirk on desktop.

- Simplify the path length check to match desktop - Tweak IsPartiallyQualified to match desktop - Update wildcards to actually match what Windows supports

JeremyKuhne · 2016-09-07T18:15:48Z

A pkgproj reference will pull in the most appropriate asset it might be the local built or not it really depends on what the test project is building against. Does the TargetGroup for the test project match what you want it to pull from the source project?

I'm a bit under the weather- it was working as expected and failing as expected. Thanks.

JeremyKuhne · 2016-09-07T20:26:18Z

@ericstj, @stephentoub, @weshaggard, @jamesqo Updated and green.

stephentoub · 2016-09-07T23:43:04Z

As long as we're ok with the behavior change, LGTM.

Windows path tweaks Commit migrated from dotnet/corefx@12e18e2

dnfclas added the cla-already-signed label May 18, 2016

dotnet-bot added the 2 - In Progress label May 18, 2016

stephentoub reviewed May 19, 2016
View reviewed changes

stephentoub added the netfx-port-consider label May 19, 2016

JeremyKuhne force-pushed the pathcleanup branch from 934cba6 to 1d6f87b Compare May 20, 2016 17:29

jamesqo reviewed May 21, 2016
View reviewed changes

JeremyKuhne force-pushed the pathcleanup branch from 1d6f87b to 422f54c Compare May 23, 2016 20:58

joshfree assigned JeremyKuhne Jun 20, 2016

joshfree added this to the 1.1.0 milestone Jun 20, 2016

jamesqo reviewed Sep 2, 2016
View reviewed changes

JeremyKuhne force-pushed the pathcleanup branch from 422f54c to 7e7f67f Compare September 7, 2016 02:38

stephentoub reviewed Sep 7, 2016
View reviewed changes

JeremyKuhne force-pushed the pathcleanup branch from 7e7f67f to 3c47374 Compare September 7, 2016 02:48

jamesqo reviewed Sep 7, 2016
View reviewed changes

Windows path tweaks

50cb4c1

- Simplify the path length check to match desktop - Tweak IsPartiallyQualified to match desktop - Update wildcards to actually match what Windows supports

JeremyKuhne force-pushed the pathcleanup branch from 3c47374 to 50cb4c1 Compare September 7, 2016 18:13

JeremyKuhne merged commit 12e18e2 into dotnet:master Sep 9, 2016

dotnet-bot removed the 2 - In Progress label Sep 9, 2016

JeremyKuhne mentioned this pull request Oct 17, 2016

", >, and < are valid wildcard characters dotnet/coreclr#3942

Closed

JeremyKuhne deleted the pathcleanup branch January 3, 2017 21:28

ianhays mentioned this pull request Mar 29, 2017

Update IO test for new Wildcard chars #17657

Merged

vcsjones mentioned this pull request Nov 11, 2020

Path.GetInvalidPathChars() doesn't include angle brackets dotnet/runtime#44520

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#8669 from JeremyKuhne/pathcleanup

4877f59

Windows path tweaks Commit migrated from dotnet/corefx@12e18e2

GerardSmit mentioned this pull request Oct 29, 2025

Path. GetInvalidPathChars() does not contain ">" on windows dotnet/runtime#121162

Open

Conversation

JeremyKuhne commented May 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianhays commented May 20, 2016

Uh oh!

JeremyKuhne commented May 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyKuhne commented May 24, 2016

Uh oh!

JeremyKuhne commented May 26, 2016

Uh oh!

ianhays commented May 26, 2016

Uh oh!

stephentoub commented Jun 3, 2016

Uh oh!

JeremyKuhne commented Jun 3, 2016

Uh oh!

stephentoub commented Aug 19, 2016

Uh oh!

JeremyKuhne commented Sep 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyKuhne commented Sep 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Sep 7, 2016

Uh oh!

JeremyKuhne commented Sep 7, 2016

Uh oh!

weshaggard commented Sep 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericstj commented Sep 7, 2016

Uh oh!

JeremyKuhne commented Sep 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyKuhne commented Sep 7, 2016

Uh oh!

JeremyKuhne commented Sep 7, 2016

Uh oh!

stephentoub commented Sep 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

JeremyKuhne commented Sep 7, 2016 •

edited

Loading