Skip to content

Fix TabExpansion2 variable leak when completing variables #18763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
daxian-dbw merged 4 commits into from
Jun 19, 2023
Merged

Fix TabExpansion2 variable leak when completing variables #18763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
896748e
Fix TabExpansion2 variable leak when completing variables
MartinGC94 Dec 10, 2022
f1f2fab
Add null check for parent scope
MartinGC94 Dec 13, 2022
8581b54
Add check for dot sourced TabExpansion2 call.
MartinGC94 Dec 13, 2022
c1adf27
Merge branch 'master' into FixTabExpansion2VarLeak
MartinGC94 Jan 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 10 additions & 29 deletions src/System.Management.Automation/engine/CommandCompletion/CommandCompletion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,43 +537,25 @@ private static CommandCompletion CompleteInputImpl(Ast ast, Token[] tokens, IScr

if (results == null || results.Count == 0)
{
/* BROKEN code commented out, fix sometime
// If we were invoked from TabExpansion2, we want to "remove" TabExpansion2 and anything it calls
// from our results. We do this by faking out the session so that TabExpansion2 isn't anywhere to be found.
MutableTuple tupleForFrameToSkipPast = null;
foreach (var stackEntry in context.Debugger.GetCallStack())
SessionStateScope scopeToRestore;
if (context.CurrentCommandProcessor.Command.CommandInfo.Name.Equals("TabExpansion2", StringComparison.OrdinalIgnoreCase)
&& context.CurrentCommandProcessor.UseLocalScope
&& context.EngineSessionState.CurrentScope.Parent is not null)
{
dynamic stackEntryAsPSObj = PSObject.AsPSObject(stackEntry);
if (stackEntryAsPSObj.Command.Equals("TabExpansion2", StringComparison.OrdinalIgnoreCase))
{
tupleForFrameToSkipPast = stackEntry.FunctionContext._localsTuple;
break;
}
scopeToRestore = context.EngineSessionState.CurrentScope;
context.EngineSessionState.CurrentScope = scopeToRestore.Parent;
Copy link
Collaborator

@SeeminglyScience SeeminglyScience Dec 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@MartinGC94 MartinGC94 Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Collaborator

@SeeminglyScience SeeminglyScience Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@MartinGC94 MartinGC94 Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@SeeminglyScience SeeminglyScience Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }?

Copy link
Contributor Author

@MartinGC94 MartinGC94 Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@SeeminglyScience SeeminglyScience Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}

SessionStateScope scopeToRestore = null;
if (tupleForFrameToSkipPast != null)
else
{
// Find this tuple in the scope stack.
scopeToRestore = context.EngineSessionState.CurrentScope;
var scope = context.EngineSessionState.CurrentScope;
while (scope != null && scope.LocalsTuple != tupleForFrameToSkipPast)
{
scope = scope.Parent;
}

if (scope != null)
{
context.EngineSessionState.CurrentScope = scope.Parent;
}
scopeToRestore = null;
}

try
{
*/
var completionAnalysis = new CompletionAnalysis(ast, tokens, positionOfCursor, options);
results = completionAnalysis.GetResults(powershell, out replacementIndex, out replacementLength);
/*
var completionAnalysis = new CompletionAnalysis(ast, tokens, positionOfCursor, options);
results = completionAnalysis.GetResults(powershell, out replacementIndex, out replacementLength);
}
finally
{
Expand All @@ -582,7 +564,6 @@ private static CommandCompletion CompleteInputImpl(Ast ast, Token[] tokens, IScr
context.EngineSessionState.CurrentScope = scopeToRestore;
}
}
*/
}

var completionResults = results ?? EmptyCompletionResult;
Expand Down
5 changes: 5 additions & 0 deletions test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -837,6 +837,11 @@ Verb-Noun -Param1 Hello ^
$res.CompletionMatches[0].CompletionText | Should -Be "Get-ChildItem"
}

it 'Should not complete TabExpansion2 variables' {
$res = TabExpansion2 -inputScript '$' -cursorColumn 1
$res.CompletionMatches.CompletionText | Should -Not -Contain '$positionOfCursor'
}

it 'Should prefer the default parameterset when completing positional parameters' {
$ScriptInput = 'Get-ChildItem | Where-Object '
$res = TabExpansion2 -inputScript $ScriptInput -cursorColumn $ScriptInput.Length
Expand Down