Runtime: MSBuild Exec task broken with .NET Core 2.1

[dsplaisted]

I'm running into an issue updating the .NET CLI to use .NET Core 2.1. My best guess currently is that this is a regression in .NET Core, because it is surfacing when I update to .NET Core 2.1 but worked with .NET Core 2.0.

Repro steps

• On a non-Windows OS
• With a version of the .NET CLI that uses .NET Core 2.1
• Run dotnet msbuild with the following project

<Project>
    <Target Name="Build">
        <Exec Command="echo Hello World" />
    </Target>
</Project>

Expected

Build succeeds

Actual

Build fails with the following:

Microsoft (R) Build Engine version 15.3.409.57025 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

/bin/sh: 1: export LANG=en_US.UTF-8; export LC_ALL=en_US.UTF-8; . /tmp/tmp365c3d61708c464b9358f5adcdafd024.exec.cmd: not found
/mnt/c/git/dotnet-cli-linux/artifacts/testexec/testexec.proj(6,9): error MSB3073: The command "echo Hello World" exited with code 127.

Details

The MSBuild Exec task creates a script file in the temporary folder in order to set encodings for the launched process. After the command is executed, the temporary file is deleted.

It appears that with .NET Core 2.1, the temporary script file is not visible to the launched process, resulting in a "not found" error.

Building a CLI with .NET Core 2.1

Currently there isn't a version of the CLI available with .NET Core 2.1. To build one yourself in order to repro the issue:

• Clone https://github.com/dsplaisted/cli
• Check out the update-runtime-to-2.1 branch
• Set the CLIBUILD_SKIP_TESTS environment variable to true
• Run build.sh from the repo root
• To use the newly built CLI, run dotnet from the artifacts/linux-x64/stage2 folder

[nguerrera]

@eerhardt @weshaggard @stephentoub @jeremykuhne

[eerhardt]

Two initial questions:

1. Can the issue be repro'd outside of MSBUILD in a simple exe?
2. Has it ever worked on 2.1? For example, if you grab a runtime build from say 2 months ago.

[dsplaisted]

@eerhardt

1. I don't have a non-MSBuild repro of the issue. I did look through the MSBuild code to see if I could do this but it didn't look trivial to isolate a repro.
2. If you give me an older build number of the runtime to try I can see if the issue repros. Currently I'm using version 2.1.0-preview2-25615-02

[eerhardt]

@dsplaisted - the list of runtime versions that I use is always on myget: https://dotnet.myget.org/feed/dotnet-core/package/nuget/Microsoft.NETCore.App

I've repro'd this break with 2.1.0-preview2-25503-01 from Mon, 03 Jul 2017 16:55:43 GMT.

I then used 2.1.0-preview1-25331-02 from Wed, 31 May 2017 20:56:05 GMT and the bug no longer repos.

So the change must have happened sometime in June. Now comes the fun challenge of narrowing it down even further.

[eerhardt]

Doesn't repro on 2.1.0-preview1-25415-01 from Thu, 15 Jun 2017 17:17:52 GMT.

[nguerrera]

I feel like you're live tweeting a git bisect. This is awesome, thanks!

[eerhardt]

Doesn't repro on 2.1.0-preview1-25505-03 from Wed, 05 Jul 2017 22:05:14 GMT. CoreFX sha e22c14a4ece7efea709105d9328a77c36d538454

DOES repro on 2.1.0-preview1-25521-02 from Fri, 21 Jul 2017 01:38:23 GMT. CoreFX sha 925417b1a65fbee91cf3a1c4b84b06045c9eb0e8

Unfortunately that is the closest I can get in the preview1 train. There were no builds between those two dates, and ~900 commits in between.

The earliest preview2 build repros the issue. So I'll have to see how far apart that build is from a preview1 build.

[eerhardt]

I was able to get a standalone repro of the issue:

https://github.com/eerhardt/ReproBug23496

Run that console app on 2.0.0, and it prints out "Hello World".

Run the console app on a recent 2.1 build, and it prints "127", the unexpected exit code.

I was also able to narrow a bit more down that the change was between https://github.com/dotnet/core-setup/compare/6edd8c7a7e96fcef57e5b7c8bafa3d01df4ec942...cbbc6155c7a88daeb18d237ad72c8f394bf4f845. And the only real difference was updating the CoreFX build from 4.5.0-preview1-25413-02 to 4.5.0-preview1-25429-02. Note that the CoreCLR build didn't change at all, from my understanding.

This looks to be either an I/O or a Process bug. Adding those two areas. I think either the file isn't available in time to invoke it, or the Process isn't being started correctly.

/cc @ianhays

[ianhays]

cc: @Priya91 in case she's aware of any changes in that timespan that could cause the error e.g. maybe https://github.com/dotnet/corefx/pull/21339

[danmosemsft]

Did you figure out the range of corefx commits corresponding to 4.5.0-preview1-25413-02 to 4.5.0-preview1-25429-02 ?

[eerhardt]

I think it is on the order of ~400. The builds were 16 days apart.

[eerhardt]

@ianhays @Priya91 - I verified this is an escaping issue between MSBuild and dotnet/corefx#21339, like you said @ianhays.

If you look at https://github.com/Microsoft/msbuild/blob/a9f64ebd108702c3fc65339c66cb124217854524/src/Tasks/Exec.cs#L609-L612, you'll see MSBuild is appending triple quotes around the option

c# commandLine.AppendSwitch("-c"); commandLine.AppendTextUnquoted(" \"\"\""); commandLine.AppendTextUnquoted("export LANG=en_US.UTF-8; export LC_ALL=en_US.UTF-8; . "); commandLine.AppendFileNameIfNotNull(batchFileForCommandLine); commandLine.AppendTextUnquoted("\"\"\"");

If I remove one of those quotes, the issue no longer repros.

@rainersigwald @jeffkl - in case they know exactly why the Exec task is using triple quotes.

Either way, I think we've narrowed down the bug to the exact problem - using triple quotes passing into Process.StartInfo.Arguments.

[rainersigwald]

The commit that added the triple quoting (https://github.com/Microsoft/msbuild/commit/ec997c5c7e555d0105e76b3102c3657ed227334c) just says "Fix Exec" which is unfortunately vague. Based on this

$ sh -c "echo foo"
foo
$ sh -c ""echo foo""

$ sh -c """echo foo"""
foo
$ sh -c """echo foo"
foo

and looking around for guidance on triple quoting (and finding none) I think MSBuild should move to single-" quoting. But it's surprising to be broken.

[Priya91]

cc @joperezr @wtgodbe

[wtgodbe]

@ValMenn I see https://github.com/Microsoft/msbuild/commit/ec997c5c7e555d0105e76b3102c3657ed227334c was your PR, can you comment on why the triple-quotes were added in that PR? would the right fix here be to use triple-quotes in a similar place in CoreFX?

[wtgodbe]

I have been able to successfully run the example project with a 2.0 & 2.1 CLI, so I am closing this as resolved. @dsplaisted feel free to re-open if I've misinterpreted something.

[eerhardt]

This is a breaking change in our Process APIs - triple quotes no longer work and they did before. The reason the initial repro no longer repros the issue is because MSBuild changed Exec to work around the problem. See my repo listed above for the repro of this issue with triple quotes.

https://github.com/eerhardt/ReproBug23496

[wtgodbe]

@eerhardt so was https://github.com/dotnet/corefx/pull/21339/files the breaking change?

[eerhardt]

so was https://github.com/dotnet/corefx/pull/21339/files the breaking change?

Yes

[wtgodbe]

@Priya91 can you comment on the reason for https://github.com/dotnet/corefx/pull/21339/files#diff-015aad2514de8320cbfe19ed2b8ee9a7R511 ? I believe this block is what is causing the issue

[Priya91]

The linked PR has the reason for making that change, the previous arg parsing code had a bug with not parsing the empty arguments correctly, and this logic was also unified with the argument parsing logic across .net stacks, the uwp corert also has the same logic.

[stephentoub]

This is a breaking change in our Process APIs

@eerhardt, is this a break from netfx? From Mono? From .NET Core 2.0?

[eerhardt]

It for sure was a break from .NET Core 2.0, since the same MSBuild code ran just fine on netcoreapp2.0. I didn't test it on other .net stacks.

[stephentoub]

It for sure was a break from .NET Core 2.0, since the same MSBuild code ran just fine on netcoreapp2.0. I didn't test it on other .net stacks.

I ask because this goes to whether this is fixing a bug that shipped in .NET Core 2.0, bringing it more in line with the other implementations, or whether this is actually causing divergence from other implementations.

[eerhardt]

The scenario (since it isn't obvious from the repro code) is triple-quoting arguments in ProcessStartInfo:

https://github.com/Microsoft/msbuild/commit/ec997c5c7e555d0105e76b3102c3657ed227334c#diff-9995af5b148b699fc79ad7e2cc972ea2R560

This string eventually gets passed into commandLine in new ProcessStartInfo(pathToTool, commandLine) where pathToTool is /bin/sh.

Why the triple-quotes are used - I have no idea. I just know that this code was working, and now it isn't.

[wtgodbe]

Looking at https://github.com/dotnet/corefx/pull/21339/files#diff-015aad2514de8320cbfe19ed2b8ee9a7R511, it looks to me like an arg that looks like """argument""" will be converted to ""argument"" (triple quotes become double quotes). This would explain a break for an argument with a triple quote, as """argument""" would wind up being treated as 2 args instead of 1. I don't see the same logic in https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/src/System/Runtime/CommandLine.Windows.cs#L48, though I may be missing it. @Priya91 do you agree? If so I can try to come up with logic that covers both scenarios (Empty args & triple-quoted args)

[Priya91]

Hmm both are doing the same, https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/src/System/Runtime/CommandLine.Windows.cs#L162

The corert one also treats a quote inside quotes and emits a quote character.

In fact the code says """argument""" will become "argument". As in after the first ", inQuotes = true, then the current char is the middle quote, and the if checks that the inQuote is true, and the next char is also a quote, so add to quote to the current argument, so """ pattern gets converted to "

[eerhardt]

Full disclosure: I'm OK with closing this issue as "We've analyzed the behavior change and believe this is the correct behavior going forward. We believe the breaking change is justified." if that's what we've decided. I haven't done that analysis myself, so I'll leave it up to @Priya91 and @wtgodbe.

The reason I re-opened the issue is because it was being closed as "I can't repro the problem".

[wtgodbe]

Ah, I see, I missed that we don't emit the first ". It appears then that before your change, we didn't emit any " characters into the output arg, so we have different behavior now for 3+ quotes to open/close strings ("""argument""" became just argument before, but is now "argument"). Is this an expected consequence of that change? I think things would start getting strange when we get to higher numbers of opening/closing quotes as well - with an even number of opening/closing quotes, we'd emit n-2 quotes, treat the section between the quotes as not being inQuote, then emit n-2 quotes again. For an odd number of quotes it'd be the same, but the in-between section would be treated as being inQuote. Is the expected behavior for these situations defined anywhere?

[Priya91]

@wtgodbe Did you check what is the output of the argument parsing in this case on desktop?

[Priya91]

Is the expected behavior for these situations defined anywhere?

This comment in code states where the rules were taken from, but I don't think there is a defined standard somewhere.

c# // Splits a command line into argc/argv lists, using the VC7 parsing rules. Adapted from CLR's // SegmentCommandLine implementation. // // This functions interface mimics the CommandLineToArgvW api. //

[4creators]

This comment in code states where the rules were taken from, but I don't think there is a defined standard somewhere.

Is it possible to have it in docs? I have hit similar issues already in my code.

[ghost]

Are there minimum steps to repro for this (like a five liner Main method)? https://github.com/eerhardt/ReproBug23496/blob/master/ConsoleApp1 seems too huge to be a self-contained steps.

Totally agree with Stephen's comment above; is the new behavior aligned with .NET 4.6x, 4.7x and Mono 5x? If not perhaps makes sense to revert it to match those? unless there is a very good reason to break compatibility, in which case .NET / Mono should also be changed to the new behavior. Lets vote and majority of platforms implementations be the deciding factor..

[jkotas]

I don't think there is a defined standard somewhere.

The escaping rules are documented here: https://docs.microsoft.com/en-us/cpp/cpp/parsing-cpp-command-line-arguments

[wtgodbe]

Seconded what @kasper3 said, I'm having trouble reproing with @eerhardt's example

[wtgodbe]

It appears though that a desktop app that is passed a command line arg of """arg""" will emit it as "arg", which is consistent with the new state of the code in question. Therefore I'm inclined to think the code is working as intended. Or to quote @eerhardt,

We've analyzed the behavior change and believe this is the correct behavior going forward. We believe the breaking change is justified

[eerhardt]

Seconded what @kasper3 said, I'm having trouble reproing with @eerhardt's example

I've updated the repro project to target netcoreapp2.1 and added instructions to the README file in the repo.

https://github.com/eerhardt/ReproBug23496/blob/master/README.md

The key here is:

On .NET Core 2.0, the program outputs:

Hello World
0

On .NET Core 2.1, the program outputs:

/bin/sh: 1: export LANG=en_US.UTF-8; export LC_ALL=en_US.UTF-8; . /tmp/tmpdfe53bca1c1b44509f2e7608b8d0dc16.exec.cmd: not found
127

[Priya91]

@eerhardt This break is expected because we changed implementation to match other .net stacks. Does the program work with .net framework? If the framework has same behavior as .netcore 2.0, then it's a bug we should investigate further.

[eerhardt]

The program is specific to Unix (since it uses /bin/sh).

I compiled the program and ran the program on Mono JIT compiler version 5.4.1.6 (2017-06/1f4613aa1ac Wed Oct 18 09:31:57 EDT 2017) and it outputted:

Hello World
0

[eerhardt]

So to answer @stephentoub's question above:

is this a break from netfx? From Mono? From .NET Core 2.0?

I've verified it is a break from Mono and from .NET Core 2.0.

[wtgodbe]

So it sounds like this is something we need to fix.

@Priya91 in https://github.com/dotnet/corefx/pull/21339/files#diff-015aad2514de8320cbfe19ed2b8ee9a7R511, were you trying to address only the case when the user passes an argument that looks exactly like "" ? It does look like
https://github.com/dotnet/corefx/blob/release/1.1.0/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs#L489-L496 would discard such an arg entirely. If so, we should be able to come up with logic that addresses only that case, and doesn't change behavior in the "3 or more consecutive quotes" case.

[Priya91]

I dont exactly remember why I started doing that work, going by the PR looks like I started to fix the case of empty arguments, and then it turned out to unify the logic with corert. If you said that this is the same behavior with .net framework as well, then by reverting this change, it is going to introduce compat issues with framework. I recommend you work out a matrix with the behavior on all available platforms, and fix on the right behavior, before starting to address a one-off test case.

[wtgodbe]

I believe Eric was saying that the behavior before this change was the same as mono/.net framework, but is different in 2.1 (I.E. this bug exists in 2.1, but not in .net framework/mono). Am I interpreting that wrong @eerhardt? Either way I agree a behavior matrix is a good idea.

[eerhardt]

I don't have data on the .net framework behavior. I've only run the program on Mono 5.4.1, .NET Core 2.0, and .NET Core 2.1. I also haven't looked into paring down the repro program to the core "this exact ProcessStartInfo args are breaking". That program was created by isolating the code in MSBuild that was causing the issue. I got it isolated from MSBuild in a simple console app, and then stopped reducing it.

[wtgodbe]

Is there a way I can run your repro on Windows? I just want to get a baseline for behavior on .net framework - do you think it'd be sufficient to write a simple console app that that prints out command line args & pass it args w/ different combinations of " characters?

[eerhardt]

The exact program can't run on Windows because it is using /bin/sh. Roughly this is what is happening:

```C#
Process p = new Process();
p.StartInfo.FileName = "/bin/sh";
p.StartInfo.Arguments = " -c """export LANG=en_US.UTF-8; export LC_ALL=en_US.UTF-8; . /tmp/tmp25048b5e928a4b948423df765b641fa0.exec.cmd"""";
p.Start();
p.WaitForExit();

Where `/tmp/tmp25048b5e928a4b948423df765b641fa0.exec.cmd` is a temporary generated file that contains 

!/bin/sh

echo Hello World
```

[wtgodbe]

I ran the following program against .net framework:

```C#
class Program
{
static void Main(string[] args)
{
Process p = new Process();
p.StartInfo.FileName = "C:\Users\wigodbe\Documents\Visual Studio 2015\Projects\ProcessStart\ProcessStart\tmp.cmd";
p.StartInfo.Arguments = """"triple quoted argument"""";
p.Start();
p.WaitForExit();
}
}
}

Where tmp.cmd contains:

echo %1
pause
```

And got the following output:

"""triple quoted argument"""

So it seems to me this behavior did not exist in .net framework, and therefore should be fixed for .net core 2.1. I can try to put together a change that fixes this behavior without breaking empty argument parsing, if desired. @eerhardt does this all seem reasonable?

[wtgodbe]

I have run the program above against .net framework, .net core 1.0, .net core 2.0, and .net core 2.1, and all output the same: """triple quoted argument""". I also have confirmed that the tests in https://github.com/dotnet/corefx/pull/25866/files#diff-a40257c563e727625287f3a6eea5e706L1126 all pass on Unix & windows against current CoreFX bits, as well as on .net framework. At this point I'm a bit stumped as to what the actual behavior difference is that is causing the repro program to fail only on Linux .net core 2.1. @eerhardt @Priya91 any ideas?

[wtgodbe]

Also worth noting that the test at https://github.com/dotnet/corefx/pull/25866/files#diff-a40257c563e727625287f3a6eea5e706R1144 outputs "a" c b, meaning that the triple quoted arg is being emitted surrounded by just 1 set of quotes. Not sure why my repro program consistently produces triple-quote output.

[Priya91]

I have run the program above against .net framework, .net core 1.0, .net core 2.0, and .net core 2.1, and all output the same: """triple quoted argument"""

I'm confused here, if all platforms report the same output, what is the bug here?

[eerhardt]

This is part of the reason I stopped at the console app in my repo above. That project reproduces the break between .net Core 2.0 and 2.1. It is a stand-alone console app isolated from the MSBuild source code. It seems someone who is more familiar with the Process code should be able to take that project and investigate/debug the break.

[wtgodbe]

With the following program (and a similar implementation on Linux):

c# class Program { static void Main(string[] args) { Process p = new Process(); p.StartInfo.FileName = "C:\\Users\\wigodbe\\Documents\\Visual Studio 2015\\Projects\\ProcessStart\\ProcessStart\\tmp.cmd"; p.StartInfo.Arguments = "\"\"\"triple quoted argument\"\"\""; p.Start(); p.WaitForExit(); } } }

I get the following behavior matrix:

| |.net framework |.net core 2.0 |.net core 2.1 |
|:------:|:------:|:------:|:------:|
|Windows |"""triple quoted argument""" |"""triple quoted argument""" |"""triple quoted argument""" |
|Linux | N/A | triple quoted argument |"triple quoted argument" |

I suspect the single vs. triple quote difference between Windows & Linux on .net Core 2.0 is a difference in behavior between how /bin/sh & cmd.exe process/emit arguments, but the difference in behavior from 2.0 & 2.1 is what is causing the bug. I believe the 2.1 behavior is the correct behavior, and that something in the mid-layer APIs is causing the failure. Still working on debugging on linux to figure out what's going on.

[wtgodbe]

This appears to be a problem with Linux shell, not with the way we parse args. To illustrate, consider the following program:

#include <unistd.h>

int main(int argc, char** argv[]) {
    char * const environment[] = {NULL};
    char * const args[] = { "-c", "\"echo.sh\"};
    execvpe("/bin/sh", args, environment);
}

This will fail with:

Can't open "echo.sh"

This is analogous to the scenario in the repro, where we parse the triple-quoted stuff into an arg surrounded by single-quotes, then pass that directly to /bin/sh. If you run the same thing from the command line, the command line shell will parse the " out of the argument & pass it to /bin/sh in a format it can read (that is, without any quote characters). When /bin/sh receives a string surrounded by quotes, it treats them literally, and looks for an executable named "echo.sh" instead of echo.sh. As long as our argument parsing behavior is consistent with windows (which it is), then I'd consider this an issue with /bin/sh, and not with our Process APIs. Therefore I'm closing this as a non-issue.

[jkotas]

@wtgodbe I disagree with your conclusion that this is a problem with Unix /bin/sh.

On Unix OS, you provide array of arguments to the OS when starting a process. There is no quoting or escaping involved. You will get exactly what you asked for. There is nothing that can go wrong there.

On Windows OS, you provide the escaped command line to the OS when starting a process. So the array of arguments has to be converted to a single string; the other side has to convert it back to the array and hope that you will end up with the same result. This round-trip is where all the problems are. Since the .NET Process APIs have been Windows-centric we are trying to emulate the round-tripping on Unix too, and thus has all the problems with round-tripping on Unix too - it will always be .NET specific issue.

[karelz]

@jkotas what would you recommend then?

The problem is that .NET offers API which takes 1 string. We have to break it out on Unix into array of args. We have 2 choices -- use Windows rules (current implementation), or emulate Linux shell rules (assuming there is a single spec and shells do not deviate).

Swapping Windows rules to Linux rules seems to be larger breaking change at this point. Not something I would choose.

[karelz]

From offline discussions with @wtgodbe and @jkotas - we are not trying to say that /bin/sh has a bug. Rather that whoever passes triple quotes on Unix (msbuild in this case) has to be aware that single-quoted string will be passed over as argument (applying Windows rules) and it is up to the caller (msbuild in this case) to make sure the callee (/bin/sh in this case) can handle it.
For /bin/sh as a callee it is invalid syntax to pass -c string with string in quotes - it is interpreted as commands and quotes are invalid there (see Invocation section).
For another app (not shell) it could be valid.

The "problematic" angle is that due to our bug we didn't handle multiple quotes correctly as we should have per Windows spec. We fixed it during 2.1 development cycle in dotnet/corefx#21339, which uncoreved this bug in msbuild (it depended on our incorrect behavior of stripping ALL triple quotes, instead of leaving single quotes in place).
I believe that the fix dotnet/corefx#21339 is a good one, even though it is technical breaking change.
Closing this issue as By Design / Won't Fix -- current behavior is the right one (given our past decisions to use Windows command line parsing rules also on Linux).