Process: throw ArgumentNullException when ArgumentList contains a null.

[tmds]

Fixes #44034.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #44034.

Author:	tmds
Assignees:	-
Labels:	area-System.Diagnostics.Process
Milestone:	-

[stephentoub]

Fixes #44034.

How does this fix that issue?

[tmds]

How does this fix that issue?

The reproducer doesn't reproduce the issue.

The null gets passed through until it causes ArgumentNullException:

System.ArgumentNullException: String reference not set to an instance of a String. (Parameter 's')
  at System.Text.Encoding.GetBytes(String s)
  at Interop.Sys.AllocNullTerminatedArray(String[] arr, Byte**& arrPtr)
  at Interop.Sys.ForkAndExecProcess(String filename, String[] argv, String[] envp, String cwd, Boolean redirectStdin, Boolean redirectStdout, Boolean redirectStderr, Boolean setUser, UInt32 userId, UInt32 groupId, UInt32[] groups, In>
  at System.Diagnostics.Process.ForkAndExecProcess(String filename, String[] argv, String[] envp, String cwd, Boolean redirectStdin, Boolean redirectStdout, Boolean redirectStderr, Boolean setCredentials, UInt32 userId, UInt32 groupI>
  at System.Diagnostics.Process.StartCore(ProcessStartInfo startInfo)
  at System.Diagnostics.Process.Start(ProcessStartInfo startInfo)

[stephentoub]

The null gets passed through until it causes ArgumentNullException

What null? And if null is being added to the collection and that's the cause, isn't this still throwing an ArgumentNullException, just with a different stack?

[tmds]

What null? And if null is being added to the collection and that's the cause, isn't this still throwing an ArgumentNullException, just with a different stack?

The reproducer doesn't reproduce the issue. The stacktrace is correct though.

[stephentoub]

The reproducer doesn't reproduce the issue. The stacktrace is correct though.

I'm still confused.

The purpose of this PR isn't to prevent an exception / change something that wasn't working into something that does work, but rather just improve where an exception comes from. Yes?

[tmds]

The purpose of this PR isn't to prevent an exception / change something that wasn't working into something that does work, but rather just improve where an exception comes from. Yes?

Yes, it's validating the user doesn't add null into the ArgumentList.

When I run:

Process.Start(new ProcessStartInfo { FileName = "echo", ArgumentList = { null }});

It throws with the stacktrace from the issue.

Unhandled exception. System.ArgumentNullException: String reference not set to an instance of a String. (Parameter 's')
   at System.Text.Encoding.GetBytes(String s) in /_/src/libraries/System.Private.CoreLib/src/System/Text/Encoding.cs:line 663
   at System.Text.UTF8Encoding.UTF8EncodingSealed.GetBytes(String s) in /_/src/libraries/System.Private.CoreLib/src/System/Text/UTF8Encoding.Sealed.cs:line 49
   at Interop.Sys.AllocNullTerminatedArray(String[] arr, Byte**& arrPtr) in /_/src/libraries/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs:line 73
   at Interop.Sys.ForkAndExecProcess(String filename, String[] argv, String[] envp, String cwd, Boolean redirectStdin, Boolean redirectStdout, Boolean redirectStderr, Boolean setUser, UInt32 userId, UInt32 groupId, UInt32[] groups, Int32& lpChildPid, Int32& stdinFd, Int32& stdoutFd, Int32& stderrFd, Boolean shouldThrow) in /_/src/libraries/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs:line 26
   at System.Diagnostics.Process.ForkAndExecProcess(String filename, String[] argv, String[] envp, String cwd, Boolean redirectStdin, Boolean redirectStdout, Boolean redirectStderr, Boolean setCredentials, UInt32 userId, UInt32 groupId, UInt32[] groups, Int32& stdinFd, Int32& stdoutFd, Int32& stderrFd, Boolean usesTerminal, Boolean throwOnNoExec) in /_/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs:line 499
   at System.Diagnostics.Process.StartCore(ProcessStartInfo startInfo) in /_/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs:line 432
   at System.Diagnostics.Process.Start() in /_/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs:line 1234
   at System.Diagnostics.Process.Start(ProcessStartInfo startInfo) in /_/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs:line 1302
   at console.Program.Main(String[] args) in /tmp/console/Program.cs:line 10

[stephentoub]

It throws with the stacktrace from the issue.

And with this the exception looks like...?

[tmds]

And with this the exception looks like...?

Adding null into the ArgumentList throws ArgumentNullException.

runtime/src/libraries/System.ServiceModel.Syndication/src/System/ServiceModel/Syndication/NullNotAllowedCollection.cs

Lines 14 to 22 in 3d4fac0

	protected override void InsertItem(int index, TCollectionItem item)
	{
	if (item == null)
	{
	throw new ArgumentNullException(nameof(item));
	}
	base.InsertItem(index, item);
	}

[am11]

Should we ignore / tolerate null instead?

A quick search in dotnet org suggests all occurrences are pretty hard to reproduce: https://github.com/search?q=org:dotnet+"String+reference+not+set+to+an+instance+of+a+String."&type=issues. Thanks for finding out the root-cause! 👍

[tmds]

Should we ignore / tolerate null instead?

We shouldn't allow null unless we have a reason to do so.

I don't have a Windows machine. Looking at the code, I expect it to throw also.

Now, we'll throw sooner with a more meaningful exception that indicates the input is invalid.

[tmds]

@stephentoub I've moved the check into Process.Start. Can you take another look?

[tmds]

Feedback addressed, this should be good to merge.

[stephentoub]

Thanks