From ac4119f42bead4e968c047d6fcf6dd84556821e2 Mon Sep 17 00:00:00 2001 From: Thomas Boop Date: Tue, 6 Sep 2022 15:20:05 -0400 Subject: [PATCH 1/4] fix issue with env's overwriting environment --- .../Container/DockerCommandManager.cs | 13 +++------ src/Runner.Worker/Container/DockerUtil.cs | 9 +++++++ src/Test/L0/Container/DockerUtilL0.cs | 27 +++++++++++++++++++ 3 files changed, 39 insertions(+), 10 deletions(-) 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..f01f84584fc 100644 --- a/src/Runner.Worker/Container/DockerUtil.cs +++ b/src/Runner.Worker/Container/DockerUtil.cs @@ -71,6 +71,15 @@ public static string CreateEscapedOption(string flag, string key) return $"{flag} \"{EscapeString(key)}\""; } + public static string CreateEscapedOption(string flag, string key, string value) + { + if (String.IsNullOrEmpty(key)) + { + return ""; + } + return $"{flag} \"{EscapeString(key)}={value.Replace("\"", "\\\"")}\""; + } + private static string EscapeString(string value) { return value.Replace("\\", "\\\\").Replace("\"", "\\\""); diff --git a/src/Test/L0/Container/DockerUtilL0.cs b/src/Test/L0/Container/DockerUtilL0.cs index 843aaf5efd1..25b188c3240 100644 --- a/src/Test/L0/Container/DockerUtilL0.cs +++ b/src/Test/L0/Container/DockerUtilL0.cs @@ -171,5 +171,32 @@ public void CreateEscapedOption_keyOnly(string input, string escaped) } Assert.Equal(expected, actual); } + + [Theory] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + [InlineData("HOME", "", "HOME", "")] + [InlineData("HOME alpine:3.8 sh -c id #", "HOME alpine:3.8 sh -c id #", "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 #", "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 #", "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 #", "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 #", "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 #", "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 #", "HOME \\\"\\\\\\\"alpine:3.8 sh -c id #", "HOME \\\"\\\\\"alpine:3.8 sh -c id #")] + public void CreateEscapedOption_keyValue(string keyInput, string valueInput, string escapedKey, string escapedValue) + { + var flag = "--example"; + var actual = DockerUtil.CreateEscapedOption(flag, keyInput, valueInput); + string expected; + if (String.IsNullOrEmpty(keyInput)) + { + expected = ""; + } + else + { + expected = $"{flag} \"{escapedKey}={escapedValue}\""; + } + Assert.Equal(expected, actual); + } } } From 328f14862b8175d6531204931b7523ef393f7e50 Mon Sep 17 00:00:00 2001 From: Thomas Boop Date: Tue, 6 Sep 2022 21:29:37 -0400 Subject: [PATCH 2/4] add release notes --- releaseNote.md | 2 +- src/runnerversion | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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/runnerversion b/src/runnerversion index f7b71d4819b..ffb30427c2f 100644 --- a/src/runnerversion +++ b/src/runnerversion @@ -1 +1 @@ -2.296.1 +2.296.2 From 3c249b315df700900e2915a90b45a0ed9c6ad4ba Mon Sep 17 00:00:00 2001 From: Thomas Boop Date: Wed, 7 Sep 2022 14:22:57 -0400 Subject: [PATCH 3/4] handle value escaping --- src/Runner.Worker/Container/DockerUtil.cs | 22 ++++++++++++++--- src/Test/L0/Container/DockerUtilL0.cs | 29 ++++++++++------------- 2 files changed, 31 insertions(+), 20 deletions(-) diff --git a/src/Runner.Worker/Container/DockerUtil.cs b/src/Runner.Worker/Container/DockerUtil.cs index f01f84584fc..52d32eccac7 100644 --- a/src/Runner.Worker/Container/DockerUtil.cs +++ b/src/Runner.Worker/Container/DockerUtil.cs @@ -68,7 +68,7 @@ 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) @@ -77,12 +77,28 @@ public static string CreateEscapedOption(string flag, string key, string value) { return ""; } - return $"{flag} \"{EscapeString(key)}={value.Replace("\"", "\\\"")}\""; + 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 = Regex.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 = Regex.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 25b188c3240..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"; @@ -175,15 +174,11 @@ public void CreateEscapedOption_keyOnly(string input, string escaped) [Theory] [Trait("Level", "L0")] [Trait("Category", "Worker")] - [InlineData("HOME", "", "HOME", "")] - [InlineData("HOME alpine:3.8 sh -c id #", "HOME alpine:3.8 sh -c id #", "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 #", "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 #", "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 #", "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 #", "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 #", "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 #", "HOME \\\"\\\\\\\"alpine:3.8 sh -c id #", "HOME \\\"\\\\\"alpine:3.8 sh -c id #")] - public void CreateEscapedOption_keyValue(string keyInput, string valueInput, string escapedKey, string escapedValue) + [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); @@ -194,7 +189,7 @@ public void CreateEscapedOption_keyValue(string keyInput, string valueInput, str } else { - expected = $"{flag} \"{escapedKey}={escapedValue}\""; + expected = $"{flag} \"{escapedString}\""; } Assert.Equal(expected, actual); } From d4130522f36eb41a77b401f71c3f2d4c393b224b Mon Sep 17 00:00:00 2001 From: Thomas Boop Date: Thu, 8 Sep 2022 11:28:36 -0400 Subject: [PATCH 4/4] compile regex for runtime perf improvements --- src/Runner.Worker/Container/DockerUtil.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Runner.Worker/Container/DockerUtil.cs b/src/Runner.Worker/Container/DockerUtil.cs index 52d32eccac7..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"; @@ -93,10 +96,10 @@ private static string EscapeString(string value) // 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 = Regex.Replace(value, @"(\\*)" + "\"", @"$1$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 = Regex.Replace(value, @"(\\+)$", @"$1$1"); + value = EndOfStringEscape.Replace(value, @"$1$1"); // Finally, wrap it in quotes return $"\"{value}\""; }