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.

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

Allow users that have certain special characters in their username to build successfully when using exec #6223

Merged
merged 22 commits into from
Apr 8, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
1314a77
Initial fix. Escape parentheses that exist in the path of the generat…
benvillalobos Mar 4, 2021
9084023
Replace both parens at the same time
benvillalobos Mar 4, 2021
19c9e4c
Add test case for parentheses. Escape all instances of parens or &
benvillalobos Mar 5, 2021
94dde5c
Clean up unit test
benvillalobos Mar 8, 2021
7ce00b3
Name folder generated in TEMP as 'MSBuild' instead of 'Temporary'
benvillalobos Mar 8, 2021
95fca99
This fix is specifically for windows
benvillalobos Mar 8, 2021
34126e1
Append characters one at a time.
benvillalobos Mar 8, 2021
1c5a2b3
Update src/Tasks/Exec.cs
benvillalobos Mar 8, 2021
c268b32
Revert "Name folder generated in TEMP as 'MSBuild' instead of 'Tempor…
benvillalobos Mar 10, 2021
7b58784
Add metadata that allows users to set special characters to escape.
benvillalobos Mar 29, 2021
803cadf
Add test cases
benvillalobos Mar 29, 2021
aff3425
Add non-windows OS failing traits for end to end test
benvillalobos Mar 29, 2021
6d07c7d
PR Feedback: Improve performance by using an array instead of a hashs…
benvillalobos Apr 2, 2021
e2d8c0c
Fix parens for checking whether or not to escape the '&'
benvillalobos Apr 2, 2021
828dedc
Move ampersand and space to the end of the array. User _shouldnt_ hav…
benvillalobos Apr 2, 2021
11c0021
Simplify equals check
benvillalobos Apr 5, 2021
73e8c5c
Ampersand and space cases are already handled.
benvillalobos Apr 5, 2021
9cca2db
Add escaping under changewave 16.10
benvillalobos Apr 5, 2021
e05b2e1
Add to changewave documentation
benvillalobos Apr 5, 2021
ef369fb
PR Feedback: make characters to escape static. Remove unnecessary test
benvillalobos Apr 6, 2021
ee79111
Avoid stringbuilder allocation until necessary
benvillalobos Apr 6, 2021
674dccd
Merge branch 'main' into fix-tooltask-escape-parentheses
benvillalobos Apr 6, 2021
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
5 changes: 3 additions & 2 deletions src/Shared/TempFileUtilities.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@ internal static partial class FileUtilities
/// Caller must delete when finished.
/// </summary>
/// <param name="createDirectory"></param>
internal static string GetTemporaryDirectory(bool createDirectory = true)
/// <param name="subfolder"></param>
internal static string GetTemporaryDirectory(bool createDirectory = true, string subfolder = null)
{
string temporaryDirectory = Path.Combine(Path.GetTempPath(), "Temporary" + Guid.NewGuid().ToString("N"));
string temporaryDirectory = Path.Combine(Path.GetTempPath(), "MSBuild" + Guid.NewGuid().ToString("N"), subfolder ?? string.Empty);
Copy link
Member

@KirillOsenkov KirillOsenkov Mar 8, 2021

Choose a reason for hiding this comment

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

Should we do "MSBuild", Guid... so that MSBuild is its own directory, and Guids are subdirectories within it?

Copy link
Member

Choose a reason for hiding this comment

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

Also modify any similar methods (if they exist)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently changing the name to MSBuild here works, but there are places where this method could be called instead of Path.GetTempDirectory (even within FileUtilities). Unfortunately this means changing a bunch of tests and I'd rather not piggy-back that onto this PR.

I say we take this change and I can make a new PR that refactors TempFileUtilities.cs.

If not, I can revert the change to using the MSBuild folder and we can merge this PR as is.

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just realized what you were asking for wasn't many MSBuild<guid> folders, but temp/MSBuild/<all the guids folders>. Given that this commit didn't actually help much, I'm going to revert it and actually fix it in a different PR that's based off of this one.

Copy link
Member

Choose a reason for hiding this comment

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

sounds great

Copy link
Member Author

Choose a reason for hiding this comment

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

tracking this here: #6239


if (createDirectory)
{
Expand Down
17 changes: 13 additions & 4 deletions src/Shared/UnitTests/TestEnvironment.cs
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,15 @@ public TransientTempPath CreateNewTempPath()
return SetTempPath(folder.Path, true);
}

/// <summary>
/// Creates a new temp path with a custom subfolder
/// </summary>
public TransientTempPath CreateNewTempPathWithSubfolder(string subfolder)
{
var folder = CreateFolder(null, true, subfolder);
return SetTempPath(folder.Path, true);
}

/// <summary>
/// Creates a new temp path
/// Sets all OS temp environment variables to the new path
Expand Down Expand Up @@ -266,9 +275,9 @@ public TransientTestFile ExpectFile(TransientTestFolder transientTestFolder, str
/// Creates a test variant used to add a unique temporary folder during a test. Will be deleted when the test
/// completes.
/// </summary>
public TransientTestFolder CreateFolder(string folderPath = null, bool createFolder = true)
public TransientTestFolder CreateFolder(string folderPath = null, bool createFolder = true, string subfolder = null)
{
var folder = WithTransientTestState(new TransientTestFolder(folderPath, createFolder));
var folder = WithTransientTestState(new TransientTestFolder(folderPath, createFolder, subfolder));

Assert.True(!(createFolder ^ FileSystems.Default.DirectoryExists(folder.Path)));

Expand Down Expand Up @@ -605,9 +614,9 @@ public void Delete()

public class TransientTestFolder : TransientTestState
{
public TransientTestFolder(string folderPath = null, bool createFolder = true)
public TransientTestFolder(string folderPath = null, bool createFolder = true, string subfolder = null)
{
Path = folderPath ?? FileUtilities.GetTemporaryDirectory(createFolder);
Path = folderPath ?? FileUtilities.GetTemporaryDirectory(createFolder, subfolder);

if (createFolder)
{
Expand Down
19 changes: 19 additions & 0 deletions src/Tasks.UnitTests/Exec_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,25 @@ private ExecWrapper PrepareExecWrapper(string command)
return exec;
}

[Fact]
public void EscapeParenthesesInPathToGeneratedBatchFile()
{
using (var testEnvironment = TestEnvironment.Create())
{
// This test counts files in TEMP. If it uses the system TEMP, some
// other process may interfere. Use a private TEMP instead.
var newTempPath = testEnvironment.CreateNewTempPathWithSubfolder("hello()wo(rld)").TempPath;

string tempPath = Path.GetTempPath();
Assert.StartsWith(newTempPath, tempPath);

// Now run the Exec task on a simple command.
Exec exec = PrepareExec("echo Hello World!");
exec.Execute().ShouldBeTrue();

Copy link
Member

Choose a reason for hiding this comment

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

nit: no newline

}
}

/// <summary>
/// Ensures that calling the Exec task does not leave any extra TEMP files
/// lying around.
Expand Down
14 changes: 12 additions & 2 deletions src/Tasks/Exec.cs
Original file line number Diff line number Diff line change
Expand Up @@ -611,12 +611,22 @@ protected internal override void AddCommandLineCommands(CommandLineBuilderExtens

// If for some crazy reason the path has a & character and a space in it
// then get the short path of the temp path, which should not have spaces in it
// and then escape the &
if (batchFileForCommandLine.Contains("&") && !batchFileForCommandLine.Contains("^&"))
{
batchFileForCommandLine = NativeMethodsShared.GetShortFilePath(batchFileForCommandLine);
batchFileForCommandLine = batchFileForCommandLine.Replace("&", "^&");
}

StringBuilder fileName = StringBuilderCache.Acquire(batchFileForCommandLine.Length).Append(batchFileForCommandLine);

// Escape any '(', ')', or '&'
for(int i = 1; i < fileName.Length; i++)
{
if((fileName[i] == '(' || fileName[i] == ')' || fileName[i] == '&') && fileName[i-1] != '^')
{
fileName.Insert(i++, '^');
Copy link
Member

Choose a reason for hiding this comment

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

Why not append each character individually so you don't have to insert into the middle?

Also, why not start at 0?

Copy link
Member

Choose a reason for hiding this comment

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

also cache fileName[i] in a local char variable to save on access
add spaces around [i - 1] or format this area using Format Selection or Format Document

Copy link
Member Author

@benvillalobos benvillalobos Mar 8, 2021

Choose a reason for hiding this comment

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

Not starting at 0 here because we're on windows and batchFileForComandLine points to a .cmd file in the temp folder. It really should start at 3 to skip over c:/.

}
}
batchFileForCommandLine = StringBuilderCache.GetStringAndRelease(fileName);
}

commandLine.AppendFileNameIfNotNull(batchFileForCommandLine);
Expand Down