Skip to content
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.

Sign up for GitHub

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

Path.GetFullPath(string, string) uses case insensitive volume name comparison for drive rooted, relative paths #97759

Merged
merged 12 commits into from
Mar 19, 2024
Merged

Path.GetFullPath(string, string) uses case insensitive volume name comparison for drive rooted, relative paths #97759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
fe4d622
check root for drive relative paths case insensitive.
AnakinRaW Jan 29, 2024
c1d71cb
Merge branch 'dotnet:main' into caseinsensitive-driverelative-path
AnakinRaW Jan 31, 2024
112a318
Merge branch 'main' into caseinsensitive-driverelative-path
AnakinRaW Jan 31, 2024
20067da
Merge branch 'main' into caseinsensitive-driverelative-path
AnakinRaW Feb 6, 2024
9007951
Merge branch 'main' into caseinsensitive-driverelative-path
AnakinRaW Feb 19, 2024
f926509
Merge branch 'main' into caseinsensitive-driverelative-path
AnakinRaW Feb 19, 2024
f03503a
Merge branch 'main' into caseinsensitive-driverelative-path
AnakinRaW Feb 23, 2024
4dd5b29
Remove link
AnakinRaW Mar 16, 2024
47cb32e
Merge branch 'main' into caseinsensitive-driverelative-path
AnakinRaW Mar 16, 2024
0acd5ce
apply feedback
AnakinRaW Mar 16, 2024
a9b320f
Merge branch 'main' into caseinsensitive-driverelative-path
AnakinRaW Mar 16, 2024
1d43a6b
Merge branch 'main' into caseinsensitive-driverelative-path
jozkee Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public static string GetFullPath(string path, string basePath)
// Drive relative paths
Debug.Assert(length == 2 || !PathInternal.IsDirectorySeparator(path[2]));

if (GetVolumeName(path.AsSpan()).EqualsOrdinal(GetVolumeName(basePath.AsSpan())))
if (GetVolumeName(path.AsSpan()).EqualsOrdinalIgnoreCase(GetVolumeName(basePath.AsSpan())))
Copy link
Member

Choose a reason for hiding this comment

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

There are other places in this file where EqualsOrdinal is used, can you change those as well?

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs

Lines 349 to 353 in 7a60900

if (!isDevice && path.Slice(0, 2).EqualsOrdinal(@"\\".AsSpan()))
return 2;
else if (isDevice && path.Length >= 8
&& (path.Slice(0, 8).EqualsOrdinal(PathInternal.UncExtendedPathPrefix.AsSpan())
|| path.Slice(5, 4).EqualsOrdinal(@"UNC\".AsSpan())))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure. done that. However, I omitted EqualsOrdinal(@"\\".AsSpan() as there is nothing to compare case-insensitive. If it should be consistent though, i can chance again of course.

Copy link

@rhuijben rhuijben Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Are you sure this is correct for \\SERVER\SHARE as I think this is a valid volume name on Windows.

In that case the server name should be case insensitive, but the share name might not (Think non windows OSs like Samba on linux).

Testcases on these paths are usually hard, as this call might try contacting the server...

Copy link
Member

Choose a reason for hiding this comment

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

GetVolumeName is only used when path is drive relative like C:foo, it's used to check whether path and basePath have the same root, the method is probably more complex than it needs to but I don't think it would affect UNC paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually that was my thought when applying the changes :)

{
// Matching root
// "C:Foo" and "C:\Bar" => "C:\Bar\Foo"
Expand Down Expand Up @@ -349,8 +349,8 @@ internal static int GetUncRootLength(ReadOnlySpan<char> path)
if (!isDevice && path.Slice(0, 2).EqualsOrdinal(@"\\".AsSpan()))
return 2;
else if (isDevice && path.Length >= 8
&& (path.Slice(0, 8).EqualsOrdinal(PathInternal.UncExtendedPathPrefix.AsSpan())
|| path.Slice(5, 4).EqualsOrdinal(@"UNC\".AsSpan())))
&& (path.Slice(0, 8).EqualsOrdinalIgnoreCase(PathInternal.UncExtendedPathPrefix.AsSpan())
|| path.Slice(5, 4).EqualsOrdinalIgnoreCase(@"UNC\".AsSpan())))
return 8;

return -1;
Expand Down
2 changes: 2 additions & 0 deletions ...aries/System.Runtime/tests/System.Runtime.Extensions.Tests/System/IO/PathTests_Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,8 @@ public void GetFullPath_CommonDevice_Windows(string path, string basePath, strin
{ @"C:tmp", @"C:\git\runtime", @"C:\git\runtime\tmp" },
{ @"C:", @"C:\git\runtime", @"C:\git\runtime" },
{ @"C", @"C:\git\runtime", @"C:\git\runtime\C" },
{ @"c:", @"C:\git\runtime", @"C:\git\runtime" },
{ @"C:tmp", @"c:\git\runtime", @"c:\git\runtime\tmp" },

{ @"Z:tmp\foo\..", @"C:\git\runtime", @"Z:\tmp" },
{ @"Z:tmp\foo\.", @"C:\git\runtime", @"Z:\tmp\foo" },
Expand Down
Loading