Runtime: `System.IO.FileInfo` doesn't throw for invalid paths

Created on 27 Jun 2020  路  8Comments  路  Source: dotnet/runtime

Description

I'm porting https://github.com/gitextensions/gitextensions/pull/7162 from .NET Framework 4.6.1 to .NET Core 3.1 and observing a number of tests fail. In particular tests for functionality that checks validity of user supplied paths.

The code:
https://github.com/gitextensions/gitextensions/blob/b210331fb67b2362cc1c666ad49911d4666d85e0/GitCommands/EnvironmentPathsProvider.cs#L73-L90

.NET Framework version of FileInfo ctor used to throw if fileName contains invalid an invalid path, e.g.:
image

However .NET Core (3.1.200) doesn't throw, despite the docs state it would:
image

https://docs.microsoft.com/en-us/dotnet/api/system.io.fileinfo.-ctor?view=netcore-3.1#exceptions
image
image

Configuration


.NET Core 3.1.5 (latest official)
W10 1909 x64

Regression?

It looks like a regression/breaking change but I couldn't find any docs pertaining to the change (looked here and here)

.NET Framework version of the API throws, if an invalid path is passed into ctor
image

Other information

area-System.IO

Most helpful comment

The only way to know a path is valid is to actually try to use it. Trying to check for "invalid characters" falls apart in a number of scenarios, notably when using file systems that aren't "normal" for the host OS (and in many other corner cases). If you really want to check invalid characters you can do that yourself with Path.InvalidPathChars, but I wouldn't recommend it outside of recommending not to use a specific filename. That said, it isn't a great filter. I've created some API suggestions for "better" pick-a-good-name helpers, such as #25010, but they haven't bubbled up to the top of the stack.

The only thing we check for now is embedded nulls as most OS APIs work off of null terminated strings and understanding the results in this case would be difficult. There is also likely some creative security holes with embedded nulls but I haven't spent a great deal of time thinking that angle through (would make it difficult to compare paths for containment).

All 8 comments

Looks like the same applies to DirectoryInfo class too, no more exceptions for invalid paths.

@RussKie The resource for that exception message was removed in https://github.com/dotnet/corefx/pull/23851/files, but can't find when this resource became unused.

In .NET Framework, the ctor was making a call to Init which calls GetFullPathInternal, which then calls NormalizePath that will delegate at some point to NewNormalizePath, the exception is thrown there.

That's just a quick tracing (not 100% sure of its correctness, but just in case it's useful).

I'm wondering if even the behavior you get on .NET Framework is correct? Shouldn't it be NotSupportedException based on docs?


Update: I believe that Path.GetFullPath should be broken too (haven't tested). My reasoning for that is:

The docs says it should throw if the path is invalid. The GetFullPath is indeed called in the FileInfo ctor. If FileInfo ctor didn't throw, that mean GetFullPath didn't throw. (because I don't see any try/catch). So, the root cause could be Path.GetFullPath (Fixing GetFullPath will fix both DirectoryInfo and FileInfo).

But again, I'm not 100% sure.


Update 2: I went to .NET Core 1.0 branch, and I think this is the relevant code that was throwing in that case. But I haven't check in which release or exact commit this was removed.

I believe this is a known breaking change that was introduced in .NET Core 2.1. It was blogged about in the post System.IO in .NET Core 2.1 sneak peek. Under "Key behavior changes", one of the items is:

Exceptions for "bad" paths are thrown when _used_, not when normalizing (as we can't know what is "valid" without using them)

and in FAQ:

What exceptions do I get from bad paths now?
Whatever the OS tells us, we'll tell you. This will always be some sort of IOException for a bad path. Sometimes it might be a File/DirectoryNotFound. The key thing to remember is that you get exceptions when you try to use the path. GetFullPath() doesn't throw for these as it has no practical way of checking.

In your case, constructing a FileInfo doesn't do anything to access the file on disk. However if you attempt to get information about the file, like accessing the Length property, it is then it will throw for ill-formed paths.

For example, if I do:

c# static void Main() { _ = new FileInfo("c::\\word\\").Length; }

That causes an exception:

System.IO.IOException: The filename, directory name, or volume label syntax is incorrect. : 'C:\:\word\'

It looks like the documentation doesn't reflect this change in Core 2.1.

Thank you for your help 馃憤

The question still remains - who does one definitely check for a given path to be a valid path?
Invoking a property on FileInfo instance can get us so far. Some paths are user supplied, some paths are concatenated from bits sourced from various sources, and not all paths necessarily physically present on a hard drive (those may be fed to git).

E.g. c:\Programs\Get"\ isn't valid, yet the new FileInfo happily creates an object for it.

  • .NET 4.6.1
    image
  • .NET Core 3.1
    image

The only way to know a path is valid is to actually try to use it. Trying to check for "invalid characters" falls apart in a number of scenarios, notably when using file systems that aren't "normal" for the host OS (and in many other corner cases). If you really want to check invalid characters you can do that yourself with Path.InvalidPathChars, but I wouldn't recommend it outside of recommending not to use a specific filename. That said, it isn't a great filter. I've created some API suggestions for "better" pick-a-good-name helpers, such as #25010, but they haven't bubbled up to the top of the stack.

The only thing we check for now is embedded nulls as most OS APIs work off of null terminated strings and understanding the results in this case would be difficult. There is also likely some creative security holes with embedded nulls but I haven't spent a great deal of time thinking that angle through (would make it difficult to compare paths for containment).

Was this page helpful?
0 / 5 - 0 ratings

Related issues

Timovzl picture Timovzl  路  3Comments

matty-hall picture matty-hall  路  3Comments

yahorsi picture yahorsi  路  3Comments

v0l picture v0l  路  3Comments

jamesqo picture jamesqo  路  3Comments