Add GetProcessEnvironment command to diagnostics server

[josalem]

These changes send the Environment block via the GetProcessEnvironment command in the Diagnostics IPC Protocol.

The environment block is retrieved in the standard null delimited list of strings form, and is sent as an Array<Array<WCHAR>> in the IPC protocol. In other words, it is retrieved as

key=val\0key=val\0\0

and sent as (the | are logical separators and not part of the data)

2|8|key=val\0|8|key=val\0

Note that the tests do not print the bytes of the payload since they include the environment where the tests are run.

closes #40572

CC @dotnet/dotnet-diag @jander-msft

[ghost]

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

[tommcdon]

GH issue tracking this PR: #40572

[josalem]

Win64 libraries test failure is #40625

[davmason]

Two comments:

1. What happens if an environment variable has a pipe character in it? As far as I know it's perfectly legal to have an environment variable like "Foo=Bar|" and that seems like it would break your scenario since you use | as a separator

2. We already have a key/value pair format we use in EventPipe for filter arguments that uses semi-colons as delimiters key1=value1;key2=value2 and can escape using quotes. We should be intentional about whether or not we use the same format here, since it's solving the same issue

[josalem]

• What happens if an environment variable has a pipe character in it? As far as I know it's perfectly legal to have an environment variable like "Foo=Bar|" and that seems like it would break your scenario since you use | as a separator
• We already have a key/value pair format we use in EventPipe for filter arguments that uses semi-colons as delimiters key1=value1;key2=value2 and can escape using quotes. We should be intentional about whether or not we use the same format here, since it's solving the same issue

Ah, @davmason, the | in that block was meant to just separate the bytes that would be written. It's not actually a string with that character in it. A more literal representation would be this:

[ nElems  ] [ n chars ] [chars                         ] [n chars  ] [ chars                        ]
00 00 00 10 00 00 10 00 'k' 'e' 'y' '=' 'v' 'a' 'l' '\0' 00 00 10 00 'k' 'e' 'y' '=' 'v' 'a' 'l' '\0'

The event block is being sent as a length-prefixed array of strings which themselves are sent as length-prefixed arrays of wchars. It's not using characters as delimiters.

[sywhang]

LGTM mostly.

[noahfalk]

LGTM : )

[noahfalk]

In the overflow case this still writes lots of data even though GetEnvironmentBlockSize() specified only 4 bytes would be written. Separately I would assume in the overflow case we'd want to return an error in the header and write 0 bytes.

[josalem]

The logic should leave _nEnvEntries set to 0, so the payload would end up saying the continuation has 4 bytes and those 4 bytes should be the number 0 since there won't be any entries in the array. I could modify it so that the continuation length in the message payload is 0 instead and let the user treat that as an error. Would that be preferable?

[noahfalk]

We don't really care how the JIT optimizes this right?

[josalem]

I don't think so. This was held over from doing a copy/paste of the proj file for another event pipe test. I'll remove.

[noahfalk]

Is there a companion PR to the diagnostics repo with the spec update?

[josalem]

There will be. It hasn't been created yet.

[josalem]

Remaining failure is #34472. If there isn't any opposition, I'll merge this this afternoon.