diff --git a/releaseNote.md b/releaseNote.md index a6cb9c027f8..b70766e7098 100644 --- a/releaseNote.md +++ b/releaseNote.md @@ -1,5 +1,5 @@ ## Bugs -- Fixed an issue where job and service container envs were corrupted (#2091) +- Fixed an issue where self hosted environments had their docker env's overwritten (#2107) ## Misc ## Windows x64 diff --git a/src/Runner.Worker/Container/DockerCommandManager.cs b/src/Runner.Worker/Container/DockerCommandManager.cs index 4d4940c9752..a0c158bdf68 100644 --- a/src/Runner.Worker/Container/DockerCommandManager.cs +++ b/src/Runner.Worker/Container/DockerCommandManager.cs @@ -107,7 +107,6 @@ public async Task DockerBuild(IExecutionContext context, string workingDire public async Task DockerCreate(IExecutionContext context, ContainerInfo container) { IList dockerOptions = new List(); - IDictionary environment = new Dictionary(); // OPTIONS dockerOptions.Add($"--name {container.ContainerDisplayName}"); dockerOptions.Add($"--label {DockerInstanceLabel}"); @@ -136,8 +135,7 @@ public async Task DockerCreate(IExecutionContext context, ContainerInfo } else { - environment.Add(env.Key, env.Value); - dockerOptions.Add(DockerUtil.CreateEscapedOption("-e", env.Key)); + dockerOptions.Add(DockerUtil.CreateEscapedOption("-e", env.Key, env.Value)); } } @@ -185,7 +183,7 @@ public async Task DockerCreate(IExecutionContext context, ContainerInfo dockerOptions.Add($"{container.ContainerEntryPointArgs}"); var optionsString = string.Join(" ", dockerOptions); - List outputStrings = await ExecuteDockerCommandAsync(context, "create", optionsString, environment); + List outputStrings = await ExecuteDockerCommandAsync(context, "create", optionsString); return outputStrings.FirstOrDefault(); } @@ -445,11 +443,6 @@ public Task DockerLogin(IExecutionContext context, string configFileDirecto } private async Task> ExecuteDockerCommandAsync(IExecutionContext context, string command, string options) - { - return await ExecuteDockerCommandAsync(context, command, options, null); - } - - private async Task> ExecuteDockerCommandAsync(IExecutionContext context, string command, string options, IDictionary environment) { string arg = $"{command} {options}".Trim(); context.Command($"{DockerPath} {arg}"); @@ -477,7 +470,7 @@ await processInvoker.ExecuteAsync( workingDirectory: context.GetGitHubContext("workspace"), fileName: DockerPath, arguments: arg, - environment: environment, + environment: null, requireExitCodeZero: true, outputEncoding: null, cancellationToken: CancellationToken.None); diff --git a/src/Runner.Worker/Container/DockerUtil.cs b/src/Runner.Worker/Container/DockerUtil.cs index e1ae6d3eeb4..97a53759bc7 100644 --- a/src/Runner.Worker/Container/DockerUtil.cs +++ b/src/Runner.Worker/Container/DockerUtil.cs @@ -6,6 +6,9 @@ namespace GitHub.Runner.Worker.Container { public class DockerUtil { + private static readonly Regex QuoteEscape = new Regex(@"(\\*)" + "\"", RegexOptions.Compiled); + private static readonly Regex EndOfStringEscape = new Regex(@"(\\+)$", RegexOptions.Compiled); + public static List ParseDockerPort(IList portMappingLines) { const string targetPort = "targetPort"; @@ -68,12 +71,37 @@ public static string CreateEscapedOption(string flag, string key) { return ""; } - return $"{flag} \"{EscapeString(key)}\""; + return $"{flag} {EscapeString(key)}"; + } + + public static string CreateEscapedOption(string flag, string key, string value) + { + if (String.IsNullOrEmpty(key)) + { + return ""; + } + var escapedString = EscapeString($"{key}={value}"); + return $"{flag} {escapedString}"; } private static string EscapeString(string value) { - return value.Replace("\\", "\\\\").Replace("\"", "\\\""); + if (String.IsNullOrEmpty(value)) + { + return ""; + } + // Dotnet escaping rules are weird here, we can only escape \ if it precedes a " + // If a double quotation mark follows two or an even number of backslashes, each proceeding backslash pair is replaced with one backslash and the double quotation mark is removed. + // If a double quotation mark follows an odd number of backslashes, including just one, each preceding pair is replaced with one backslash and the remaining backslash is removed; however, in this case the double quotation mark is not removed. + // https://docs.microsoft.com/en-us/dotnet/api/system.environment.getcommandlineargs?redirectedfrom=MSDN&view=net-6.0#remarks + + // First, find any \ followed by a " and double the number of \ + 1. + value = QuoteEscape.Replace(value, @"$1$1\" + "\""); + // Next, what if it ends in `\`, it would escape the end quote. So, we need to detect that at the end of the string and perform the same escape + // Luckily, we can just use the $ character with detects the end of string in regex + value = EndOfStringEscape.Replace(value, @"$1$1"); + // Finally, wrap it in quotes + return $"\"{value}\""; } } } diff --git a/src/Test/L0/Container/DockerUtilL0.cs b/src/Test/L0/Container/DockerUtilL0.cs index 843aaf5efd1..1e45d598e89 100644 --- a/src/Test/L0/Container/DockerUtilL0.cs +++ b/src/Test/L0/Container/DockerUtilL0.cs @@ -149,13 +149,12 @@ public void ParseRegistryHostnameFromImageName(string input, string expected) [Trait("Level", "L0")] [Trait("Category", "Worker")] [InlineData("", "")] - [InlineData("HOME alpine:3.8 sh -c id #", "HOME alpine:3.8 sh -c id #")] - [InlineData("HOME \"alpine:3.8 sh -c id #", "HOME \\\"alpine:3.8 sh -c id #")] - [InlineData("HOME \\\"alpine:3.8 sh -c id #", "HOME \\\\\\\"alpine:3.8 sh -c id #")] - [InlineData("HOME \\\\\"alpine:3.8 sh -c id #", "HOME \\\\\\\\\\\"alpine:3.8 sh -c id #")] - [InlineData("HOME \"\"alpine:3.8 sh -c id #", "HOME \\\"\\\"alpine:3.8 sh -c id #")] - [InlineData("HOME \\\"\"alpine:3.8 sh -c id #", "HOME \\\\\\\"\\\"alpine:3.8 sh -c id #")] - [InlineData("HOME \"\\\"alpine:3.8 sh -c id #", "HOME \\\"\\\\\\\"alpine:3.8 sh -c id #")] + [InlineData("foo", "foo")] + [InlineData("foo \\ bar", "foo \\ bar")] + [InlineData("foo \\", "foo \\\\")] + [InlineData("foo \\\\", "foo \\\\\\\\")] + [InlineData("foo \\\" bar", "foo \\\\\\\" bar")] + [InlineData("foo \\\\\" bar", "foo \\\\\\\\\\\" bar")] public void CreateEscapedOption_keyOnly(string input, string escaped) { var flag = "--example"; @@ -171,5 +170,28 @@ public void CreateEscapedOption_keyOnly(string input, string escaped) } Assert.Equal(expected, actual); } + + [Theory] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + [InlineData("foo", "bar", "foo=bar")] + [InlineData("foo\\", "bar", "foo\\=bar")] + [InlineData("foo\\", "bar\\", "foo\\=bar\\\\")] + [InlineData("foo \\","bar \\", "foo \\=bar \\\\")] + public void CreateEscapedOption_keyValue(string keyInput, string valueInput, string escapedString) + { + var flag = "--example"; + var actual = DockerUtil.CreateEscapedOption(flag, keyInput, valueInput); + string expected; + if (String.IsNullOrEmpty(keyInput)) + { + expected = ""; + } + else + { + expected = $"{flag} \"{escapedString}\""; + } + Assert.Equal(expected, actual); + } } } diff --git a/src/runnerversion b/src/runnerversion index f7b71d4819b..ffb30427c2f 100644 --- a/src/runnerversion +++ b/src/runnerversion @@ -1 +1 @@ -2.296.1 +2.296.2