Fix TabExpansion2 variable leak when completing variables #18763

MartinGC94 · 2022-12-10T18:27:39Z

PR Summary

Fixes PowerShell/PowerShellEditorServices#1969 where tab completing variables would show Variables/Parameters from TabExpansion2.
This approach could probably be extended to fix a similar issue with PSConsoleHostReadLine (see #16941 ) but I'm not sure if I can add PSReadLine specific code here.

PR Context

PR Checklist

kilasuit · 2022-12-10T18:47:26Z

@MartinGC94 - PSReadline specific code would need to be pushed to that repo as it can then be used not only in v7 but also v5.1 too

SeeminglyScience · 2022-12-13T19:19:49Z

src/System.Management.Automation/engine/CommandCompletion/CommandCompletion.cs

-                            break;
-                        }
+                        scopeToRestore = context.EngineSessionState.CurrentScope;
+                        context.EngineSessionState.CurrentScope = scopeToRestore.Parent;


If TabExpansion2 is defined in a module then this will revert to the module's script scope rather than the actual previous scope.

Also if TabExpansion2 is dot sourced from global, Parent will be null here

If TabExpansion2 is defined in a module then this will revert to the module's script scope rather than the actual previous scope.

What's the difference? If I look at the session state API it seems to simply use the Parent property to move to different scopes:

https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/SessionStateScopeAPIs.cs#L128
https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/SessionStateScopeEnumerator.cs#L30
https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/SessionStateVariableAPIs.cs#L1791

Can you show a simple example where it would cause a problem?

Also if TabExpansion2 is dot sourced from global, Parent will be null here

How do you dot source from global? Do you mean like this: . $Function:TabExpansion2 "$" because if so it won't use the parent because the command name isn't "TabExpansion2".

What's the difference? If I look at the session state API it seems to simply use the Parent property to move to different scopes:

A SessionState is sort of like a Stack of scopes. Every module has its own stack. To move to a different stack, EngineSessionState needs to be assigned. Though, afaik the previous SessionState is only stored in a local variable inside a few differenttry/finallys.

How do you dot source from global? Do you mean like this: . $Function:TabExpansion2 "$" because if so it won't use the parent because the command name isn't "TabExpansion2".

You'd do . TabExpansion2 'etc', or the more likely route would be calling PowerShell.AddCommand("TabExpansion2", useLocalScope: false) from a host application.

All right, thanks for the explanation. I've added a null check for the dot sourcing scenario.
As for the module scope, would it be any different from the current behavior? If you are tab completing a variable today it's just calling Get-Item variable:\* from the tabexpansion2 scope which as far as I can tell retrieves variables from that scope and traverses through the Parent property on each scope. So with my change the only difference would be that it skips the initial TabExpansion2 scope, right?

All right, thanks for the explanation. I've added a null check for the dot sourcing scenario.

So null is one potential dot sourcing problem. Lets say the debugger is stopped in the middle of a script and the REPL starts. If TabExpansion2 is dot sourced then we'd be reverting not just TabExpansion2 but the scope of the currently running command as well. Now, I can't say for sure if that would be a problem, but some very bizarre bugs are possible when changing to an unexpected scope.

Ideally we'd be able to detect that TabExpansion2 was dot sourced and abort the attempt to revert scope. I don't know of a reliable way to tell that from here though, @daxian-dbw may have some ideas

As for the module scope, would it be any different from the current behavior?

Mmmm that's a fair point. Though maybe the behavior should be to skip trying to "fix" the issue if EngineSessionState is not TopLevelSessionState. I can't come up with a specific scenario, but changing scope when a module is involved worries me, and even the fix doesn't necessarily help the scenario.

Although at the same time, if TabExpansion2 is defined with no session state affinity, and lets say the debugger is stopped inside module code, then we'd want to revert still.

Maybe the check should be if (commandInfo.ScriptBlock.SesssionStateInternal != null) { DontRevert }?

Ideally we'd be able to detect that TabExpansion2 was dot sourced and abort the attempt to revert scope. I don't know of a reliable way to tell that from here though

I think you do without realizing. There's a UseLocalScope property in the CurrentCommandProcessor and you said that PowerShell.AddCommand("TabExpansion2", useLocalScope: false) is equivalent to dot sourcing, so can't we just use that? It seems to toggle back and forth like I would expect when I dotsource or run it normally.

Alternatively, instead of changing the scope we could just add the parent scope as an additional property to the CompletionContext object and update the variable/member completion code to use the parent scope if it's available. I don't think any other completion paths is affected by the session state but I could be wrong.

I think you do without realizing. There's a UseLocalScope property in the CurrentCommandProcessor and you said that PowerShell.AddCommand("TabExpansion2", useLocalScope: false) is equivalent to dot sourcing, so can't we just use that?

Oh yay I was wrong! 😁 I must have been thinking of when you only have the call stack, good catch!

ghost · 2022-12-21T02:00:56Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

adityapatwardhan · 2023-01-03T19:42:55Z

@MartinGC94 I would need signoff from either @daxian-dbw or @SeeminglyScience before this can be merged, as the as SMEs.

ghost · 2023-01-11T02:00:59Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

pull-request-quantifier-deprecated · 2023-01-18T19:54:43Z

This PR has 31 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Extra Small
Size       : +13 -18
Percentile : 12.4%

Total files changed: 2

Change summary by file extension:
.cs : +10 -18
.ps1 : +3 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

SeeminglyScience · 2023-01-19T17:22:28Z

FYI I haven't forgotten about this, I need some time to play around with it and see if any issues crop up

JamesWTruher

This looks fine, it seems that the automatic variables which are created by the use of [CmdletBinding()] for TabExpansion2 should also be removed from completion, yes?

MartinGC94 · 2023-06-19T18:43:03Z

it seems that the automatic variables which are created by the use of [CmdletBinding()] for TabExpansion2 should also be removed from completion, yes?

You mean variables like $PSCmdlet? I think they are in the list of built-in variables that are always added without looking at the session state or script text. I don't think that's a problem because people should be aware of them so they don't accidentally end up with conflicts when changing a function from basic to advanced because they happened to use the same var name.

SeeminglyScience

I couldn't find a way to break this so LGTM! Thanks again @MartinGC94! ❤️

daxian-dbw · 2023-06-19T22:53:17Z

This also fixes the following scenario:

$ast = [System.Management.Automation.Language.Parser]::ParseInput('$a = ''abc''', [ref]$null, [ref]$null)
$ast.End<tab>

It didn't work before because the variable $ast is defined in the TabExpansion2 scope.

ghost · 2023-06-29T22:08:10Z

🎉v7.4.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

Fix TabExpansion2 variable leak when completing variables

896748e

pull-request-quantifier-deprecated bot added the Extra Small label Dec 10, 2022

ghost assigned adityapatwardhan Dec 10, 2022

kilasuit requested a review from daxian-dbw December 10, 2022 18:47

kilasuit added WG-Interactive-PSReadLine PSReadline related issues WG-Interactive-Console the console experience WG-Interactive-IntelliSense tab completion labels Dec 10, 2022

MartinGC94 mentioned this pull request Dec 13, 2022

Type inference should handle [ref] #18759

Closed

SeeminglyScience reviewed Dec 13, 2022

View reviewed changes

MartinGC94 added 2 commits December 13, 2022 21:35

Add null check for parent scope

f1f2fab

Add check for dot sourced TabExpansion2 call.

8581b54

ghost added the Review - Needed The PR is being reviewed label Dec 21, 2022

ghost removed the Review - Needed The PR is being reviewed label Jan 3, 2023

ghost added the Review - Needed The PR is being reviewed label Jan 11, 2023

Merge branch 'master' into FixTabExpansion2VarLeak

c1adf27

daxian-dbw added the Needs-Triage The issue is new and needs to be triaged by a work group. label May 1, 2023

JamesWTruher approved these changes Jun 19, 2023

View reviewed changes

SeeminglyScience approved these changes Jun 19, 2023

View reviewed changes

daxian-dbw merged commit 994e43b into PowerShell:master Jun 19, 2023

daxian-dbw added CL-Engine Indicates that a PR should be marked as an engine change in the Change Log and removed Review - Needed The PR is being reviewed Needs-Triage The issue is new and needs to be triaged by a work group. labels Jun 19, 2023

ghost mentioned this pull request Jun 29, 2023

DLL fails on New-Object #1969

Closed

Fix TabExpansion2 variable leak when completing variables #18763

Fix TabExpansion2 variable leak when completing variables #18763

Uh oh!

Conversation

MartinGC94 commented Dec 10, 2022

PR Summary

PR Context

PR Checklist

Uh oh!

kilasuit commented Dec 10, 2022

Uh oh!

SeeminglyScience Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinGC94 Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

SeeminglyScience Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

MartinGC94 Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

SeeminglyScience Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

MartinGC94 Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

SeeminglyScience Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

ghost commented Dec 21, 2022

Uh oh!

adityapatwardhan commented Jan 3, 2023

Uh oh!

ghost commented Jan 11, 2023

Uh oh!

pull-request-quantifier-deprecated bot commented Jan 18, 2023

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

SeeminglyScience commented Jan 19, 2023

Uh oh!

JamesWTruher left a comment

Choose a reason for hiding this comment

Uh oh!

MartinGC94 commented Jun 19, 2023

Uh oh!

SeeminglyScience left a comment

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Jun 19, 2023

Uh oh!

ghost commented Jun 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

SeeminglyScience Dec 13, 2022 •

edited

Loading