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 12 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
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,7 @@ public sealed partial class ErrorFromResources : Microsoft.Build.Tasks.TaskExten
public partial class Exec : Microsoft.Build.Tasks.ToolTaskExtension
{
public Exec() { }
public string CharactersToEscape { set { } }
[Microsoft.Build.Framework.RequiredAttribute]
public string Command { get { throw null; } set { } }
[Microsoft.Build.Framework.OutputAttribute]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ public sealed partial class ErrorFromResources : Microsoft.Build.Tasks.TaskExten
public partial class Exec : Microsoft.Build.Tasks.ToolTaskExtension
{
public Exec() { }
public string CharactersToEscape { set { } }
[Microsoft.Build.Framework.RequiredAttribute]
public string Command { get { throw null; } set { } }
[Microsoft.Build.Framework.OutputAttribute]
Expand Down
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(), "Temporary" + Guid.NewGuid().ToString("N"), subfolder ?? string.Empty);
Copy link
Member

Choose a reason for hiding this comment

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

While we're here, this was always bugging me. Should we have a root folder "MSBuild" inside TEMP, and stick all our stuff inside that? Otherwise you can't attribute to which process a given file belongs to. Just being good citizens of TEMP. Maybe replace Temporary with MSBuild (and have a subfolder).

See #6219

Copy link
Member

Choose a reason for hiding this comment

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

Might that trip some customers into a path-too-long situation?


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
93 changes: 93 additions & 0 deletions src/Tasks.UnitTests/Exec_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,46 @@ private ExecWrapper PrepareExecWrapper(string command)
return exec;
}

[Fact]
[Trait("Category", "mono-osx-failing")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this failing on other OSes? Does it need fixing there too?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but since we don't currently have a customer asking for it, BenVillalobos wanted to hold off on fixing it until we have feedback that it's needed.

[Trait("Category", "netcore-osx-failing")]
[Trait("Category", "netcore-linux-failing")]
public void EscapeSpecifiedCharactersInPathToGeneratedBatchFile()
{
using (var testEnvironment = TestEnvironment.Create())
{
var newTempPath = testEnvironment.CreateNewTempPathWithSubfolder("hello()w]o(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.CharactersToEscape = "()]";
exec.Execute().ShouldBeTrue();
}
}

[Fact]
[Trait("Category", "mono-osx-failing")]
[Trait("Category", "netcore-osx-failing")]
[Trait("Category", "netcore-linux-failing")]
public void EscapeParenthesesInPathToGeneratedBatchFile_DuplicateCharactersToEscapeDontGetEscapedMultipleTimes()
{
using (var testEnvironment = TestEnvironment.Create())
{
var newTempPath = testEnvironment.CreateNewTempPathWithSubfolder("hello()wo(rld)").TempPath;

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

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

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

/// <summary>
/// Ensures that calling the Exec task does not leave any extra TEMP files
/// lying around.
Expand Down Expand Up @@ -918,6 +958,59 @@ public void EndToEndMultilineExec()
}
}
}

[Fact]
[Trait("Category", "mono-osx-failing")]
[Trait("Category", "netcore-osx-failing")]
[Trait("Category", "netcore-linux-failing")]
public void EndToEndMultilineExec_WithCharactersToEscapeMetadata()
{
using (var env = TestEnvironment.Create(_output))
{
var testProject = env.CreateTestProjectWithFiles(@"<Project>
<Target Name=""ExecCommand"">
<Exec CharactersToEscape=""()"" Command=""echo Hello, World!"" />
</Target>
</Project>");

// Ensure path has subfolders
var newTempPath = env.CreateNewTempPathWithSubfolder("hello()wo(rld)").TempPath;
string tempPath = Path.GetTempPath();
Assert.StartsWith(newTempPath, tempPath);

using (var buildManager = new BuildManager())
{
MockLogger logger = new MockLogger(_output, profileEvaluation: false, printEventsToStdout: false);

var parameters = new BuildParameters()
{
Loggers = new[] { logger },
};

var collection = new ProjectCollection(
new Dictionary<string, string>(),
new[] { logger },
remoteLoggers: null,
ToolsetDefinitionLocations.Default,
maxNodeCount: 1,
onlyLogCriticalEvents: false,
loadProjectsReadOnly: true);

var project = collection.LoadProject(testProject.ProjectFile).CreateProjectInstance();

var request = new BuildRequestData(
project,
targetsToBuild: new[] { "ExecCommand" },
hostServices: null);

var result = buildManager.Build(parameters, request);

logger.AssertLogContains("Hello, World!");

result.OverallResult.ShouldBe(BuildResultCode.Success);
}
}
}
}

internal class ExecWrapper : Exec
Expand Down
37 changes: 35 additions & 2 deletions src/Tasks/Exec.cs
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,19 @@ public ITaskItem[] Outputs
[Output]
public ITaskItem[] ConsoleOutput => !ConsoleToMSBuild ? Array.Empty<ITaskItem>(): _nonEmptyOutput.ToArray();

private HashSet<char> _charactersToEscape;

public string CharactersToEscape
{
set
{
if (!string.IsNullOrEmpty(value))
{
_charactersToEscape = new HashSet<char>(value);
}
}
}

#endregion

#region Methods
Expand Down Expand Up @@ -611,18 +624,38 @@ 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);

// Escape any characters specified by the CharactersToEscape metadata, or '&'
for (int i = 0; i < batchFileForCommandLine.Length; i++)
{
char c = batchFileForCommandLine[i];

if (ShouldEscapeCharacter(c) && (i == 0 || batchFileForCommandLine[i - 1] != '^'))
Copy link
Member

Choose a reason for hiding this comment

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

You said you wanted to put off fixing edge cases like not escaping a character preceded by an escaped ^, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. at the moment we've only gotten a request for the parentheses case (user's username was something(dev) which seems reasonable). So this fix will account for most other special characters, and I'll make an issue tracking that we don't account for '^''s that are already escaped and the other special cases that arise with it. If it gains traction we can worry about it then. For now this is a "better than it was before" PR.

{
fileName.Append('^');
}
fileName.Append(c);
}

batchFileForCommandLine = StringBuilderCache.GetStringAndRelease(fileName);
}

commandLine.AppendFileNameIfNotNull(batchFileForCommandLine);
}
}

private bool ShouldEscapeCharacter(char c)
{
// Escape '&' to preserve previous functionality
return c == '&' || (_charactersToEscape != null && _charactersToEscape.Contains(c));
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking about performance here...this function should be fast, but the batch file could be huge, in which case this would be called an immense number of times. It would be nice to factor out things like the null check to make this particular function call even faster.

If you want it even faster, you could make a bit array representing the characters to escape (i.e., bit x is 1 iff (int)c is in _charactersToEscape) and check that instead of this HashSet. On the other hand, I wouldn't be surprised if there's an optimization to HashSet to do just that, so it would be worth checking whether it makes a difference first.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point. In theory we could swap the implementation from: "Supply a set of the characters you want escaped" to "Escape all special characters (but if some are already escaped, or you fall into a specific edge case, you're out of luck)".

If we have an array of the characters we know need to be escaped and the user sets "TheMagicFlag", we could iterate through the array and check if ((c | specialChars[i]) == c).

Thoughts? /cc: @ladipro

Copy link
Member Author

Choose a reason for hiding this comment

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

Did some rough testing with stopwatches comparing hashset.contains with a for loop through an array of special characters that checks if ((c | specialChars[i])==c). The loop/array beats out the hashset.contains by several orders of magnitude (in terms of ticks).

Copy link
Member Author

Choose a reason for hiding this comment

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

Here I am on a weekend realizing "wait, why not just do a simple comparison?" I was incepted by the bit comment above 😅

Copy link
Member

Choose a reason for hiding this comment

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

Also, (c | specialChars[i] == c) would actually be wrong if, for example, specialChars[i] were 0, it would always return true.

}

#endregion

#region Overridden properties
Expand Down