System.IO.DirectoryInfo CreateSubdirectory method fails when DirectoryInfo refers to a root directory

[trajanmcgill]

Description

The below code fails to create a subdirectory and throws an exception.

using System;
using System.IO;

namespace CreateSubdirectoryTest
{
    class Program
    {
        static void Main(string[] args)
        {
            DirectoryInfo directory = new DirectoryInfo("T:\\"); // Or any other root directory, including / on Linux.
            directory.CreateSubdirectory("foo");
        }
    }
}

Expected:
T:\foo is created (or /foo, if doing the equivalent on Linux).

Actual:

System.ArgumentException: The directory specified, 'foo', is not a subdirectory of 'T:'. (Parameter 'path')

Configuration

.NET Version: 5.0.3
OS: Windows 10 or Linux

Other information

The exception message is a very odd one for the given circumstance, which led me to explore
runtime/src/libraries/System.IO.FileSystem/src/System/IO/DirectoryInfo.cs,
where I found the following definition of CreateSubdirectory():

public DirectoryInfo CreateSubdirectory(string path)
{
    if (path == null)
        throw new ArgumentNullException(nameof(path));
    if (PathInternal.IsEffectivelyEmpty(path.AsSpan()))
        throw new ArgumentException(SR.Argument_PathEmpty, nameof(path));
    if (Path.IsPathRooted(path))
        throw new ArgumentException(SR.Arg_Path2IsRooted, nameof(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));
}

I believe the above is guaranteed to fail for any path (e.g., foo) if FullPath is something like T:\ or / because:

1. newPath will be T:\foo (Linux: /foo)
2. trimmedNewPath will be T:\foo (/foo)
3. trimmedCurrentPath will be T:\ (/) <-- the outcome of this step is the key thing I think the above code fails to anticipate.
4. The trimmedNewPath.StartsWith() condition is fulfilled, but:
5. trimmedCurrentPath.Length == 3, and newPath[3] == 'f' (or, for the Linux root path, the length is 1, which results in the same: the tested character of newPath being 'f'), so the PathInternal.IsDirectorySeparator() test against that character fails, allowing execution to pass to throwing an exception.

The issue is that for a directory such as T:\ or /, Path.TrimEndingDirectorySeparator() does not remove the trailing separator, because it is not just a separator; it is actually the path.

Suggested Solution

It looks to me like what the above code is trying to accomplish is to verify that the full starting path is either identical to or a substring of the finished new full path and make sure that the new part is tacked on on the other side of a directory separator from the old part.

Rather than trimming the directory separator from the original full path, then testing for the new one to start with the original followed by a directory separator, perhaps it would be more accurate to add a directory separator to the original if missing, and then simply test that the new full path starts with [original full path including separator]. That would also have the benefit of making the purpose of the if condition easier to understand by making its expression much simpler.

[trajanmcgill]

This is a version of the method which I think would fix the problem, and be a little cleaner as well:

public DirectoryInfo CreateSubdirectory(string path)
{
    if (path == null)
        throw new ArgumentNullException(nameof(path));
    if (PathInternal.IsEffectivelyEmpty(path.AsSpan()))
        throw new ArgumentException(SR.Argument_PathEmpty, nameof(path));
    if (Path.IsPathRooted(path))
        throw new ArgumentException(SR.Arg_Path2IsRooted, nameof(path));

    // We want to make sure the requested directory is actually under the subdirectory.
    // Allow the exact same path, but prevent allowing "..\FooBar" through when the directory is "Foo"
    string newPath = PathInternal.EnsureTrailingSeparator(Path.GetFullPath(Path.Combine(FullPath, path)));
    string currentPath = PathInternal.EnsureTrailingSeparator(FullPath);
    if (!newPath.StartsWith(currentPath, PathInternal.StringComparison))
        throw new ArgumentException(SR.Format(SR.Argument_InvalidSubPath, path, FullPath), nameof(path));

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

However, there are a few things I don't know the answer to:

1. Are the arguments passed to the ArgumentException constructor correct? I didn't change them above, but I suspect they were incorrect already-- I think the second argument to SR.Format() perhaps should be newPath, not path. As it is, the error message created by this doesn't seem to me to quite make sense.

2. I'm not sure what an appropriate test method would be for the case I'm trying to fix (creating a directory off of the root). All the existing tests create their files and directories in a test location, and the root of a drive or filesystem seems a problematic place to be trying to create things in a test.

3. A whole host of existing tests fail with the above code. But I'm not sure if they should-- they may simply be relying on non-required behavior of the existing implementation of CreateSubdirectory(). Here's what is going on: A DirectoryInfo object's FullName property does not have consistent behavior regarding trailing directory separators. (new DirectoryInfo("/foo/bar")).FullName will give you "/foo/bar", whereas (new DirectoryInfo("/foo/bar/")).FullName is "/foo/bar/". Which is to say, the FullName property seems to preserve the presence or lack of trailing separator in the string that was originally passed to the constructor. The existing behavior of DirectoryInfo.CreateSubdirectory() returns a new DirectoryInfo object created without the trailing separator; my version above returns one created with the separator. That breaks some of the tests of this class, because they're testing the FullName property of the returned DirectoryInfo against a particular expected value. One could make the case that the tests could just change, since the separator's absence isn't really what they are testing for; it's just something they rely on in their logic. On the other hand, maybe lots of production code out there also makes the same assumption and therefore this behavior should be maintained, which would require a slight adjustment to the above, to strip the trailing separator from newPath before using it in the returned object. I did verify that a version of the above that instead uses a trimmed path in the return object does seem to pass all existing tests.

[carlossanlop]

Thanks for the detailed info, @trajanmcgill, and for sending the PR.

[trajanmcgill]

@carlossanlop no problem, hopefully my fix is suitable. I might need a little help walking me through getting all the pieces in place, though; I haven't contributed to this project before, and I'm not sure how to design tests in accordance with this project's standards for the bug my PR addresses. Nor am I sure why my PR failed several of the automated checks that don't seem to have anything to do with the changes I made.

[SuperJMN]

This has caused me an issue. Please, add fix this ASAP. Thanks.

[danmoseley]

@SuperJMN would you like to pick up the draft PR and complete it?

[SuperJMN]

@SuperJMN would you like to pick up the draft PR and complete it?

I can try. What has been done? What's missing?

[danmoseley]

@SuperJMN, I see there is a PR #49414 that @trajanmcgill was not able to complete. Not sure of how far they got.