Skip to content

Commit 4c29f57

Browse files
daxian-dbwlzybkr
daxian-dbw
authored and
lzybkr
committed
Fix bugs with expression redirected to file (#4847)
When handling file redirection for CommandExpression, we don't call 'DoComplete' on the underlying PipelineProcessor of the FileRedirection object, and thus the EndProcessing method is not called on Out-File, which causes different behaviors between <expr> > out.txt and <expr> | Out-File out.txt. The fix is to make sure 'DoComplete' is called after the stream output has been written to the redirection pipe. Also fix another issue This PR also fixes an issue that could mess up restoring the original pipes. Here is the repro: PS> 1 *> b.txt > a.txt; 123 Cannot perform operation because object "PipelineProcessor" has already been disposed The root cause is that we don't always restore pipes in the correct order. Please see the code changes in Compiler.cs for more details. Fix #4812
1 parent 916ef56 commit 4c29f57

3 files changed

Lines changed: 111 additions & 22 deletions

File tree

‎src/System.Management.Automation/engine/parser/Compiler.cs‎

Lines changed: 64 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,8 @@ internal static class CachedReflectionInfo
181181

182182
internal static readonly MethodInfo FileRedirection_BindForExpression =
183183
typeof(FileRedirection).GetMethod(nameof(FileRedirection.BindForExpression), instanceFlags);
184+
internal static readonly MethodInfo FileRedirection_CallDoCompleteForExpression =
185+
typeof(FileRedirection).GetMethod(nameof(FileRedirection.CallDoCompleteForExpression), instanceFlags);
184186
internal static readonly ConstructorInfo FileRedirection_ctor =
185187
typeof(FileRedirection).GetConstructor(instanceFlags, null, CallingConventions.Standard,
186188
new Type[] { typeof(RedirectionStream), typeof(bool), typeof(string) }, null);
@@ -3186,6 +3188,7 @@ private Expression GetRedirectedExpression(CommandExpressionAst commandExpr, boo
31863188
exprs.Add(Expression.Call(oldPipe, CachedReflectionInfo.Pipe_SetVariableListForTemporaryPipe, s_getCurrentPipe));
31873189
}
31883190

3191+
List<Expression> extraFileRedirectExprs = null;
31893192
// We must generate the code for output redirection to a file before any merging redirections
31903193
// because merging redirections will use funcContext._outputPipe as the value to merge to, so defer merging
31913194
// redirections until file redirections are done.
@@ -3194,35 +3197,55 @@ private Expression GetRedirectedExpression(CommandExpressionAst commandExpr, boo
31943197
// This will simply return a Linq.Expression representing the redirection.
31953198
var compiledRedirection = VisitFileRedirection(fileRedirectionAst);
31963199

3197-
// For non-output streams (error, warning, etc.) we must save the old stream so it can be restored.
3198-
// The savedPipe variable is used only for setting funcContext._outputPipe for redirecting Output to file.
3199-
// The savedPipes variable is used for restoring non-output streams (error, warning, etc.).
3200-
var savedPipes = NewTemp(typeof(Pipe[]), "savedPipes");
3201-
temps.Add(savedPipes);
3200+
if (extraFileRedirectExprs == null)
3201+
{
3202+
extraFileRedirectExprs = new List<Expression>(commandExpr.Redirections.Count);
3203+
}
32023204

3205+
// Hold the current 'FileRedirection' instance for later use
32033206
var redirectionExpr = NewTemp(typeof(FileRedirection), "fileRedirection");
32043207
temps.Add(redirectionExpr);
32053208
exprs.Add(Expression.Assign(redirectionExpr, (Expression)compiledRedirection));
3206-
/*
3207-
if (fileRedirectionAst.FromStream != RedirectionStream.Output && !(redirectionExpr is ConstantExpression))
3208-
{
3209-
// We'll be reusing redirectionExpr, it's not constant, so save it in a temp.
3210-
var temp = Expression.Variable(redirectionExpr.Type);
3211-
temps.Add(temp);
3212-
exprs.Add(Expression.Assign(temp, redirectionExpr));
3213-
redirectionExpr = temp;
3214-
}
3215-
*/
3209+
3210+
// We must save the old streams so they can be restored later.
3211+
var savedPipes = NewTemp(typeof(Pipe[]), "savedPipes");
3212+
temps.Add(savedPipes);
32163213
exprs.Add(Expression.Assign(
32173214
savedPipes,
32183215
Expression.Call(redirectionExpr, CachedReflectionInfo.FileRedirection_BindForExpression, _functionContext)));
3219-
finallyExprs.Add(Expression.Call(redirectionExpr.Cast(typeof(CommandRedirection)),
3220-
CachedReflectionInfo.CommandRedirection_UnbindForExpression,
3221-
_functionContext,
3222-
savedPipes));
3216+
3217+
// We need to call 'DoComplete' on the file redirection pipeline processor after writing the stream output to redirection pipe,
3218+
// so that the 'EndProcessing' method of 'Out-File' would be called as expected.
3219+
// Expressions for this purpose are kept in 'extraFileRedirectExprs' and will be used later.
3220+
extraFileRedirectExprs.Add(Expression.Call(redirectionExpr, CachedReflectionInfo.FileRedirection_CallDoCompleteForExpression));
3221+
3222+
// The 'UnBind' and 'Dispose' operations on 'FileRedirection' objects must be done in the reverse order of 'Bind' operations.
3223+
// Namely, it should be done is this order:
3224+
// try {
3225+
// // The order is A, B
3226+
// fileRedirectionA = new FileRedirection(..)
3227+
// fileRedirectionA.BindForExpression()
3228+
// fileRedirectionB = new FileRedirection(..)
3229+
// fileRedirectionB.BindForExpression()
3230+
// ...
3231+
// } finally {
3232+
// // The oder must be B, A
3233+
// fileRedirectionB.UnBind()
3234+
// fileRedirectionB.Dispose()
3235+
// fileRedirectionA.UnBind()
3236+
// fileRedirectionA.Dispose()
3237+
// }
3238+
//
3239+
// Otherwise, the original pipe might not be correctly restored in the end. For example,
3240+
// '1 *> b.txt > a.txt; 123' would result in the following error when evaluating '123':
3241+
// "Cannot perform operation because object "PipelineProcessor" has already been disposed"
3242+
finallyExprs.Insert(0, Expression.Call(redirectionExpr.Cast(typeof(CommandRedirection)),
3243+
CachedReflectionInfo.CommandRedirection_UnbindForExpression,
3244+
_functionContext,
3245+
savedPipes));
32233246
// In either case, we must dispose of the redirection or file handles won't get released.
3224-
finallyExprs.Add(Expression.Call(redirectionExpr,
3225-
CachedReflectionInfo.FileRedirection_Dispose));
3247+
finallyExprs.Insert(1, Expression.Call(redirectionExpr,
3248+
CachedReflectionInfo.FileRedirection_Dispose));
32263249
}
32273250

32283251
Expression result = null;
@@ -3287,10 +3310,29 @@ private Expression GetRedirectedExpression(CommandExpressionAst commandExpr, boo
32873310
}
32883311
}
32893312

3313+
if (extraFileRedirectExprs != null)
3314+
{
3315+
// Now that the redirection is done, we need to call 'DoComplete' on the file redirection pipeline processors.
3316+
// Exception may be thrown when running 'DoComplete', but we don't want the exception to affect the cleanup
3317+
// work in 'finallyExprs'. We generate the following code to make sure it does what we want.
3318+
// try {
3319+
// try {
3320+
// exprs
3321+
// } finally {
3322+
// extraFileRedirectExprs
3323+
// }
3324+
// finally {
3325+
// finallyExprs
3326+
// }
3327+
var wrapExpr = Expression.TryFinally(Expression.Block(exprs), Expression.Block(extraFileRedirectExprs));
3328+
exprs.Clear();
3329+
exprs.Add(wrapExpr);
3330+
}
3331+
32903332
if (oldPipe != null)
32913333
{
32923334
// If a temporary pipe was created at the beginning, we should restore the original pipe in the
3293-
// very end of the finally block. Otherwise, _getCurrentPipe may be messed up by the following
3335+
// very end of the finally block. Otherwise, s_getCurrentPipe may be messed up by the following
32943336
// file redirection unbind operation.
32953337
// For example:
32963338
// function foo

‎src/System.Management.Automation/engine/runtime/Operations/MiscOps.cs‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1158,6 +1158,23 @@ internal Pipe GetRedirectionPipe(ExecutionContext context, PipelineProcessor par
11581158
return new Pipe(context, PipelineProcessor);
11591159
}
11601160

1161+
/// <summary>
1162+
/// After file redirection is done, we need to call 'DoComplete' on the pipeline processor,
1163+
/// so that 'EndProcessing' of Out-File can be called to wrap up the file write operation.
1164+
/// </summary>
1165+
/// <remark>
1166+
/// 'StartStepping' is called after creating the pipeline processor.
1167+
/// 'Step' is called when an object is added to the pipe created with the pipeline processor.
1168+
/// </remark>
1169+
internal void CallDoCompleteForExpression()
1170+
{
1171+
// The pipe returned from 'GetRedirectionPipe' could be a NullPipe
1172+
if (PipelineProcessor != null)
1173+
{
1174+
PipelineProcessor.DoComplete();
1175+
}
1176+
}
1177+
11611178
private bool _disposed;
11621179

11631180
public void Dispose()

‎test/powershell/Language/Parser/RedirectionOperator.Tests.ps1‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,3 +92,33 @@ Describe "File redirection mixed with Out-Null" -Tags CI {
9292
Get-Content $TestDrive\out.txt | Should Be "some more text"
9393
}
9494
}
95+
96+
Describe "File redirection should have 'DoComplete' called on the underlying pipeline processor" -Tags CI {
97+
It "File redirection should result in the same file as Out-File" {
98+
$object = [pscustomobject] @{ one = 1 }
99+
$redirectFile = Join-Path $TestDrive fileRedirect.txt
100+
$outFile = Join-Path $TestDrive outFile.txt
101+
102+
$object > $redirectFile
103+
$object | Out-File $outFile
104+
105+
$redirectFileContent = Get-Content $redirectFile -Raw
106+
$outFileContent = Get-Content $outFile -Raw
107+
$redirectFileContent | Should Be $outFileContent
108+
}
109+
110+
It "File redirection should not mess up the original pipe" {
111+
$outputFile = Join-Path $TestDrive output.txt
112+
$otherStreamFile = Join-Path $TestDrive otherstream.txt
113+
114+
$result = & { $(Get-Command NonExist; 1234) > $outputFile *> $otherStreamFile; "Hello" }
115+
$result | Should Be "Hello"
116+
117+
$outputContent = Get-Content $outputFile -Raw
118+
$outputContent.Trim() | Should Be '1234'
119+
120+
$errorContent = Get-Content $otherStreamFile | ForEach-Object { $_.Trim() }
121+
$errorContent = $errorContent -join ""
122+
$errorContent | Should Match "CommandNotFoundException,Microsoft.PowerShell.Commands.GetCommandCommand"
123+
}
124+
}

0 commit comments

Comments
 (0)