From ddf845be083a046744b50a3bf4c0d46fe3944be6 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Wed, 6 Oct 2021 13:47:27 +0200 Subject: [PATCH 1/3] Process.Start: validate ArgumentList doesn't contain null. --- .../src/System/Diagnostics/Process.cs | 10 ++++++++++ .../System.Diagnostics.Process/tests/ProcessTests.cs | 12 ++++++++++++ 2 files changed, 22 insertions(+) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs index 0c5bf3a96542a..5f37e580340d6 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs @@ -1230,6 +1230,16 @@ public bool Start() { throw new InvalidOperationException(SR.ArgumentAndArgumentListInitialized); } + if (startInfo.HasArgumentList) + { + foreach (string arg in startInfo.ArgumentList) + { + if (arg is null) + { + throw new ArgumentNullException(nameof(startInfo.ArgumentList)); + } + } + } //Cannot start a new process and store its handle if the object has been disposed, since finalization has been suppressed. CheckDisposed(); diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs index 880da43a64653..bb3276c4332e9 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs @@ -2214,6 +2214,18 @@ public void StartProcessWithArgumentList() } } + [Fact] + public void ArgumentListArgumentNullThrowsOnStart() + { + ProcessStartInfo psi = new ProcessStartInfo(GetCurrentProcessName()); + psi.ArgumentList.Add(null); + + Process testProcess = CreateProcess(); + testProcess.StartInfo = psi; + + Assert.Throws(() => testProcess.Start()); + } + [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] [PlatformSpecific(TestPlatforms.Windows)] public void StartProcessWithSameArgumentList() From bc0e5db0565b829160d7717d3622581c4b190aad Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Wed, 6 Oct 2021 14:20:11 +0200 Subject: [PATCH 2/3] Add a custom message. --- .../System.Diagnostics.Process/src/Resources/Strings.resx | 3 +++ .../src/System/Diagnostics/Process.cs | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.Process/src/Resources/Strings.resx b/src/libraries/System.Diagnostics.Process/src/Resources/Strings.resx index 82c5f81597e97..31cd6a75a072a 100644 --- a/src/libraries/System.Diagnostics.Process/src/Resources/Strings.resx +++ b/src/libraries/System.Diagnostics.Process/src/Resources/Strings.resx @@ -326,6 +326,9 @@ Only one of Arguments or ArgumentList may be used. + + ArgumentList may not contain null. + Cannot be used to terminate a process tree containing the calling process. diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs index 5f37e580340d6..744634ad23614 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs @@ -1236,7 +1236,7 @@ public bool Start() { if (arg is null) { - throw new ArgumentNullException(nameof(startInfo.ArgumentList)); + throw new ArgumentNullException("item", SR.ArgumentListMayNotContainNull); } } } From dce500a64063c7e24c0575ba8e1f293a97e9f61e Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 26 Oct 2021 11:43:16 +0200 Subject: [PATCH 3/3] PR feedback --- .../src/System/Diagnostics/Process.cs | 5 +++-- .../System.Diagnostics.Process/tests/ProcessTests.cs | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs index 744634ad23614..fd01706bab59f 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs @@ -1232,9 +1232,10 @@ public bool Start() } if (startInfo.HasArgumentList) { - foreach (string arg in startInfo.ArgumentList) + int argumentCount = startInfo.ArgumentList.Count; + for (int i = 0; i < argumentCount; i++) { - if (arg is null) + if (startInfo.ArgumentList[i] is null) { throw new ArgumentNullException("item", SR.ArgumentListMayNotContainNull); } diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs index bb3276c4332e9..cdfa25c184c78 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs @@ -2223,7 +2223,7 @@ public void ArgumentListArgumentNullThrowsOnStart() Process testProcess = CreateProcess(); testProcess.StartInfo = psi; - Assert.Throws(() => testProcess.Start()); + AssertExtensions.Throws("item", () => testProcess.Start()); } [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]