Reject null characters in process environment variable names

[Copilot]

Embedded nulls in environment variable names can corrupt Windows environment blocks. This change rejects them during collection mutation.

• Validation
  • Throw ArgumentException from both indexers and all Add overloads.
  • Preserve existing null-key behavior.
• Coverage
  • Exercise generic and non-generic mutation paths.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[adamsitnik]

@copilot address my feedback and extend the tests to cover env var values as well.

[Copilot]

Pull request overview

This PR hardens ProcessStartInfo.Environment mutation behavior by rejecting environment variable names that contain embedded null ('\0') characters, preventing malformed environment blocks (notably on Windows).

Changes:

• Add key validation in the environment dictionary wrapper to throw ArgumentException for names containing '\0' during mutation.
• Introduce a localized resource string for the new validation error message.
• Add a regression test covering generic and non-generic mutation APIs (indexers and Add overloads).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs	Adds a test ensuring null-containing environment variable names are rejected across mutation entry points.
src/libraries/System.Diagnostics.Process/src/System/Collections/Specialized/DictionaryWrapper.cs	Centralizes mutation-time key validation (including '\0' checks) via the string indexer setter used by all Add/indexer mutation paths.
src/libraries/System.Diagnostics.Process/src/Resources/Strings.resx	Adds the new localized error message resource used by the thrown ArgumentException.

[dotnet-policy-service]

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

[Copilot]

@copilot address my feedback and extend the tests to cover env var values as well.

Addressed in 327a2ca. Validation and mutation-path coverage now reject embedded null characters in both keys and values.

Note

This response was generated by GitHub Copilot.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/libraries/System.Diagnostics.Process/src/Resources/Strings.resx:323

• The new exception message is ambiguous about whether it applies to the environment variable name, value, or both. Since the wrapper now validates both keys and values, consider updating the resource text to reflect that to make the error actionable.

  <data name="Argument_NullCharInEnvVar" xml:space="preserve">
    <value>Environment variable cannot contain a null character.</value>
  </data>

src/libraries/System.Diagnostics.Process/src/System/Collections/Specialized/DictionaryWrapper.cs:96

• ValidateKey takes a non-nullable string but then checks key is not null, which is redundant under nullable analysis and can trigger “always true” warnings. If the intent is to preserve the existing null-key behavior (letting the underlying Dictionary throw), make the helper accept a nullable string and use a null-safe check.

        private static void ValidateKey(string key)
        {
            if (key is not null && key.Contains('\0'))
            {
                throw new ArgumentException(SR.Argument_NullCharInEnvVar, nameof(key));

[adamsitnik]

@copilot address my feedback and reduce the code duplication

[adamsitnik]

Suggested change

	private static void ValidateKey(string key)
	{
	if (key is not null && key.Contains('\0'))
	{
	throw new ArgumentException(SR.Argument_NullCharInEnvVar, nameof(key));
	}
	}
	private static void ValidateValue(string? value)
	{
	if (value is not null && value.Contains('\0'))
	{
	throw new ArgumentException(SR.Argument_NullCharInEnvVar, nameof(value));
	}
	}
	private static void Validate(string name, string? value)
	{
	if (value is not null && value.Contains('\0'))
	{
	throw new ArgumentException(SR.Argument_NullCharInEnvVar, name);
	}
	}

[adamsitnik]

Let's reduce the code duplication

Suggested change

	ValidateKey(key);
	ValidateValue(value);
	Validate(nameof(key), key);
	Validate(nameof(value), value);

[Copilot]

@copilot address my feedback and reduce the code duplication

Addressed in dedf422c by consolidating duplicated key/value null-character checks into a single Validate(name, value) helper while keeping the existing key/value test coverage intact.

Note

This reply was generated by GitHub Copilot.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/libraries/System.Diagnostics.Process/src/System/Collections/Specialized/DictionaryWrapper.cs:25

• The new null-character validation can change the exception thrown when both arguments are invalid (e.g., key is null and value contains '\0'): the setter currently validates the value before the underlying Dictionary throws ArgumentNullException for the null key. To preserve the usual validation order (ArgumentNullException before ArgumentException) and keep null-key behavior consistent, explicitly throw for a null key before validating the value.

            set
            {
                Validate(nameof(key), key);
                Validate(nameof(value), value);
                _contents[key] = value;
            }

src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs:233

• The new test exercises generic IDictionary and non-generic IDictionary/ICollection mutation paths, but it doesn't cover the public EnvironmentVariables (StringDictionary) wrapper, which has its own indexer/Add implementations. Adding a couple of assertions for EnvironmentVariables helps ensure the same null-character contract is enforced across both public entry points.

            ProcessStartInfo psi = new ProcessStartInfo();
            IDictionary environment = (IDictionary)psi.Environment;
            ICollection<KeyValuePair<string, string>> environmentCollection = psi.Environment;