SDK: Proxy functions generated with [System.Management.Automation.ProxyCommand]::Create() incorrectly modify the -OutBuffer common parameter on invocation, mistakenly use Throw

[mklement0]

When you create a proxy command, the begin block contains the following snippet:

       $outBuffer = $null
        if ($PSBoundParameters.TryGetValue('OutBuffer', [ref]$outBuffer))
        {
            $PSBoundParameters['OutBuffer'] = 1
        }

That is, if a -OutBuffer value is passed, it is quietly reset to 1, which raises two questions:

• Why does the -OutBuffer value need to be reset at all for the steppable pipeline created by the proxy (wrapper) command?

• Assuming the intent is to reset the value so as to perform NO buffering, why is the value set to 1, which implies paired output (it is -OutBuffer 0 that specifies no buffering)?

Environment data

PowerShell Core 7.0.0-preview.4

[jhoneill]

I don't know why but I remember this when I first used this to create proxy functions. I have one which adds "-recurse" to select string, which was last modified in 2015, and has this code in it

           # For proxy functions using stepable pipeline Buffer should always be 1
           $outBuffer = $null
           if ($PSBoundParameters.TryGetValue('OutBuffer', [ref]$outBuffer)) {
                $PSBoundParameters['OutBuffer'] = 1
           }

So whatever the logic, it isn't new :-)

[vexx32]

@SeeminglyScience do you happen to know anything about this little oddity? 😊

[SeeminglyScience]

No idea. I always wondered why:

1. It doesn't use ContainsKey if it isn't even going to use $outBuffer (maybe that API didn't exist at the time?)
2. Why do it at all
3. Why set it to 1

Honestly I generally just remove it. I don't think I've ever actually used OutBuffer for anything though, so ymmv.

[mklement0]

Thanks, @jhoneill - indeed it isn't a new issue, but I was surprised to see the comment in your code snippet - was that auto-generated? If you use [System.Management.Automation.ProxyCommand]::Create() now, this comment is not there anymore.

@SeeminglyScience:

As for .ContainsKey(): the proxy-command class dates back to PSv2, but even then $PSBoundParameters implemented the generic IDicationary<T,T> interface, so I believe that the method was available.

Good point about removing the code - it does seem to work OK without it, even when you do use -OutBuffer, that rare beast.

Here's my guess as to what happened: The generated code is based on:

• a general misconception [see comment below]: since -OutBuffer only applies to the output side and the receiving cmdlet needn't be aware of whether the one providing the input used buffering or not - it still receives the objects one by one, there is no need to modify -OutBuffer at all; and - correct me if I'm wrong - on the output side the buffering is handled automatically by the plumbing.

• a specific misconception: -OutBuffer 1 sends 2 objects at a time through the pipeline (which is an understandable misconception and an unfortunate original design decision for -OutBuffer - too late...)

@SteveL-MSFT:

• Do you know anyone who can speak to the original design intent?

• Do we feel comfortable removing this part of the generated code? While it is typically benign, it actually thwarts the intentional use of -OutBuffer.

[jhoneill]

Thanks, @jhoneill - indeed it isn't a new issue, but I was surprised to see the comment in your code snippet - was that auto-generated? If you use [System.Management.Automation.ProxyCommand]::Create() now, this comment is not there anymore.

I don't think any comments are (or were) auto-generated, so it must have been my comment .

I don't understand why it has / always had

    try {
        $steppablePipeline.Process($_)
    } catch {
        throw
    }

Putting throw <> in a catch block doesn't seem very useful to me

[SeeminglyScience]

I don't understand why it has / always had

Yeah. I always replace throw with $PSCmdlet.ThrowTerminatingError($PSItem) personally.

Also inconsistent brace, inconsistent white space, and almost always unnecessary variable name escaping. (I think you fixed some of that recently though right @vexx32?)

[mklement0]

Good point, @jhoneill; to add to @SeeminglyScience's comment:

While argument-less throw does preserve the original error record (exception details), it converts a statement-terminating error into a script-terminating (runspace-terminating) error - so the wrapper (proxy) function will in fact behave differently than the wrapped cmdlet: it will not just terminate itself, but the entire runspace (unless caught).

$PSCmdlet.ThrowTerminatingError($_) avoids that problem.

To summarize:

• You do need try / catch - to prevent the proxy function from continuing when a a statement-terminating error occurs in the proxied function, ...

• ... but the catch block should call $PSCmdlet.ThrowTerminatingError($_), not throw.

[vexx32]

Yeah I tweak it a bit in #9900 I think, since I'm in there adding the dispose block to the template. Not merged yet, though.

[SteveL-MSFT]

cc @JamesWTruher

[lzybkr]

The OutBuffer code is to help avoid a DOS attack. Proxies are typically generated on servers for RBAC and we didn't want an attacker to easily create large buffers by specifying a large OutBuffer. I wish I could say I remembered this (I added the code), but I had to check my email archives.

IIRC, the try/catch/rethrow was added so errors with a proxy looked as much as possible like they did without the proxy. Any other benefit/downside was unintentional - at least on my part (that was also a change of mine.)

[mklement0]

Thanks, @lzybkr, but given that DOS isn't around anymore ... (just kidding).

So it sounds like the following changes are worth making:

• Streamline the OutBuffer code and precede it with a comment; e.g.:

# Ignore -OutBuffer to prevent DoS attacks. 
# If that is not a concern and you want -OutBuffer support, remove this `if` statement.
if ($PSBoundParameters.ContainsKey('OutBuffer')) { $PSBoundParameters.Remove('OutBuffer') }

Note that the code now removes the OutBuffer value (tantamount to setting it to 0, i.e. no buffering), which I presume was the original intent.

• In the spirit of so errors with a proxy looked as much as possible like they did without the proxy, replace the throw statements in all catch blocks with $PSCmdlet.ThrowTerminatingError($_) as suggested by @SeeminglyScience

[lzybkr]

A comment was not included largely for performance reasons - the generated code is mostly not meant for humans and Exchange generates many proxies.