Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 18 additions & 0 deletions src/Framework.UnitTests/AbsolutePath_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -308,5 +308,23 @@ public void AbsolutePath_NotRooted_ShouldThrowWithLocalizedMessage()
var exception = Should.Throw<ArgumentException>(() => new AbsolutePath("relative/path"));
exception.Message.ShouldContain("Path must be rooted");
}

[WindowsOnlyFact]
public void AbsolutePath_WhitespacePath_ShouldThrowOnWindows()
{
var basePath = GetTestBasePath();

Should.Throw<ArgumentException>(() => new AbsolutePath(" ", basePath));
}

[UnixOnlyFact]
public void AbsolutePath_WhitespacePath_ShouldNotThrowOnUnix()
{
var basePath = GetTestBasePath();

// Whitespace is a valid filename character on Unix — should not throw.
var absolutePath = new AbsolutePath(" ", basePath);
absolutePath.Value.ShouldNotBeNullOrEmpty();
}
}
}
10 changes: 10 additions & 0 deletions src/Framework/PathHelpers/AbsolutePath.cs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,16 @@ public AbsolutePath(string path, AbsolutePath basePath)
{
ArgumentException.ThrowIfNullOrEmpty(path);

// Before MSBuild's migration from multi-process to multi-threaded, Path.GetFullPath(" ") threw
// ArgumentException on Windows because whitespace-only is not a legal path. After combining with
// a rooted base directory, Path.GetFullPath silently trims the trailing whitespace, masking the
// error. Preserve the historical Windows contract by explicitly rejecting whitespace-only input.
// Unix retains the historical accepting behavior because whitespace is a valid filename character there.
if (NativeMethods.IsWindows && string.IsNullOrWhiteSpace(path))
{
throw new ArgumentException(SR.WhitespacePathNotAllowedOnWindows, nameof(path));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that we should have in multiple places code that checks the input before passing it to the AbsolutePath constructor, in order to make sure that the constructor does not throw. We probably should check all of those places and change IsNullOrEmpty check to IsNullOrWhiteSpace.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If there is a common pattern for that, we probably should make a helper method internally.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

note that whitespace is a valid filename on *nix 😉

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is a very risky change as any task that uses AbsolutePath may now start throwing - and this would be change of behavior. We need to be careful with it and make another review of all enlightened tasks that use AbsolutePath

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, I think we probably should make a separate issue and PR for this.

}

// This function should not throw when path has illegal characters.
// For .NET Framework, Microsoft.IO.Path.Combine should be used instead of System.IO.Path.Combine to achieve it.
// For .NET Core, System.IO.Path.Combine already does not throw in this case.
Expand Down
3 changes: 3 additions & 0 deletions src/Framework/Resources/SR.resx
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,9 @@
<data name="PathMustBeRooted" xml:space="preserve">
<value>Path must be rooted.</value>
</data>
<data name="WhitespacePathNotAllowedOnWindows" xml:space="preserve">
<value>Path cannot be null or whitespace on Windows.</value>
</data>
<data name="PathTooLong" xml:space="preserve">
<value>Path: {0} exceeds the OS max path limit. The fully qualified file name must be less than {1} characters.</value>
</data>
Expand Down
7 changes: 6 additions & 1 deletion src/Framework/Resources/xlf/SR.cs.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion src/Framework/Resources/xlf/SR.de.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion src/Framework/Resources/xlf/SR.es.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion src/Framework/Resources/xlf/SR.fr.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion src/Framework/Resources/xlf/SR.it.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion src/Framework/Resources/xlf/SR.ja.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion src/Framework/Resources/xlf/SR.ko.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion src/Framework/Resources/xlf/SR.pl.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion src/Framework/Resources/xlf/SR.pt-BR.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion src/Framework/Resources/xlf/SR.ru.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion src/Framework/Resources/xlf/SR.tr.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion src/Framework/Resources/xlf/SR.zh-Hans.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion src/Framework/Resources/xlf/SR.zh-Hant.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading