Skip to content

Make CreateSubdirectory() work in root directory #49414

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

Closed
trajanmcgill wants to merge 1 commit into from
Closed

Make CreateSubdirectory() work in root directory #49414

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 8 additions & 13 deletions src/libraries/System.IO.FileSystem/src/System/IO/DirectoryInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,20 +74,15 @@ public DirectoryInfo CreateSubdirectory(string path)

string newPath = Path.GetFullPath(Path.Combine(FullPath, path));

ReadOnlySpan<char> trimmedNewPath = Path.TrimEndingDirectorySeparator(newPath.AsSpan());
ReadOnlySpan<char> trimmedCurrentPath = Path.TrimEndingDirectorySeparator(FullPath.AsSpan());

// We want to make sure the requested directory is actually under the subdirectory.
if (trimmedNewPath.StartsWith(trimmedCurrentPath, PathInternal.StringComparison)
// Allow the exact same path, but prevent allowing "..\FooBar" through when the directory is "Foo"
&& ((trimmedNewPath.Length == trimmedCurrentPath.Length) || PathInternal.IsDirectorySeparator(newPath[trimmedCurrentPath.Length])))
{
FileSystem.CreateDirectory(newPath);
return new DirectoryInfo(newPath);
}

// We weren't nested
throw new ArgumentException(SR.Format(SR.Argument_InvalidSubPath, path, FullPath), nameof(path));
// Allow the exact same path, but prevent allowing "..\FooBar" through when the directory is "Foo"
Copy link
Contributor

@carlossanlop carlossanlop Mar 23, 2021

Choose a reason for hiding this comment

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

Have you verified that this behavior is not being regressed?

Copy link
Author

@trajanmcgill trajanmcgill Mar 26, 2021

Choose a reason for hiding this comment

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

Best I can tell, that comment was added, and behavior fixed, here: 6c41cc9
Which added or touched three tests:
SubDirectoryIsParentDirectory_ThrowsArgumentException()
ValidPathWithTrailingSlash()
ValidPathWithoutTrailingSlash()
All three of those do still pass. (At least on Linux. I've had only headaches trying to build this project on Windows so far, so haven't yet verified on that platform.)

string newPathWithTrailingSeparator = PathInternal.EnsureTrailingSeparator(newPath);
string currentPathWithTrailingSeparator = PathInternal.EnsureTrailingSeparator(FullPath);
if (!newPathWithTrailingSeparator.StartsWith(currentPathWithTrailingSeparator, PathInternal.StringComparison))
throw new ArgumentException(SR.Format(SR.Argument_InvalidSubPath, newPath, FullPath), nameof(path));
Comment on lines +81 to +82
Copy link
Contributor

@carlossanlop carlossanlop Mar 23, 2021

Choose a reason for hiding this comment

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

Suggested change
if (!newPathWithTrailingSeparator.StartsWith(currentPathWithTrailingSeparator, PathInternal.StringComparison))
throw new ArgumentException(SR.Format(SR.Argument_InvalidSubPath, newPath, FullPath), nameof(path));
if (!newPathWithTrailingSeparator.StartsWith(currentPathWithTrailingSeparator, PathInternal.StringComparison))
{
throw new ArgumentException(SR.Format(SR.Argument_InvalidSubPath, newPath, FullPath), nameof(path));
}

Copy link
Author

@trajanmcgill trajanmcgill Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is perfectly fine with me, but out of curiosity, what's the reason we'd add braces around this one when all the other if(X) throw Y; lines that appear just above this one simply use indentation for the single-statement block? I did read the code style guidelines for the project, but I guess I must have missed something there.

Copy link
Contributor

@iSazonov iSazonov Mar 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We could use (this was fixed in #45139)

runtime/src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/InMemoryDirectoryInfo.cs

Lines 120 to 127 in 72e41f9

private bool IsRootDirectory(string rootDir, string filePath)
{
int rootDirLength = rootDir.Length;
return filePath.StartsWith(rootDir, StringComparison.Ordinal) &&
(rootDir[rootDirLength - 1] == Path.DirectorySeparatorChar ||
filePath.IndexOf(Path.DirectorySeparatorChar, rootDirLength) == rootDirLength);
}

Copy link
Author

@trajanmcgill trajanmcgill Mar 31, 2021

Choose a reason for hiding this comment

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

@iSazonov - Thanks for the suggestion. How are you proposing we put that function to use? The solution I've proposed up to this point doesn't actually do a special-case check for being a root directory at all; instead, it adjusts the logic for one of the forbidden cases so that it doesn't accidentally also disallow creating subdirectories of the root directory.

Copy link
Contributor

@iSazonov iSazonov Mar 31, 2021

Choose a reason for hiding this comment

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

doesn't actually do a special-case check for being a root directory at all

The comment in line 77 suggests otherwise. Actually FullPath must be a root path for newPath.

Copy link
Author

@trajanmcgill trajanmcgill Apr 1, 2021

Choose a reason for hiding this comment

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

Oh, when you say "root path" do you mean "parent path" of another path? Because "IsRootDirectory" to me is a method name that suggests it checks to see if a path is an actual root directory, an absolute root, as in "/" or "C:" or the like. Especially since this bug we're addressing here is specifically a failure in the case where it is used on a drive root directory, I was thrown off by the ambiguity of the term "root". (In fact, I'd suggest maybe the above method should be called "IsParentDirectory" instead.)

In any case, I'll take a look at that- need to make sure using it doesn't regress what was being fixed by 6c41cc9.

Copy link
Contributor

@iSazonov iSazonov Apr 1, 2021

Choose a reason for hiding this comment

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

I agree that the method name confuses.


FileSystem.CreateDirectory(newPath);
return new DirectoryInfo(newPath);
}

public void Create()
Expand Down