Powershell: 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.

[mklement0]

I see. Leaving the comments issue aside (see below), do you agree with the proposed changes?

---

The reason I asked for comments is that I had a different use case in mind - and I know that this will eventually have to be a separate discussion, but just to get the ball rolling:

End users too can benefit from proxy functions - they're a powerful way to customize existing cmdlets.

For instance, I have a hd function in my profile (similar to the Unix head utility) that wraps Select-Object -First <n>.

In this scenario, comments are important, because the generated code does require tweaking by humans.

@jpsnover originally introduced the concept in this blog post from 2009 and even published a PSGallery module with a New-ProxyCommand function - thought it didn't gain much traction, possibly in part because it is rough around the edges.

I think better support for this use case is well worth considering, possibly even with an official cmdlet.

[lzybkr]

The code changes sound reasonable.

I think it's possible to generate a comment without hurting Exchange like scenarios. If the process is interactive (there are some existing checks already), it seems safe to assume PowerShell isn't being used in a server scenario where performance is more important.