From 6ca376298f8c1b8ed20dadef9795668280f6380a Mon Sep 17 00:00:00 2001 From: Hal <22532613+HalRiveria@users.noreply.github.com> Date: Wed, 28 Oct 2026 03:33:02 -0400 Subject: [PATCH 1/3] Refuse scriptblock job values at runtime; tighten Lint Rules 7-9 Invoke-ZipParallelProcessing now refuses a job whose value is a scriptblock, or holds one directly in an array or hashtable value, with a message naming the key and the [scriptblock]::Create() workaround. A -JobParams list is checked before anything runs, and each -NextJobProvider job as it's produced. The new Scenario H in the harness fails without this check. That replaces the static job-parameter matcher in Lint Rule 7, which missed several call shapes. Rule 7 now flags a scriptblock variable, or a [scriptblock]-typed parameter, passed to a .NET method or constructor, and only treats .GetNewClosure() results as scriptblocks. The new Rule 9 flags host cmdlets (Write-Host, Read-Host and others) on runspace-reachable paths. Workers share the console host, so these would bypass the console queue. The old cmdlet ban used to catch them by accident. Rules 7-9 share one traversal, and each function's file is recorded once instead of being searched for in every rule. Co-Authored-By: Claude Opus 5.5 --- Modules/ZipParallelProcessing.ps1 | 27 ++ _Harness/Lint.Harness.ps1 | 284 +++++++++------------ _Harness/ZipParallelProcessing.Harness.ps1 | 46 ++++ module-descriptions.md | 4 +- 4 files changed, 199 insertions(+), 162 deletions(-) diff --git a/Modules/ZipParallelProcessing.ps1 b/Modules/ZipParallelProcessing.ps1 index 20d35fb..cceeb8e 100644 --- a/Modules/ZipParallelProcessing.ps1 +++ b/Modules/ZipParallelProcessing.ps1 @@ -1,5 +1,20 @@ # See module-descriptions.md for this module's full description. +function Get-ZipJobScriptBlockKey { + param([hashtable]$Job) + foreach ($key in $Job.Keys) { + $value = $Job[$key] + if ($value -is [scriptblock]) { return [string]$key } + $inner = $null + if ($value -is [System.Collections.IDictionary]) { $inner = $value.Values } + elseif ($value -is [System.Collections.IEnumerable] -and $value -isnot [string]) { $inner = $value } + if ($null -ne $inner) { + foreach ($item in $inner) { if ($item -is [scriptblock]) { return [string]$key } } + } + } + return $null +} + function Invoke-ZipParallelProcessing { param( [Parameter(Mandatory)] @@ -27,6 +42,14 @@ function Invoke-ZipParallelProcessing { if ($null -ne $JobParams -and $JobParams.Count -eq 0) { return @{ ForceStoppedCount = 0 } } + $scriptBlockJobError = "Invoke-ZipParallelProcessing: job parameter '{0}' is (or contains) a scriptblock. A scriptblock belongs to the runspace that created it, and run inside a worker it can deadlock or crash the process. Pass plain data instead, or pass the code as a string and build it inside the worker with [scriptblock]::Create()." + if ($null -ne $JobParams) { + foreach ($job in $JobParams) { + $badKey = Get-ZipJobScriptBlockKey -Job $job + if ($null -ne $badKey) { throw ($scriptBlockJobError -f $badKey) } + } + } + $fixedListIndex = 0 $previousLogMutex = $null @@ -68,6 +91,10 @@ function Invoke-ZipParallelProcessing { } } else { $nextJob = & $NextJobProvider + if ($null -ne $nextJob) { + $badKey = Get-ZipJobScriptBlockKey -Job $nextJob + if ($null -ne $badKey) { throw ($scriptBlockJobError -f $badKey) } + } } if ($null -eq $nextJob) { $scanExhausted = $true; break } diff --git a/_Harness/Lint.Harness.ps1 b/_Harness/Lint.Harness.ps1 index 865cb14..24ee808 100644 --- a/_Harness/Lint.Harness.ps1 +++ b/_Harness/Lint.Harness.ps1 @@ -63,6 +63,15 @@ $script:LintScriptVarAllowed = @( Why = 'Only read after Test-AbortSignal''s own worker early-return (returns [ConsoleCtrl]::AbortRequested, never announces), i.e. main thread only.' } ) +# Rule 9 only: specific (function, host cmdlet) pairs reviewed as safe on a +# runspace-reachable path. Each entry must say why the cmdlet can't +# actually run inside a worker. +$script:LintHostCmdletAllowed = @( + @{ Function = 'Write-LogLineToHost' + Cmdlets = @('Write-Host') + Why = 'Only called from Write-Log past its worker early-return (worker lines go through the console queue) and from Flush-ConsoleQueue (main thread). Listed in Get-LoggingSharedFunctionNames only because Rule 5 walks Write-Log''s callees statically.' } +) + $script:passCount = 0 $script:failCount = 0 @@ -74,6 +83,14 @@ function Test-IsScriptVarAllowed { return $false } +function Test-IsHostCmdletAllowed { + param([string]$Function, [string]$Cmdlet) + foreach ($a in $script:LintHostCmdletAllowed) { + if ($a.Function -eq $Function -and ($a.Cmdlets -contains $Cmdlet)) { return $true } + } + return $false +} + function Test-Assert { param([bool]$Condition, [string]$Message) if ($Condition) { @@ -278,6 +295,7 @@ Write-Host "" Write-Host "=== Lint: -SharedFunctionNames missing a transitively-called function ===" -ForegroundColor Cyan $fnAstByName = @{} +$fnFileByName = @{} $fileAsts = @{} $allFnNames = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::OrdinalIgnoreCase) foreach ($name in $fileText.Keys) { @@ -285,6 +303,7 @@ foreach ($name in $fileText.Keys) { $fileAsts[$name] = $ast foreach ($f in $ast.FindAll({ param($n) $n -is [System.Management.Automation.Language.FunctionDefinitionAst] }, $true)) { $fnAstByName[$f.Name] = $f + $fnFileByName[$f.Name] = $name $null = $allFnNames.Add($f.Name) } } @@ -349,6 +368,7 @@ function Get-MissingSharedFunctionNames { # not a bare ScriptBlockExpressionAst, and this codebase's own # ResultHandler callbacks already use .GetNewClosure() elsewhere. $scriptBlockVars = @{} +$scriptBlockVarFile = @{} foreach ($name in $fileText.Keys) { foreach ($assign in $fileAsts[$name].FindAll({ param($n) $n -is [System.Management.Automation.Language.AssignmentStatementAst] }, $true)) { if ($assign.Right -is [System.Management.Automation.Language.CommandExpressionAst] -and @@ -359,6 +379,7 @@ foreach ($name in $fileText.Keys) { } if ($rhsExpr -is [System.Management.Automation.Language.ScriptBlockExpressionAst]) { $scriptBlockVars[$assign.Left.VariablePath.UserPath] = $rhsExpr.ScriptBlock + $scriptBlockVarFile[$assign.Left.VariablePath.UserPath] = $name } } } @@ -449,6 +470,26 @@ Test-Assert (@($rule5).Count -eq 0) "every -SharedFunctionNames array covers eve Show-Findings -Findings @($rule5) Write-Host "" +# Runs $Finder (param($AstNode, $FunctionName), returning @{ Line; What } +# items) over every runspace-reachable function and worker body, and shapes +# the hits for Show-Findings. $FunctionName is $null for a worker body. +function Get-RunspaceReachableFindings { + param([scriptblock]$Finder) + $out = [System.Collections.Generic.List[object]]::new() + foreach ($fname in $runspaceReachable) { + if (-not $fnAstByName.ContainsKey($fname)) { continue } + foreach ($hit in (& $Finder $fnAstByName[$fname] $fname)) { + $out.Add(@{ File = $fnFileByName[$fname]; Line = $hit.Line; Text = ($fname + " (runspace-reachable) " + $hit.What) }) + } + } + foreach ($workerVar in $runspaceWorkerBodies) { + foreach ($hit in (& $Finder $scriptBlockVars[$workerVar] $null)) { + $out.Add(@{ File = $scriptBlockVarFile[$workerVar]; Line = $hit.Line; Text = ("worker `$" + $workerVar + " " + $hit.What) }) + } + } + return ,$out +} + # ------------------------------------------------------------------------ # Rule 6: $script: variable used on a runspace-reachable path # ------------------------------------------------------------------------ @@ -478,8 +519,7 @@ $scriptVarRefFilter = { foreach ($fname in $runspaceReachable) { if (-not $fnAstByName.ContainsKey($fname)) { continue } $fnAst = $fnAstByName[$fname] - $fnFile = $null - foreach ($name in $fileAsts.Keys) { if ($fileText[$name].Contains($fnAst.Extent.Text)) { $fnFile = $name; break } } + $fnFile = $fnFileByName[$fname] foreach ($ref in $fnAst.FindAll($scriptVarRefFilter, $true)) { $varName = $ref.VariablePath.UserPath -replace '^script:', '' if (Test-IsScriptVarAllowed -Function $fname -Variable $varName) { continue } @@ -488,8 +528,7 @@ foreach ($fname in $runspaceReachable) { } foreach ($workerVar in $runspaceWorkerBodies) { $body = $scriptBlockVars[$workerVar] - $workerFile = $null - foreach ($name in $fileAsts.Keys) { if ($fileText[$name].Contains($body.Extent.Text)) { $workerFile = $name; break } } + $workerFile = $scriptBlockVarFile[$workerVar] foreach ($nested in $body.FindAll({ param($n) $n -is [System.Management.Automation.Language.ScriptBlockExpressionAst] }, $true)) { foreach ($ref in $nested.FindAll($scriptVarRefFilter, $true)) { $varName = $ref.VariablePath.UserPath -replace '^script:', '' @@ -502,29 +541,29 @@ Show-Findings -Findings @($rule6) Write-Host "" # ------------------------------------------------------------------------ -# Rule 7: a scriptblock crossing into a worker runspace +# Rule 7: a scriptblock variable handed to .NET on a runspace-reachable path # ------------------------------------------------------------------------ # A scriptblock belongs to the runspace that created it. Run on another -# runspace's thread - as a delegate (Rule 8), or because it was handed -# over as a job parameter or through shared state and called with & - -# every scriptblock PowerShell runs inside it (a Where-Object filter, a -# nested delegate) gets queued back to the owning runspace, and the -# calling thread waits. If the owner is itself waiting on that thread (a -# lock, a Lazy), both hang for good; if the owner is idle, the queued -# block can run against the owner's scope stack and crash the process. -# Workers get plain data only. +# runspace's thread, every scriptblock PowerShell runs inside it (a +# Where-Object filter, a nested delegate) gets queued back to the owning +# runspace, and the calling thread waits. If the owner is itself waiting on +# that thread (a lock, a Lazy), both hang for good; if the owner is idle, +# the queued block can run against the owner's scope stack and crash the +# process. # -# Two shapes, both flagged: -# - a job-parameter hashtable handed to Invoke-ZipParallelProcessing (a -# -JobParams entry, or what -NextJobProvider returns) with a -# scriptblock value -# - runspace-reachable code passing a variable that holds a scriptblock -# to a .NET method or constructor ($dict.TryAdd($k, $sb), -# [System.Lazy[object]]::new($factory)) - .NET receives a delegate, -# or stores it where another runspace can run it. Rule 8 covers the -# literal and cast forms of the same thing. +# Two ways a scriptblock reaches another runspace: +# - as a job parameter. Invoke-ZipParallelProcessing refuses those at +# runtime (Get-ZipJobScriptBlockKey), so every harness or real run +# that dispatches a job already catches it - no static check here. +# - through .NET: runspace-reachable code passing a scriptblock to a .NET +# method or constructor ($dict.TryAdd($k, $sb), +# [System.Lazy[object]]::new($Factory)). .NET receives a delegate, or +# stores it where another runspace can run it. This rule flags a +# variable holding one - assigned a scriptblock literal, or a +# [scriptblock]-typed parameter. Rule 8 covers the literal and cast +# forms of the same thing. # Add "# lint:ignore" on the line for a deliberate exception. -Write-Host "=== Lint: scriptblock crossing into a worker runspace ===" -ForegroundColor Cyan +Write-Host "=== Lint: scriptblock variable handed to .NET on a runspace-reachable path ===" -ForegroundColor Cyan function Get-ScriptBlockVariableNames { param($AstNode) @@ -533,82 +572,21 @@ function Get-ScriptBlockVariableNames { if ($assign.Left -is [System.Management.Automation.Language.VariableExpressionAst] -and $assign.Right -is [System.Management.Automation.Language.CommandExpressionAst]) { $rhsExpr = $assign.Right.Expression - if ($rhsExpr -is [System.Management.Automation.Language.InvokeMemberExpressionAst]) { $rhsExpr = $rhsExpr.Expression } + if ($rhsExpr -is [System.Management.Automation.Language.InvokeMemberExpressionAst] -and $rhsExpr.Member.Extent.Text -eq 'GetNewClosure') { $rhsExpr = $rhsExpr.Expression } if ($rhsExpr -is [System.Management.Automation.Language.ScriptBlockExpressionAst]) { $null = $names.Add($assign.Left.VariablePath.UserPath) } } } - return ,$names -} - -function Test-IsScriptBlockValue { - param($Expr, $ScriptBlockVarNames) - if ($Expr -is [System.Management.Automation.Language.PipelineAst] -and $Expr.PipelineElements.Count -eq 1) { $Expr = $Expr.PipelineElements[0] } - if ($Expr -is [System.Management.Automation.Language.CommandExpressionAst]) { $Expr = $Expr.Expression } - if ($Expr -is [System.Management.Automation.Language.InvokeMemberExpressionAst]) { $Expr = $Expr.Expression } - if ($Expr -is [System.Management.Automation.Language.ScriptBlockExpressionAst]) { return $true } - return ($Expr -is [System.Management.Automation.Language.VariableExpressionAst] -and - $ScriptBlockVarNames.Contains($Expr.VariablePath.UserPath)) -} - -# Job-parameter hashtables for every Invoke-ZipParallelProcessing call -# under $RootAst: hashtables .Add()ed to (or assigned to) the -JobParams -# variable, and hashtables the -NextJobProvider scriptblock returns. -$jobParamScriptBlockFinder = { - param($RootAst) - $found = [System.Collections.Generic.List[object]]::new() - foreach ($call in $RootAst.FindAll({ param($n) $n -is [System.Management.Automation.Language.CommandAst] -and $n.GetCommandName() -eq 'Invoke-ZipParallelProcessing' }, $true)) { - $scope = $call.Parent - while ($scope -and -not ($scope -is [System.Management.Automation.Language.FunctionDefinitionAst])) { $scope = $scope.Parent } - if (-not $scope) { $scope = $RootAst } - $sbVarNames = Get-ScriptBlockVariableNames -AstNode $scope - - $jobParamsVar = $null; $providerVar = $null - for ($i = 0; ($i + 1) -lt $call.CommandElements.Count; $i++) { - $el = $call.CommandElements[$i]; $valEl = $call.CommandElements[$i + 1] - if ($el -isnot [System.Management.Automation.Language.CommandParameterAst] -or - $valEl -isnot [System.Management.Automation.Language.VariableExpressionAst]) { continue } - if ($el.ParameterName -eq 'JobParams') { $jobParamsVar = $valEl.VariablePath.UserPath } - if ($el.ParameterName -eq 'NextJobProvider') { $providerVar = $valEl.VariablePath.UserPath } - } - - $tables = [System.Collections.Generic.List[object]]::new() - if ($jobParamsVar) { - foreach ($m in $scope.FindAll({ param($n) $n -is [System.Management.Automation.Language.InvokeMemberExpressionAst] }, $true)) { - if ($m.Expression -is [System.Management.Automation.Language.VariableExpressionAst] -and - $m.Expression.VariablePath.UserPath -eq $jobParamsVar -and $m.Member.Extent.Text -eq 'Add' -and $m.Arguments) { - foreach ($argument in $m.Arguments) { - foreach ($h in $argument.FindAll({ param($n) $n -is [System.Management.Automation.Language.HashtableAst] }, $true)) { $tables.Add($h) } - } - } - } - } - foreach ($assign in $scope.FindAll({ param($n) $n -is [System.Management.Automation.Language.AssignmentStatementAst] }, $true)) { - if ($assign.Left -isnot [System.Management.Automation.Language.VariableExpressionAst]) { continue } - $assignedName = $assign.Left.VariablePath.UserPath - if ($jobParamsVar -and $assignedName -eq $jobParamsVar) { - foreach ($h in $assign.Right.FindAll({ param($n) $n -is [System.Management.Automation.Language.HashtableAst] }, $true)) { $tables.Add($h) } - } - if ($providerVar -and $assignedName -eq $providerVar) { - foreach ($ret in $assign.Right.FindAll({ param($n) $n -is [System.Management.Automation.Language.ReturnStatementAst] }, $true)) { - if ($null -eq $ret.Pipeline) { continue } - foreach ($h in $ret.Pipeline.FindAll({ param($n) $n -is [System.Management.Automation.Language.HashtableAst] }, $true)) { $tables.Add($h) } - } - } - } - - foreach ($h in $tables) { - foreach ($pair in $h.KeyValuePairs) { - if ($pair.Item2.Extent.StartScriptPosition.Line -match '#\s*lint:ignore') { continue } - if (Test-IsScriptBlockValue -Expr $pair.Item2 -ScriptBlockVarNames $sbVarNames) { - $found.Add(@{ Line = $pair.Item2.Extent.StartLineNumber; What = ("job parameter '" + $pair.Item1.Extent.Text + "' is a scriptblock") }) - } + foreach ($p in $AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.ParameterAst] }, $true)) { + foreach ($attr in $p.Attributes) { + if ($attr -is [System.Management.Automation.Language.TypeConstraintAst] -and + $attr.TypeName.FullName -match '^(System\.Management\.Automation\.)?scriptblock$') { + $null = $names.Add($p.Name.VariablePath.UserPath) } } } - return ,$found + return ,$names } -# A scriptblock-holding variable passed to a .NET method or constructor. $scriptBlockArgFinder = { param($AstNode) $found = [System.Collections.Generic.List[object]]::new() @@ -628,64 +606,29 @@ $scriptBlockArgFinder = { return ,$found } -# Proves both finders fire, since nothing in the real code trips them. +# Proves the finder fires, since nothing in the real code trips it. $rule7SampleText = @' -function Sample-Caller { - $cb = { param($x) $x } - $plain = 'value' - $jobs = [System.Collections.Generic.List[hashtable]]::new() - $jobs.Add(@{ Name = $plain; Callback = $cb; Inline = { 1 }; Closure = { 2 }.GetNewClosure() }) - $provider = { - return @{ Path = $plain; Hook = $cb } - }.GetNewClosure() - $jobs.Add(@{ Skipped = $cb }) # lint:ignore - sample - Invoke-ZipParallelProcessing -WorkerScript $w -JobParams $jobs -NextJobProvider $provider -} function Sample-Worker { - $factory = { 3 }.GetNewClosure() - $lazy = [System.Lazy[object]]::new($factory) - $null = $dict.TryAdd('k', $factory) - $null = $dict.TryAdd('k', $plain) - $e = @(1, 2).Where($factory) + param([scriptblock]$Factory, [string]$Name) + $built = { 3 }.GetNewClosure() + $result = { 4 }.Invoke() + $lazy = [System.Lazy[object]]::new($Factory) + $null = $dict.TryAdd('k', $built) + $null = $dict.TryAdd('k', $Name) + $null = $dict.TryAdd('k', $result) + $e = @(1, 2).Where($built) + $null = $dict.TryAdd('k', $built) # lint:ignore - sample } '@ $rule7SampleAst = [System.Management.Automation.Language.Parser]::ParseInput($rule7SampleText, [ref]$null, [ref]$null) -$rule7JobSample = & $jobParamScriptBlockFinder $rule7SampleAst -$rule7JobSampleText = @($rule7JobSample | ForEach-Object { $_.What }) -Test-Assert ($rule7JobSampleText -contains "job parameter 'Callback' is a scriptblock") "Rule 7 flags a scriptblock variable in a -JobParams hashtable" -Test-Assert (($rule7JobSampleText -contains "job parameter 'Inline' is a scriptblock") -and ($rule7JobSampleText -contains "job parameter 'Closure' is a scriptblock")) "Rule 7 flags a scriptblock literal (bare or .GetNewClosure()'d) in a -JobParams hashtable" -Test-Assert ($rule7JobSampleText -contains "job parameter 'Hook' is a scriptblock") "Rule 7 flags a scriptblock in a hashtable the -NextJobProvider returns" -Test-Assert ($rule7JobSample.Count -eq 4) ("Rule 7 leaves plain job-parameter values and a lint:ignore'd line alone (flagged " + $rule7JobSample.Count + ", expected 4)") -$rule7ArgSample = & $scriptBlockArgFinder $rule7SampleAst.Find({ param($n) $n -is [System.Management.Automation.Language.FunctionDefinitionAst] -and $n.Name -eq 'Sample-Worker' }, $true) -$rule7ArgSampleLines = @($rule7ArgSample | ForEach-Object { $_.Line }) -Test-Assert ($rule7ArgSampleLines -contains 14) "Rule 7 flags a scriptblock variable passed to a constructor ([System.Lazy]::new(`$factory))" -Test-Assert ($rule7ArgSampleLines -contains 15) "Rule 7 flags a scriptblock variable stored through a .NET method (TryAdd)" -Test-Assert ($rule7ArgSample.Count -eq 2) ("Rule 7 leaves a plain value and .Where() alone (flagged " + $rule7ArgSample.Count + ", expected 2)") - -$rule7 = @() -foreach ($name in $fileAsts.Keys) { - foreach ($hit in (& $jobParamScriptBlockFinder $fileAsts[$name])) { - $rule7 += @{ File = $name; Line = $hit.Line; Text = $hit.What } - } -} -foreach ($fname in $runspaceReachable) { - if (-not $fnAstByName.ContainsKey($fname)) { continue } - $fnAst = $fnAstByName[$fname] - $fnFile = $null - foreach ($name in $fileAsts.Keys) { if ($fileText[$name].Contains($fnAst.Extent.Text)) { $fnFile = $name; break } } - foreach ($hit in (& $scriptBlockArgFinder $fnAst)) { - $rule7 += @{ File = $fnFile; Line = $hit.Line; Text = ($fname + " (runspace-reachable) " + $hit.What) } - } -} -foreach ($workerVar in $runspaceWorkerBodies) { - $body = $scriptBlockVars[$workerVar] - $workerFile = $null - foreach ($name in $fileAsts.Keys) { if ($fileText[$name].Contains($body.Extent.Text)) { $workerFile = $name; break } } - foreach ($hit in (& $scriptBlockArgFinder $body)) { - $rule7 += @{ File = $workerFile; Line = $hit.Line; Text = ("worker `$" + $workerVar + " " + $hit.What) } - } -} -Test-Assert (@($rule7).Count -eq 0) "no scriptblock is handed to a worker runspace as a job parameter or passed to .NET from runspace-reachable code" +$rule7Sample = & $scriptBlockArgFinder $rule7SampleAst +$rule7SampleLines = @($rule7Sample | ForEach-Object { $_.Line }) +Test-Assert ($rule7SampleLines -contains 5) "Rule 7 flags a [scriptblock] parameter passed to a constructor ([System.Lazy]::new(`$Factory))" +Test-Assert ($rule7SampleLines -contains 6) "Rule 7 flags a .GetNewClosure()'d scriptblock variable stored through a .NET method (TryAdd)" +Test-Assert ($rule7Sample.Count -eq 2) ("Rule 7 leaves a plain parameter, a scriptblock's .Invoke() result, .Where() and a lint:ignore'd line alone (flagged " + $rule7Sample.Count + ", expected 2)") + +$rule7 = Get-RunspaceReachableFindings -Finder $scriptBlockArgFinder +Test-Assert (@($rule7).Count -eq 0) "no runspace-reachable function or worker body passes a scriptblock variable to .NET" Show-Findings -Findings @($rule7) Write-Host "" @@ -755,27 +698,46 @@ Test-Assert ($rule8SampleLines -contains 2) "Rule 8 flags a written delegate cas Test-Assert ($rule8SampleLines -contains 3) "Rule 8 flags a scriptblock passed straight to a .NET method (GetOrAdd)" Test-Assert ($rule8SampleLines -contains 4) "Rule 8 flags a scriptblock passed straight to a constructor ([System.Lazy]::new)" Test-Assert ($rule8SampleFound.Count -eq 3) ("Rule 8 leaves .ForEach()/.Where(), a lint:ignore'd line and a plain call alone (flagged " + $rule8SampleFound.Count + ", expected 3)") -foreach ($fname in $runspaceReachable) { - if (-not $fnAstByName.ContainsKey($fname)) { continue } - $fnAst = $fnAstByName[$fname] - $fnFile = $null - foreach ($name in $fileAsts.Keys) { if ($fileText[$name].Contains($fnAst.Extent.Text)) { $fnFile = $name; break } } - foreach ($hit in (& $delegateCastFinder $fnAst)) { - $rule8 += @{ File = $fnFile; Line = $hit.Line; Text = ($fname + " (runspace-reachable) " + $hit.What) } - } -} -foreach ($workerVar in $runspaceWorkerBodies) { - $body = $scriptBlockVars[$workerVar] - $workerFile = $null - foreach ($name in $fileAsts.Keys) { if ($fileText[$name].Contains($body.Extent.Text)) { $workerFile = $name; break } } - foreach ($hit in (& $delegateCastFinder $body)) { - $rule8 += @{ File = $workerFile; Line = $hit.Line; Text = ("worker `$" + $workerVar + " " + $hit.What) } - } -} +$rule8 = Get-RunspaceReachableFindings -Finder $delegateCastFinder Test-Assert (@($rule8).Count -eq 0) "no runspace-reachable function or worker body turns a scriptblock into a delegate (written cast or implicit)" Show-Findings -Findings @($rule8) Write-Host "" +# ------------------------------------------------------------------------ +# Rule 9: host cmdlet on a runspace-reachable path +# ------------------------------------------------------------------------ +# Worker runspaces share the real console host (the pool is built with +# $Host), so a worker writing straight to it bypasses the console queue the +# main thread drains - its lines interleave with, and scramble, Clean +# Console Output's live display - and Read-Host would prompt from a +# background thread mid-run. Worker output goes through Write-Log (the +# console queue) instead. Other cmdlets are fine in runspace code. +# Deliberate, reviewed exceptions go in $script:LintHostCmdletAllowed with +# a reason. +Write-Host "=== Lint: host cmdlet on a runspace-reachable path ===" -ForegroundColor Cyan + +$hostCmdletNames = @('Write-Host', 'Read-Host', 'Out-Host', 'Clear-Host', 'Get-Credential', 'Out-GridView') +$hostCmdletFinder = { + param($AstNode, $FunctionName) + $found = [System.Collections.Generic.List[object]]::new() + foreach ($c in $AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.CommandAst] }, $true)) { + $cmdName = $c.GetCommandName() + if (-not $cmdName -or $hostCmdletNames -notcontains $cmdName) { continue } + if ($FunctionName -and (Test-IsHostCmdletAllowed -Function $FunctionName -Cmdlet $cmdName)) { continue } + if ($c.Extent.StartScriptPosition.Line -match '#\s*lint:ignore') { continue } + $found.Add(@{ Line = $c.Extent.StartLineNumber; What = ("calls host cmdlet " + $cmdName) }) + } + return ,$found +} +$rule9SampleAst = [System.Management.Automation.Language.Parser]::ParseInput("function Sample-Worker {`n Write-Host 'x'`n `$a = Read-Host`n Write-Output 'y'`n}", [ref]$null, [ref]$null) +$rule9Sample = & $hostCmdletFinder $rule9SampleAst 'Sample-Worker' +Test-Assert ((@($rule9Sample | ForEach-Object { $_.Line }) -join ',') -eq '2,3') "Rule 9 flags Write-Host and Read-Host, and leaves Write-Output alone" + +$rule9 = Get-RunspaceReachableFindings -Finder $hostCmdletFinder +Test-Assert (@($rule9).Count -eq 0) "no runspace-reachable function or worker body calls a host cmdlet" +Show-Findings -Findings @($rule9) +Write-Host "" + # ============================================================================== # Summary # ============================================================================== diff --git a/_Harness/ZipParallelProcessing.Harness.ps1 b/_Harness/ZipParallelProcessing.Harness.ps1 index a17006b..03608b2 100644 --- a/_Harness/ZipParallelProcessing.Harness.ps1 +++ b/_Harness/ZipParallelProcessing.Harness.ps1 @@ -533,6 +533,52 @@ Test-Assert ($outcomeG -is [hashtable]) ("the return is exactly one hashtable (g Test-Assert ($outcomeG -is [hashtable] -and $outcomeG.ForceStoppedCount -eq 0) "ForceStoppedCount is still readable on it" Write-Host "" +# ============================================================================== +# Scenario H: a scriptblock job value is refused before any worker runs. +# A scriptblock belongs to the runspace that created it; run inside a worker +# it can deadlock or crash the process. +# ============================================================================== +Write-Host "=== Scenario H: scriptblock job values are refused ===" -ForegroundColor Cyan +Reset-HarnessAbortState +$sharedH = @(Get-LoggingSharedFunctionNames) +function Invoke-ScenarioHJobs { + param($Job, [switch]$UseProvider) + $results = [System.Collections.Generic.List[hashtable]]::new() + $cts = [System.Threading.CancellationTokenSource]::new() + $err = $null + try { + if ($UseProvider) { + $state = @{ Given = $false } + $provider = { if ($state.Given) { return $null }; $state.Given = $true; return $Job }.GetNewClosure() + $null = Invoke-ZipParallelProcessing -WorkerScript $fakeWorkerOk -NextJobProvider $provider -SharedFunctionNames $sharedH ` + -MaxParallel 1 -Cts $cts -ResultHandler { param($r) $results.Add($r) }.GetNewClosure() + } else { + $list = [System.Collections.Generic.List[hashtable]]::new() + $list.Add($Job) + $null = Invoke-ZipParallelProcessing -WorkerScript $fakeWorkerOk -JobParams $list -SharedFunctionNames $sharedH ` + -MaxParallel 1 -Cts $cts -ResultHandler { param($r) $results.Add($r) }.GetNewClosure() + } + } catch { $err = $_.ToString() } + $cts.Dispose() + return @{ Error = $err; Results = $results.Count } +} +$baseH = @{ JobId = 1; DelayMs = 0; DoFullLogging = $FullLogging; LogFile = $logFile; LogErrorFile = $logErrorFile; LogWarnFile = $logWarnFile } +function New-ScenarioHJob { param([hashtable]$Extra) $j = $baseH.Clone(); foreach ($k in $Extra.Keys) { $j[$k] = $Extra[$k] }; return $j } + +$h1 = Invoke-ScenarioHJobs -Job (New-ScenarioHJob @{ Callback = { 1 } }) +Test-Assert ($null -ne $h1.Error -and $h1.Error -match "'Callback'" -and $h1.Error -match '\[scriptblock\]::Create') ("a -JobParams scriptblock value throws, naming the key and the workaround (got: " + $h1.Error + ")") +Test-Assert ($h1.Results -eq 0) "no worker ran for the refused -JobParams list" +$h2 = Invoke-ScenarioHJobs -Job (New-ScenarioHJob @{ Steps = @('a', { 2 }) }) +Test-Assert ($null -ne $h2.Error -and $h2.Error -match "'Steps'") "a scriptblock inside an array job value is refused" +$h3 = Invoke-ScenarioHJobs -Job (New-ScenarioHJob @{ Options = @{ OnDone = { 3 } } }) +Test-Assert ($null -ne $h3.Error -and $h3.Error -match "'Options'") "a scriptblock inside a hashtable job value is refused" +$h4 = Invoke-ScenarioHJobs -Job (New-ScenarioHJob @{ Hook = { 4 } }) -UseProvider +Test-Assert ($null -ne $h4.Error -and $h4.Error -match "'Hook'" -and $h4.Results -eq 0) "a scriptblock in a -NextJobProvider job is refused before that job runs" +$h5 = Invoke-ScenarioHJobs -Job (New-ScenarioHJob @{ Code = '{ 5 }'; Names = @('x', 'y'); Map = @{ K = 'v' } }) +Test-Assert ($null -eq $h5.Error -and $h5.Results -eq 1) ("plain data, including code passed as a string, still runs (error: " + $h5.Error + ", results: " + $h5.Results + ")") +Reset-HarnessAbortState +Write-Host "" + # ============================================================================== # Summary # ============================================================================== diff --git a/module-descriptions.md b/module-descriptions.md index ce9e54a..20c67f0 100644 --- a/module-descriptions.md +++ b/module-descriptions.md @@ -30,7 +30,7 @@ Every module is a file of function (and type) definitions with no side effects w Zip Folders' workers and Full Pipeline's two engines run in separate PowerShell runspaces, and anything they call must follow two rules. Each module's section says which of its functions run there. 1. **Don't read `$script:` variables directly.** A function copied into a runspace can resolve `$script:` to an empty scope (inside a `.GetNewClosure()` block, for example), and the read silently comes back empty. Workers get their state as parameters, or through `Get-WorkerRunspaceValue` (`Logging.ps1`). -2. **Never run a scriptblock in a runspace other than the one that created it.** Workers get plain data, not scriptblocks, as job parameters and through shared caches, and no scriptblock is handed to .NET as a delegate (`[Func[...]]{ }`, a `System.Lazy` factory, or a scriptblock passed to a method such as `ConcurrentDictionary.GetOrAdd`), since .NET can then call it from another engine's thread. A scriptblock running on another runspace's thread queues every scriptblock it calls (a `Where-Object` filter, a nested delegate) back to the runspace it came from and waits. If that runspace is waiting on the first thread, both hang; if it's idle, the process can crash. +2. **Never run a scriptblock in a runspace other than the one that created it.** Workers get plain data, not scriptblocks, as job parameters (`Invoke-ZipParallelProcessing` refuses a scriptblock job value) and through shared caches, and no scriptblock is handed to .NET as a delegate (`[Func[...]]{ }`, a `System.Lazy` factory, or a scriptblock passed to a method such as `ConcurrentDictionary.GetOrAdd`), since .NET can then call it from another engine's thread. A scriptblock running on another runspace's thread queues every scriptblock it calls (a `Where-Object` filter, a nested delegate) back to the runspace it came from and waits. If that runspace is waiting on the first thread, both hang; if it's idle, the process can crash. Cmdlets are fine in worker code. The code uses .NET calls in many of those places for historical reasons, not because a cmdlet would break. @@ -489,12 +489,14 @@ A shared runspace-pool dispatcher. It runs a worker scriptblock once per job in | Function | What it does | |---|---| +| `Get-ZipJobScriptBlockKey` | Returns the first key in a job hashtable whose value is a scriptblock, or an array, list or hashtable directly holding one; `$null` if there is none. | | `Invoke-ZipParallelProcessing` | Runs `-WorkerScript` for each job in a runspace pool of at most `-MaxParallel`, feeds every result to `-ResultHandler` on the main thread, and returns `@{ ForceStoppedCount = n }`. | **How it works** - Jobs come from exactly one of two sources: `-JobParams` (a fixed list of parameter hashtables) or `-NextJobProvider` (a scriptblock called on the main thread that returns the next hashtable, or `$null` when finished). Supplying both or neither throws. An empty `-JobParams` list returns immediately. - Each job's hashtable keys are passed to the worker as named parameters. The function adds three more: `CancelToken` (from `-Cts`), `LogMutex` and `ConsoleQueue`. A worker script must declare those parameters. +- Every job is checked before it starts: a scriptblock value, or one directly inside an array, list or hashtable value, throws a message naming the key (see the worker-runspace rules above). A `-JobParams` list is checked whole before anything runs; a `-NextJobProvider` job is checked as it is produced. To hand a worker code, pass it as a string and build it inside the worker with `[scriptblock]::Create()`. - The pool is built from a fresh `InitialSessionState`. Only the functions named in `-SharedFunctionNames` are copied into it (by reading each function's definition and adding a function entry). A name that doesn't resolve throws a message naming the entry. Nothing else from the calling script exists inside a worker. - Each run creates its own log mutex (`Initialize-LogMutex`) and a `ConcurrentQueue` for console output. The previous `$script:logMutex` is restored, and the new mutex disposed, when the call ends. - The main loop keeps the pool filled up to `-MaxParallel`, then on each pass: checks `Test-AbortSignal`, flushes the console queue (`Flush-ConsoleQueue`), calls `-PollCallback` if given, and harvests completed jobs. It sleeps 100 ms only when it has nothing to start. From 1ba8d3e889eb4f1b360f3f2cb222d088a7541db8 Mon Sep 17 00:00:00 2001 From: Hal <22532613+HalRiveria@users.noreply.github.com> Date: Wed, 28 Oct 2026 04:01:05 -0400 Subject: [PATCH 2/3] Harden the scriptblock job guard; widen Lint Rules 7 and 9 Job guard (Invoke-ZipParallelProcessing): - Looks up to four levels down, through arrays, lists, hashtables and [pscustomobject] properties. - Reads each collection once per call. A collection passed by reference to every job is checked with the first job, before any worker can be writing to it. A collection that can't be read because something is changing it is let through instead of failing the run. - A refused -NextJobProvider job no longer throws mid-loop and stops the workers already running. New jobs stop starting, the running ones finish and report, and then the call throws. Lint: - Rule 7 also flags a scriptblock stored through an indexer or property on a container the code didn't create ($cache[$k] = $sb, $slot.Factory = { }). - Rule 9 resolves aliases (cls, clear, oh) and flags direct [Console]:: and $Host.UI use. module-descriptions.md documents the console rule as a third worker-runspace rule. The new harness and Lint assertions fail against the previous code. Co-Authored-By: Claude Opus 5.5 --- Modules/ZipParallelProcessing.ps1 | 42 ++++++--- _Harness/Lint.Harness.ps1 | 101 +++++++++++++++++---- _Harness/ZipParallelProcessing.Harness.ps1 | 39 ++++++++ module-descriptions.md | 9 +- 4 files changed, 156 insertions(+), 35 deletions(-) diff --git a/Modules/ZipParallelProcessing.ps1 b/Modules/ZipParallelProcessing.ps1 index cceeb8e..14f3ef7 100644 --- a/Modules/ZipParallelProcessing.ps1 +++ b/Modules/ZipParallelProcessing.ps1 @@ -1,16 +1,31 @@ # See module-descriptions.md for this module's full description. +function Test-ZipJobValueHasScriptBlock { + param($Value, [System.Collections.Generic.HashSet[object]]$Seen, [int]$Depth) + if ($null -eq $Value) { return $false } + if ($Value -is [scriptblock]) { return $true } + if ($Depth -ge 4 -or $Value -is [string] -or $Value -is [System.ValueType]) { return $false } + $isCustomObject = $Value -is [System.Management.Automation.PSCustomObject] + if (-not $isCustomObject -and $Value -isnot [System.Collections.IEnumerable]) { return $false } + if (-not $isCustomObject -and -not $Seen.Add($Value)) { return $false } + try { + if ($isCustomObject) { $items = foreach ($p in $Value.PSObject.Properties) { $p.Value } } + elseif ($Value -is [System.Collections.IDictionary]) { $items = $Value.Values } + else { $items = $Value } + foreach ($item in $items) { + if (Test-ZipJobValueHasScriptBlock -Value $item -Seen $Seen -Depth ($Depth + 1)) { return $true } + } + } catch { + # A collection something else is writing to right now can't be read + # safely; let it through unchecked rather than fail the run over it. + } + return $false +} + function Get-ZipJobScriptBlockKey { - param([hashtable]$Job) + param([hashtable]$Job, [System.Collections.Generic.HashSet[object]]$Seen) foreach ($key in $Job.Keys) { - $value = $Job[$key] - if ($value -is [scriptblock]) { return [string]$key } - $inner = $null - if ($value -is [System.Collections.IDictionary]) { $inner = $value.Values } - elseif ($value -is [System.Collections.IEnumerable] -and $value -isnot [string]) { $inner = $value } - if ($null -ne $inner) { - foreach ($item in $inner) { if ($item -is [scriptblock]) { return [string]$key } } - } + if (Test-ZipJobValueHasScriptBlock -Value $Job[$key] -Seen $Seen -Depth 0) { return [string]$key } } return $null } @@ -43,9 +58,11 @@ function Invoke-ZipParallelProcessing { if ($null -ne $JobParams -and $JobParams.Count -eq 0) { return @{ ForceStoppedCount = 0 } } $scriptBlockJobError = "Invoke-ZipParallelProcessing: job parameter '{0}' is (or contains) a scriptblock. A scriptblock belongs to the runspace that created it, and run inside a worker it can deadlock or crash the process. Pass plain data instead, or pass the code as a string and build it inside the worker with [scriptblock]::Create()." + $checkedJobValues = [System.Collections.Generic.HashSet[object]]::new() + $refusedJobError = $null if ($null -ne $JobParams) { foreach ($job in $JobParams) { - $badKey = Get-ZipJobScriptBlockKey -Job $job + $badKey = Get-ZipJobScriptBlockKey -Job $job -Seen $checkedJobValues if ($null -ne $badKey) { throw ($scriptBlockJobError -f $badKey) } } } @@ -92,8 +109,8 @@ function Invoke-ZipParallelProcessing { } else { $nextJob = & $NextJobProvider if ($null -ne $nextJob) { - $badKey = Get-ZipJobScriptBlockKey -Job $nextJob - if ($null -ne $badKey) { throw ($scriptBlockJobError -f $badKey) } + $badKey = Get-ZipJobScriptBlockKey -Job $nextJob -Seen $checkedJobValues + if ($null -ne $badKey) { $refusedJobError = $scriptBlockJobError -f $badKey; $nextJob = $null } } } if ($null -eq $nextJob) { $scanExhausted = $true; break } @@ -158,6 +175,7 @@ function Invoke-ZipParallelProcessing { Flush-ConsoleQueue -Queue $localConsoleQueue if ($null -ne $PollCallback) { $null = & $PollCallback } + if ($null -ne $refusedJobError) { throw $refusedJobError } } finally { # A worker stuck inside a .NET call can't be stopped, and Stop()/ # Dispose()/Close() would all sit and wait for it, so an abandoned worker may keep running. diff --git a/_Harness/Lint.Harness.ps1 b/_Harness/Lint.Harness.ps1 index 24ee808..aa634a5 100644 --- a/_Harness/Lint.Harness.ps1 +++ b/_Harness/Lint.Harness.ps1 @@ -541,7 +541,7 @@ Show-Findings -Findings @($rule6) Write-Host "" # ------------------------------------------------------------------------ -# Rule 7: a scriptblock variable handed to .NET on a runspace-reachable path +# Rule 7: a scriptblock handed to .NET or stored into shared state on a runspace-reachable path # ------------------------------------------------------------------------ # A scriptblock belongs to the runspace that created it. Run on another # runspace's thread, every scriptblock PowerShell runs inside it (a @@ -555,15 +555,17 @@ Write-Host "" # - as a job parameter. Invoke-ZipParallelProcessing refuses those at # runtime (Get-ZipJobScriptBlockKey), so every harness or real run # that dispatches a job already catches it - no static check here. -# - through .NET: runspace-reachable code passing a scriptblock to a .NET -# method or constructor ($dict.TryAdd($k, $sb), -# [System.Lazy[object]]::new($Factory)). .NET receives a delegate, or -# stores it where another runspace can run it. This rule flags a -# variable holding one - assigned a scriptblock literal, or a -# [scriptblock]-typed parameter. Rule 8 covers the literal and cast -# forms of the same thing. +# - through .NET or shared state: runspace-reachable code passing a +# scriptblock to a .NET method or constructor ($dict.TryAdd($k, $sb), +# [System.Lazy[object]]::new($Factory)), or storing one into a +# collection or object it didn't create itself ($Cache[$k] = $sb, +# $Slot.Factory = { ... }) - a parameter or outer variable, so +# possibly shared with the other engine. This rule flags a variable +# holding a scriptblock (assigned a scriptblock literal, or a +# [scriptblock]-typed parameter) passed to .NET, and any scriptblock +# value stored that way. Rule 8 covers literals and casts passed to .NET. # Add "# lint:ignore" on the line for a deliberate exception. -Write-Host "=== Lint: scriptblock variable handed to .NET on a runspace-reachable path ===" -ForegroundColor Cyan +Write-Host "=== Lint: scriptblock handed to .NET or stored into shared state on a runspace-reachable path ===" -ForegroundColor Cyan function Get-ScriptBlockVariableNames { param($AstNode) @@ -591,7 +593,6 @@ $scriptBlockArgFinder = { param($AstNode) $found = [System.Collections.Generic.List[object]]::new() $sbVarNames = Get-ScriptBlockVariableNames -AstNode $AstNode - if ($sbVarNames.Count -eq 0) { return ,$found } foreach ($call in $AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.InvokeMemberExpressionAst] }, $true)) { if ($null -eq $call.Arguments) { continue } $memberName = [string]$call.Member.Extent.Text @@ -603,13 +604,37 @@ $scriptBlockArgFinder = { } } } + # Stores: $x[...] = / $x.Prop = , where $x + # isn't a variable this code assigns itself (a local container is fine). + $localNames = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::OrdinalIgnoreCase) + foreach ($assign in $AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.AssignmentStatementAst] }, $true)) { + if ($assign.Left -is [System.Management.Automation.Language.VariableExpressionAst]) { $null = $localNames.Add($assign.Left.VariablePath.UserPath) } + } + foreach ($assign in $AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.AssignmentStatementAst] }, $true)) { + $target = $assign.Left + if ($target -isnot [System.Management.Automation.Language.IndexExpressionAst] -and + $target -isnot [System.Management.Automation.Language.MemberExpressionAst]) { continue } + if ($assign.Right -isnot [System.Management.Automation.Language.CommandExpressionAst]) { continue } + $value = $assign.Right.Expression + if ($value -is [System.Management.Automation.Language.InvokeMemberExpressionAst] -and $value.Member.Extent.Text -eq 'GetNewClosure') { $value = $value.Expression } + $isScriptBlock = ($value -is [System.Management.Automation.Language.ScriptBlockExpressionAst]) -or + ($value -is [System.Management.Automation.Language.VariableExpressionAst] -and $sbVarNames.Contains($value.VariablePath.UserPath)) + if (-not $isScriptBlock) { continue } + $root = $target + while ($root -is [System.Management.Automation.Language.IndexExpressionAst] -or $root -is [System.Management.Automation.Language.MemberExpressionAst]) { + if ($root -is [System.Management.Automation.Language.IndexExpressionAst]) { $root = $root.Target } else { $root = $root.Expression } + } + if ($root -is [System.Management.Automation.Language.VariableExpressionAst] -and $localNames.Contains($root.VariablePath.UserPath)) { continue } + if ($assign.Extent.StartScriptPosition.Line -match '#\s*lint:ignore') { continue } + $found.Add(@{ Line = $assign.Extent.StartLineNumber; What = ("stores a scriptblock into " + $target.Extent.Text + ", which it didn't create") }) + } return ,$found } # Proves the finder fires, since nothing in the real code trips it. $rule7SampleText = @' function Sample-Worker { - param([scriptblock]$Factory, [string]$Name) + param([scriptblock]$Factory, [string]$Name, $Cache) $built = { 3 }.GetNewClosure() $result = { 4 }.Invoke() $lazy = [System.Lazy[object]]::new($Factory) @@ -618,6 +643,11 @@ function Sample-Worker { $null = $dict.TryAdd('k', $result) $e = @(1, 2).Where($built) $null = $dict.TryAdd('k', $built) # lint:ignore - sample + $Cache['k'] = $Factory + $Cache.Hook = { 5 }.GetNewClosure() + $local = @{} + $local['k'] = $built + $Cache['n'] = $Name } '@ $rule7SampleAst = [System.Management.Automation.Language.Parser]::ParseInput($rule7SampleText, [ref]$null, [ref]$null) @@ -625,10 +655,12 @@ $rule7Sample = & $scriptBlockArgFinder $rule7SampleAst $rule7SampleLines = @($rule7Sample | ForEach-Object { $_.Line }) Test-Assert ($rule7SampleLines -contains 5) "Rule 7 flags a [scriptblock] parameter passed to a constructor ([System.Lazy]::new(`$Factory))" Test-Assert ($rule7SampleLines -contains 6) "Rule 7 flags a .GetNewClosure()'d scriptblock variable stored through a .NET method (TryAdd)" -Test-Assert ($rule7Sample.Count -eq 2) ("Rule 7 leaves a plain parameter, a scriptblock's .Invoke() result, .Where() and a lint:ignore'd line alone (flagged " + $rule7Sample.Count + ", expected 2)") +Test-Assert ($rule7SampleLines -contains 11) "Rule 7 flags a scriptblock stored through an indexer on a container it didn't create (`$Cache['k'] = `$Factory)" +Test-Assert ($rule7SampleLines -contains 12) "Rule 7 flags a scriptblock literal stored into a property of a container it didn't create (`$Cache.Hook = { }.GetNewClosure())" +Test-Assert ($rule7Sample.Count -eq 4) ("Rule 7 leaves a plain parameter, a scriptblock's .Invoke() result, .Where(), a lint:ignore'd line, a local container and a plain stored value alone (flagged " + $rule7Sample.Count + ", expected 4)") $rule7 = Get-RunspaceReachableFindings -Finder $scriptBlockArgFinder -Test-Assert (@($rule7).Count -eq 0) "no runspace-reachable function or worker body passes a scriptblock variable to .NET" +Test-Assert (@($rule7).Count -eq 0) "no runspace-reachable function or worker body passes a scriptblock to .NET or stores one into a container it didn't create" Show-Findings -Findings @($rule7) Write-Host "" @@ -716,22 +748,53 @@ Write-Host "" # a reason. Write-Host "=== Lint: host cmdlet on a runspace-reachable path ===" -ForegroundColor Cyan +# The same goes for writing to the console directly: [Console]::... and +# $Host.UI. Those go in the allow-list as '[Console]' and '$Host.UI'. $hostCmdletNames = @('Write-Host', 'Read-Host', 'Out-Host', 'Clear-Host', 'Get-Credential', 'Out-GridView') +# Aliases (cls, clear, oh, ogv, ...) resolve to the cmdlet they stand for. +$hostCmdletByName = @{} +foreach ($n in $hostCmdletNames) { $hostCmdletByName[$n] = $n } +foreach ($a in @(Get-Alias -Definition $hostCmdletNames -ErrorAction SilentlyContinue)) { $hostCmdletByName[$a.Name] = $a.Definition } $hostCmdletFinder = { param($AstNode, $FunctionName) $found = [System.Collections.Generic.List[object]]::new() + $hits = [System.Collections.Generic.List[object]]::new() foreach ($c in $AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.CommandAst] }, $true)) { $cmdName = $c.GetCommandName() - if (-not $cmdName -or $hostCmdletNames -notcontains $cmdName) { continue } - if ($FunctionName -and (Test-IsHostCmdletAllowed -Function $FunctionName -Cmdlet $cmdName)) { continue } - if ($c.Extent.StartScriptPosition.Line -match '#\s*lint:ignore') { continue } - $found.Add(@{ Line = $c.Extent.StartLineNumber; What = ("calls host cmdlet " + $cmdName) }) + if (-not $cmdName -or -not $hostCmdletByName.ContainsKey($cmdName)) { continue } + $hits.Add(@{ Ast = $c; Name = $hostCmdletByName[$cmdName]; What = ("calls host cmdlet " + $cmdName) }) + } + foreach ($m in $AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.MemberExpressionAst] }, $true)) { + if ($m.Static -and $m.Expression -is [System.Management.Automation.Language.TypeExpressionAst] -and + $m.Expression.TypeName.FullName -match '^(System\.)?Console$') { + $hits.Add(@{ Ast = $m; Name = '[Console]'; What = ("uses [Console]::" + $m.Member.Extent.Text) }) + } elseif ($m.Expression -is [System.Management.Automation.Language.VariableExpressionAst] -and + $m.Expression.VariablePath.UserPath -match '^(global:)?Host$' -and $m.Member.Extent.Text -eq 'UI') { + $hits.Add(@{ Ast = $m; Name = '$Host.UI'; What = "uses `$Host.UI" }) + } + } + foreach ($h in $hits) { + if ($FunctionName -and (Test-IsHostCmdletAllowed -Function $FunctionName -Cmdlet $h.Name)) { continue } + if ($h.Ast.Extent.StartScriptPosition.Line -match '#\s*lint:ignore') { continue } + $found.Add(@{ Line = $h.Ast.Extent.StartLineNumber; What = $h.What }) } return ,$found } -$rule9SampleAst = [System.Management.Automation.Language.Parser]::ParseInput("function Sample-Worker {`n Write-Host 'x'`n `$a = Read-Host`n Write-Output 'y'`n}", [ref]$null, [ref]$null) +$rule9SampleText = @' +function Sample-Worker { + Write-Host 'x' + $a = Read-Host + Write-Output 'y' + cls + [Console]::WriteLine('z') + $Host.UI.WriteLine('w') + $width = $Host.Name +} +'@ +$rule9SampleAst = [System.Management.Automation.Language.Parser]::ParseInput($rule9SampleText, [ref]$null, [ref]$null) $rule9Sample = & $hostCmdletFinder $rule9SampleAst 'Sample-Worker' -Test-Assert ((@($rule9Sample | ForEach-Object { $_.Line }) -join ',') -eq '2,3') "Rule 9 flags Write-Host and Read-Host, and leaves Write-Output alone" +$rule9SampleLines = @($rule9Sample | ForEach-Object { $_.Line } | Sort-Object) +Test-Assert (($rule9SampleLines -join ',') -eq '2,3,5,6,7') ("Rule 9 flags Write-Host, Read-Host, an alias (cls), [Console]:: and `$Host.UI, and leaves Write-Output and other `$Host members alone (flagged lines " + ($rule9SampleLines -join ',') + ", expected 2,3,5,6,7)") $rule9 = Get-RunspaceReachableFindings -Finder $hostCmdletFinder Test-Assert (@($rule9).Count -eq 0) "no runspace-reachable function or worker body calls a host cmdlet" diff --git a/_Harness/ZipParallelProcessing.Harness.ps1 b/_Harness/ZipParallelProcessing.Harness.ps1 index 03608b2..2e9eb49 100644 --- a/_Harness/ZipParallelProcessing.Harness.ps1 +++ b/_Harness/ZipParallelProcessing.Harness.ps1 @@ -576,6 +576,45 @@ $h4 = Invoke-ScenarioHJobs -Job (New-ScenarioHJob @{ Hook = { 4 } }) -UseProvide Test-Assert ($null -ne $h4.Error -and $h4.Error -match "'Hook'" -and $h4.Results -eq 0) "a scriptblock in a -NextJobProvider job is refused before that job runs" $h5 = Invoke-ScenarioHJobs -Job (New-ScenarioHJob @{ Code = '{ 5 }'; Names = @('x', 'y'); Map = @{ K = 'v' } }) Test-Assert ($null -eq $h5.Error -and $h5.Results -eq 1) ("plain data, including code passed as a string, still runs (error: " + $h5.Error + ", results: " + $h5.Results + ")") +$h6 = Invoke-ScenarioHJobs -Job (New-ScenarioHJob @{ Steps = @(@{ Run = { 6 } }) }) +Test-Assert ($null -ne $h6.Error -and $h6.Error -match "'Steps'") "a scriptblock two levels down (an array of hashtables) is refused" +$h7 = Invoke-ScenarioHJobs -Job (New-ScenarioHJob @{ Config = [pscustomobject]@{ Name = 'x'; OnDone = { 7 } } }) +Test-Assert ($null -ne $h7.Error -and $h7.Error -match "'Config'") "a scriptblock held in a [pscustomobject] property is refused" + +# A collection shared by reference across jobs is read once per run, not +# once per job: later jobs may run while workers are writing to it. +if (-not ([System.Management.Automation.PSTypeName]'ZppEnumerationCounter').Type) { + Add-Type -TypeDefinition @' +public class ZppEnumerationCounter : System.Collections.IEnumerable { + public int Enumerations; + public System.Collections.IEnumerator GetEnumerator() { Enumerations++; return new object[] { "a", "b" }.GetEnumerator(); } +} +'@ +} +$counterH = [ZppEnumerationCounter]::new() +$seenH = [System.Collections.Generic.HashSet[object]]::new() +$keysH = @(@(foreach ($i in 1..3) { Get-ZipJobScriptBlockKey -Job @{ JobId = $i; Shared = $counterH } -Seen $seenH }) | Where-Object { $null -ne $_ }) +Test-Assert ($keysH.Count -eq 0 -and $counterH.Enumerations -eq 1) ("a collection shared by three jobs is enumerated once, not per job (enumerated " + $counterH.Enumerations + " times)") + +# A provider job refused mid-run lets the jobs already running finish +# normally, then throws - it doesn't stop them or cancel the caller's token. +$resultsH9 = [System.Collections.Generic.List[hashtable]]::new() +$ctsH9 = [System.Threading.CancellationTokenSource]::new() +$stateH9 = @{ N = 0 } +$providerH9 = { + $stateH9.N++ + if ($stateH9.N -eq 1) { return (New-ScenarioHJob @{ JobId = 1; DelayMs = 800 }) } + if ($stateH9.N -eq 2) { return (New-ScenarioHJob @{ JobId = 2; Hook = { 9 } }) } + return $null +} +$errH9 = $null +try { + $null = Invoke-ZipParallelProcessing -WorkerScript $fakeWorkerOk -NextJobProvider $providerH9 -SharedFunctionNames $sharedH ` + -MaxParallel 2 -Cts $ctsH9 -ResultHandler { param($r) $resultsH9.Add($r) } +} catch { $errH9 = $_.ToString() } +Test-Assert ($null -ne $errH9 -and $errH9 -match "'Hook'") "a provider job refused mid-run still makes the call throw" +Test-Assert ($resultsH9.Count -eq 1 -and -not $ctsH9.Token.IsCancellationRequested) ("the job already running finished and reported, and the caller's token wasn't cancelled (results " + $resultsH9.Count + ", cancelled " + $ctsH9.Token.IsCancellationRequested + ")") +$ctsH9.Dispose() Reset-HarnessAbortState Write-Host "" diff --git a/module-descriptions.md b/module-descriptions.md index 20c67f0..13b0bc7 100644 --- a/module-descriptions.md +++ b/module-descriptions.md @@ -27,12 +27,13 @@ Every module is a file of function (and type) definitions with no side effects w ## Rules for code that runs in worker runspaces -Zip Folders' workers and Full Pipeline's two engines run in separate PowerShell runspaces, and anything they call must follow two rules. Each module's section says which of its functions run there. +Zip Folders' workers and Full Pipeline's two engines run in separate PowerShell runspaces, and anything they call must follow three rules. Each module's section says which of its functions run there. 1. **Don't read `$script:` variables directly.** A function copied into a runspace can resolve `$script:` to an empty scope (inside a `.GetNewClosure()` block, for example), and the read silently comes back empty. Workers get their state as parameters, or through `Get-WorkerRunspaceValue` (`Logging.ps1`). 2. **Never run a scriptblock in a runspace other than the one that created it.** Workers get plain data, not scriptblocks, as job parameters (`Invoke-ZipParallelProcessing` refuses a scriptblock job value) and through shared caches, and no scriptblock is handed to .NET as a delegate (`[Func[...]]{ }`, a `System.Lazy` factory, or a scriptblock passed to a method such as `ConcurrentDictionary.GetOrAdd`), since .NET can then call it from another engine's thread. A scriptblock running on another runspace's thread queues every scriptblock it calls (a `Where-Object` filter, a nested delegate) back to the runspace it came from and waits. If that runspace is waiting on the first thread, both hang; if it's idle, the process can crash. +3. **Don't write to the console directly.** Workers share the real console host, so `Write-Host`, `Read-Host`, `Clear-Host`, `[Console]::...` or `$Host.UI` in worker code bypasses the console queue and scrambles Clean Console Output's live display. Worker output goes through `Write-Log`, which queues it for the main thread. -Cmdlets are fine in worker code. The code uses .NET calls in many of those places for historical reasons, not because a cmdlet would break. +Other cmdlets are fine in worker code. The code uses .NET calls in many of those places for historical reasons, not because a cmdlet would break. A runspace starts with only the functions it's given, so a function a worker calls must be in its caller's `-SharedFunctionNames` list (built from each module's `Get-...SharedFunctionNames`). `_Harness\Lint.Harness.ps1` checks all of this. @@ -489,14 +490,14 @@ A shared runspace-pool dispatcher. It runs a worker scriptblock once per job in | Function | What it does | |---|---| -| `Get-ZipJobScriptBlockKey` | Returns the first key in a job hashtable whose value is a scriptblock, or an array, list or hashtable directly holding one; `$null` if there is none. | +| `Get-ZipJobScriptBlockKey` | Returns the first key in a job hashtable whose value is, or holds, a scriptblock (up to four levels down, through arrays, lists, hashtables and `[pscustomobject]` properties); `$null` if there is none. | | `Invoke-ZipParallelProcessing` | Runs `-WorkerScript` for each job in a runspace pool of at most `-MaxParallel`, feeds every result to `-ResultHandler` on the main thread, and returns `@{ ForceStoppedCount = n }`. | **How it works** - Jobs come from exactly one of two sources: `-JobParams` (a fixed list of parameter hashtables) or `-NextJobProvider` (a scriptblock called on the main thread that returns the next hashtable, or `$null` when finished). Supplying both or neither throws. An empty `-JobParams` list returns immediately. - Each job's hashtable keys are passed to the worker as named parameters. The function adds three more: `CancelToken` (from `-Cts`), `LogMutex` and `ConsoleQueue`. A worker script must declare those parameters. -- Every job is checked before it starts: a scriptblock value, or one directly inside an array, list or hashtable value, throws a message naming the key (see the worker-runspace rules above). A `-JobParams` list is checked whole before anything runs; a `-NextJobProvider` job is checked as it is produced. To hand a worker code, pass it as a string and build it inside the worker with `[scriptblock]::Create()`. +- Every job is checked before it starts: a value that is, or holds, a scriptblock (up to four levels down, through arrays, lists, hashtables and `[pscustomobject]` properties) is refused with a message naming the key (see the worker-runspace rules above). A `-JobParams` list is checked whole before anything runs, so nothing starts. A refused `-NextJobProvider` job stops new jobs from starting; the jobs already running finish and report normally, and then the call throws. Each collection is read once per call, so one passed by reference to every job is checked with the first job, before workers are writing to it. To hand a worker code, pass it as a string and build it inside the worker with `[scriptblock]::Create()`. - The pool is built from a fresh `InitialSessionState`. Only the functions named in `-SharedFunctionNames` are copied into it (by reading each function's definition and adding a function entry). A name that doesn't resolve throws a message naming the entry. Nothing else from the calling script exists inside a worker. - Each run creates its own log mutex (`Initialize-LogMutex`) and a `ConcurrentQueue` for console output. The previous `$script:logMutex` is restored, and the new mutex disposed, when the call ends. - The main loop keeps the pool filled up to `-MaxParallel`, then on each pass: checks `Test-AbortSignal`, flushes the console queue (`Flush-ConsoleQueue`), calls `-PollCallback` if given, and harvests completed jobs. It sleeps 100 ms only when it has nothing to start. From f760ad2e9421af50f050cf3e8e5de715e8bcafd8 Mon Sep 17 00:00:00 2001 From: Hal <22532613+HalRiveria@users.noreply.github.com> Date: Wed, 28 Oct 2026 04:11:53 -0400 Subject: [PATCH 3/3] Job guard: skip lazy enumerables, check pairs, fail loudly; Lint fixes Job guard (Invoke-ZipParallelProcessing): - Reads only materialized collections (ICollection, ICollection, IReadOnlyCollection). A lazy or consuming enumerable such as GetConsumingEnumerable() is no longer drained before the worker gets it. - Looks inside KeyValuePair and DictionaryEntry values, which were skipped along with every other value type. - Reads a [pscustomobject] shared across jobs once per call, like collections. - Lets a collection through only when it reports being modified. Any other failure to read a value now throws, naming the key. Lint: - One helper decides what counts as an assigned scriptblock literal. - Rule 7 treats a container as local only if every assignment to it creates a new one, so a store through an alias of a shared container or through a loop variable is flagged. - Rule 9 matches module-qualified names. The harness's enumeration test doubles are PowerShell classes, so the harness no longer compiles a type with Add-Type. The new harness and Lint assertions fail against the previous code. Co-Authored-By: Claude Opus 5.5 --- Modules/ZipParallelProcessing.ps1 | 50 +++++++++--- _Harness/Lint.Harness.ps1 | 92 ++++++++++++++-------- _Harness/ZipParallelProcessing.Harness.ps1 | 62 ++++++++++++--- module-descriptions.md | 4 +- 4 files changed, 155 insertions(+), 53 deletions(-) diff --git a/Modules/ZipParallelProcessing.ps1 b/Modules/ZipParallelProcessing.ps1 index 14f3ef7..7f3e6c6 100644 --- a/Modules/ZipParallelProcessing.ps1 +++ b/Modules/ZipParallelProcessing.ps1 @@ -1,23 +1,46 @@ # See module-descriptions.md for this module's full description. +function Test-ZipJobValueIsMaterializedCollection { + param($Value) + if ($Value -is [System.Collections.ICollection]) { return $true } + foreach ($i in $Value.GetType().GetInterfaces()) { + if ($i.IsGenericType) { + $def = $i.GetGenericTypeDefinition() + if ($def -eq [System.Collections.Generic.ICollection`1] -or $def -eq [System.Collections.Generic.IReadOnlyCollection`1]) { return $true } + } + } + return $false +} + function Test-ZipJobValueHasScriptBlock { param($Value, [System.Collections.Generic.HashSet[object]]$Seen, [int]$Depth) if ($null -eq $Value) { return $false } if ($Value -is [scriptblock]) { return $true } - if ($Depth -ge 4 -or $Value -is [string] -or $Value -is [System.ValueType]) { return $false } - $isCustomObject = $Value -is [System.Management.Automation.PSCustomObject] - if (-not $isCustomObject -and $Value -isnot [System.Collections.IEnumerable]) { return $false } - if (-not $isCustomObject -and -not $Seen.Add($Value)) { return $false } + if ($Depth -ge 4 -or $Value -is [string]) { return $false } + $type = $Value.GetType() + $itemsAreProperties = $false + if ($Value -is [System.Collections.DictionaryEntry] -or + ($type.IsGenericType -and $type.GetGenericTypeDefinition() -eq [System.Collections.Generic.KeyValuePair`2])) { + $items = @($Value.Key, $Value.Value) + } elseif ($Value -is [System.Management.Automation.PSCustomObject]) { + if (-not $Seen.Add($Value)) { return $false } + $items = $Value.PSObject.Properties + $itemsAreProperties = $true + } elseif ($Value -is [System.Collections.IEnumerable] -and (Test-ZipJobValueIsMaterializedCollection -Value $Value)) { + if (-not $type.IsValueType -and -not $Seen.Add($Value)) { return $false } + if ($Value -is [System.Collections.IDictionary]) { $items = $Value.Values } else { $items = $Value } + } else { + return $false + } try { - if ($isCustomObject) { $items = foreach ($p in $Value.PSObject.Properties) { $p.Value } } - elseif ($Value -is [System.Collections.IDictionary]) { $items = $Value.Values } - else { $items = $Value } foreach ($item in $items) { + if ($itemsAreProperties) { $item = $item.Value } if (Test-ZipJobValueHasScriptBlock -Value $item -Seen $Seen -Depth ($Depth + 1)) { return $true } } - } catch { - # A collection something else is writing to right now can't be read - # safely; let it through unchecked rather than fail the run over it. + } catch [System.InvalidOperationException] { + # "Collection was modified": something else is writing to it right + # now, so it can't be read safely. Let it through unchecked rather + # than fail the run over it. } return $false } @@ -25,7 +48,12 @@ function Test-ZipJobValueHasScriptBlock { function Get-ZipJobScriptBlockKey { param([hashtable]$Job, [System.Collections.Generic.HashSet[object]]$Seen) foreach ($key in $Job.Keys) { - if (Test-ZipJobValueHasScriptBlock -Value $Job[$key] -Seen $Seen -Depth 0) { return [string]$key } + try { + $found = Test-ZipJobValueHasScriptBlock -Value $Job[$key] -Seen $Seen -Depth 0 + } catch { + throw ("Invoke-ZipParallelProcessing: couldn't check job parameter '" + $key + "' for scriptblocks - reading it failed: " + $_) + } + if ($found) { return [string]$key } } return $null } diff --git a/_Harness/Lint.Harness.ps1 b/_Harness/Lint.Harness.ps1 index aa634a5..d5b795a 100644 --- a/_Harness/Lint.Harness.ps1 +++ b/_Harness/Lint.Harness.ps1 @@ -361,24 +361,29 @@ function Get-MissingSharedFunctionNames { return ,@($missing | Select-Object -Unique) } +# The scriptblock literal an assignment's right-hand side holds - a bare +# { ... }, or { ... }.GetNewClosure(), which parses as an +# InvokeMemberExpressionAst wrapping it - or $null. The one place this is +# decided; Rule 5's worker-body lookup below and Rule 7 both call it. +function Get-AssignedScriptBlockLiteral { + param($Right) + if ($Right -isnot [System.Management.Automation.Language.CommandExpressionAst]) { return $null } + $expr = $Right.Expression + if ($expr -is [System.Management.Automation.Language.InvokeMemberExpressionAst] -and $expr.Member.Extent.Text -eq 'GetNewClosure') { $expr = $expr.Expression } + if ($expr -is [System.Management.Automation.Language.ScriptBlockExpressionAst]) { return $expr } + return $null +} + # Every $script:XXX = { ... } scriptblock assignment, so a -WorkerScript/ # -NextJobProvider $script:XXX argument can be resolved to its real body. -# Also unwraps $script:XXX = { ... }.GetNewClosure() - that shape parses -# as an InvokeMemberExpressionAst wrapping the ScriptBlockExpressionAst, -# not a bare ScriptBlockExpressionAst, and this codebase's own -# ResultHandler callbacks already use .GetNewClosure() elsewhere. $scriptBlockVars = @{} $scriptBlockVarFile = @{} foreach ($name in $fileText.Keys) { foreach ($assign in $fileAsts[$name].FindAll({ param($n) $n -is [System.Management.Automation.Language.AssignmentStatementAst] }, $true)) { - if ($assign.Right -is [System.Management.Automation.Language.CommandExpressionAst] -and - $assign.Left -is [System.Management.Automation.Language.VariableExpressionAst]) { - $rhsExpr = $assign.Right.Expression - if ($rhsExpr -is [System.Management.Automation.Language.InvokeMemberExpressionAst]) { - $rhsExpr = $rhsExpr.Expression - } - if ($rhsExpr -is [System.Management.Automation.Language.ScriptBlockExpressionAst]) { - $scriptBlockVars[$assign.Left.VariablePath.UserPath] = $rhsExpr.ScriptBlock + if ($assign.Left -is [System.Management.Automation.Language.VariableExpressionAst]) { + $literal = Get-AssignedScriptBlockLiteral -Right $assign.Right + if ($null -ne $literal) { + $scriptBlockVars[$assign.Left.VariablePath.UserPath] = $literal.ScriptBlock $scriptBlockVarFile[$assign.Left.VariablePath.UserPath] = $name } } @@ -572,10 +577,8 @@ function Get-ScriptBlockVariableNames { $names = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::OrdinalIgnoreCase) foreach ($assign in $AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.AssignmentStatementAst] }, $true)) { if ($assign.Left -is [System.Management.Automation.Language.VariableExpressionAst] -and - $assign.Right -is [System.Management.Automation.Language.CommandExpressionAst]) { - $rhsExpr = $assign.Right.Expression - if ($rhsExpr -is [System.Management.Automation.Language.InvokeMemberExpressionAst] -and $rhsExpr.Member.Extent.Text -eq 'GetNewClosure') { $rhsExpr = $rhsExpr.Expression } - if ($rhsExpr -is [System.Management.Automation.Language.ScriptBlockExpressionAst]) { $null = $names.Add($assign.Left.VariablePath.UserPath) } + $null -ne (Get-AssignedScriptBlockLiteral -Right $assign.Right)) { + $null = $names.Add($assign.Left.VariablePath.UserPath) } } foreach ($p in $AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.ParameterAst] }, $true)) { @@ -604,27 +607,47 @@ $scriptBlockArgFinder = { } } } - # Stores: $x[...] = / $x.Prop = , where $x - # isn't a variable this code assigns itself (a local container is fine). - $localNames = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::OrdinalIgnoreCase) - foreach ($assign in $AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.AssignmentStatementAst] }, $true)) { - if ($assign.Left -is [System.Management.Automation.Language.VariableExpressionAst]) { $null = $localNames.Add($assign.Left.VariablePath.UserPath) } + # Stores: $x[...] = / $x.Prop = , unless $x + # is a container this code built itself - every assignment to it creates + # a new one (@{}, @(), [Type]::new(), New-Object) and it isn't a + # parameter. An alias of something else ($c = $SharedCache), a parameter + # or a loop variable could be shared, so those count. + $assignments = @($AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.AssignmentStatementAst] }, $true)) + $freshNames = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::OrdinalIgnoreCase) + $otherNames = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::OrdinalIgnoreCase) + foreach ($p in $AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.ParameterAst] }, $true)) { $null = $otherNames.Add($p.Name.VariablePath.UserPath) } + foreach ($assign in $assignments) { + if ($assign.Left -isnot [System.Management.Automation.Language.VariableExpressionAst]) { continue } + $rhs = $assign.Right + $isFresh = $false + if ($rhs -is [System.Management.Automation.Language.CommandExpressionAst]) { + $expr = $rhs.Expression + if ($expr -is [System.Management.Automation.Language.ConvertExpressionAst]) { $expr = $expr.Child } + $isFresh = ($expr -is [System.Management.Automation.Language.HashtableAst]) -or + ($expr -is [System.Management.Automation.Language.ArrayExpressionAst]) -or + ($expr -is [System.Management.Automation.Language.ArrayLiteralAst]) -or + ($expr -is [System.Management.Automation.Language.InvokeMemberExpressionAst] -and $expr.Static -and $expr.Member.Extent.Text -eq 'new') + } elseif ($rhs -is [System.Management.Automation.Language.PipelineAst] -and $rhs.PipelineElements.Count -eq 1 -and + $rhs.PipelineElements[0] -is [System.Management.Automation.Language.CommandAst] -and $rhs.PipelineElements[0].GetCommandName() -eq 'New-Object') { + $isFresh = $true + } + if ($isFresh) { $null = $freshNames.Add($assign.Left.VariablePath.UserPath) } else { $null = $otherNames.Add($assign.Left.VariablePath.UserPath) } } - foreach ($assign in $AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.AssignmentStatementAst] }, $true)) { + foreach ($assign in $assignments) { $target = $assign.Left if ($target -isnot [System.Management.Automation.Language.IndexExpressionAst] -and $target -isnot [System.Management.Automation.Language.MemberExpressionAst]) { continue } - if ($assign.Right -isnot [System.Management.Automation.Language.CommandExpressionAst]) { continue } - $value = $assign.Right.Expression - if ($value -is [System.Management.Automation.Language.InvokeMemberExpressionAst] -and $value.Member.Extent.Text -eq 'GetNewClosure') { $value = $value.Expression } - $isScriptBlock = ($value -is [System.Management.Automation.Language.ScriptBlockExpressionAst]) -or + $value = $null + if ($assign.Right -is [System.Management.Automation.Language.CommandExpressionAst]) { $value = $assign.Right.Expression } + $isScriptBlock = ($null -ne (Get-AssignedScriptBlockLiteral -Right $assign.Right)) -or ($value -is [System.Management.Automation.Language.VariableExpressionAst] -and $sbVarNames.Contains($value.VariablePath.UserPath)) if (-not $isScriptBlock) { continue } $root = $target while ($root -is [System.Management.Automation.Language.IndexExpressionAst] -or $root -is [System.Management.Automation.Language.MemberExpressionAst]) { if ($root -is [System.Management.Automation.Language.IndexExpressionAst]) { $root = $root.Target } else { $root = $root.Expression } } - if ($root -is [System.Management.Automation.Language.VariableExpressionAst] -and $localNames.Contains($root.VariablePath.UserPath)) { continue } + if ($root -is [System.Management.Automation.Language.VariableExpressionAst] -and + $freshNames.Contains($root.VariablePath.UserPath) -and -not $otherNames.Contains($root.VariablePath.UserPath)) { continue } if ($assign.Extent.StartScriptPosition.Line -match '#\s*lint:ignore') { continue } $found.Add(@{ Line = $assign.Extent.StartLineNumber; What = ("stores a scriptblock into " + $target.Extent.Text + ", which it didn't create") }) } @@ -648,6 +671,9 @@ function Sample-Worker { $local = @{} $local['k'] = $built $Cache['n'] = $Name + $alias = $Cache + $alias['a'] = $built + foreach ($slot in $Cache.Values) { $slot.Factory = $Factory } } '@ $rule7SampleAst = [System.Management.Automation.Language.Parser]::ParseInput($rule7SampleText, [ref]$null, [ref]$null) @@ -657,7 +683,8 @@ Test-Assert ($rule7SampleLines -contains 5) "Rule 7 flags a [scriptblock] parame Test-Assert ($rule7SampleLines -contains 6) "Rule 7 flags a .GetNewClosure()'d scriptblock variable stored through a .NET method (TryAdd)" Test-Assert ($rule7SampleLines -contains 11) "Rule 7 flags a scriptblock stored through an indexer on a container it didn't create (`$Cache['k'] = `$Factory)" Test-Assert ($rule7SampleLines -contains 12) "Rule 7 flags a scriptblock literal stored into a property of a container it didn't create (`$Cache.Hook = { }.GetNewClosure())" -Test-Assert ($rule7Sample.Count -eq 4) ("Rule 7 leaves a plain parameter, a scriptblock's .Invoke() result, .Where(), a lint:ignore'd line, a local container and a plain stored value alone (flagged " + $rule7Sample.Count + ", expected 4)") +Test-Assert (($rule7SampleLines -contains 17) -and ($rule7SampleLines -contains 18)) "Rule 7 flags a store through a local alias of a shared container, and through a loop variable" +Test-Assert ($rule7Sample.Count -eq 6) ("Rule 7 leaves a plain parameter, a scriptblock's .Invoke() result, .Where(), a lint:ignore'd line, a container it built itself and a plain stored value alone (flagged " + $rule7Sample.Count + ", expected 6)") $rule7 = Get-RunspaceReachableFindings -Finder $scriptBlockArgFinder Test-Assert (@($rule7).Count -eq 0) "no runspace-reachable function or worker body passes a scriptblock to .NET or stores one into a container it didn't create" @@ -761,8 +788,10 @@ $hostCmdletFinder = { $hits = [System.Collections.Generic.List[object]]::new() foreach ($c in $AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.CommandAst] }, $true)) { $cmdName = $c.GetCommandName() - if (-not $cmdName -or -not $hostCmdletByName.ContainsKey($cmdName)) { continue } - $hits.Add(@{ Ast = $c; Name = $hostCmdletByName[$cmdName]; What = ("calls host cmdlet " + $cmdName) }) + if (-not $cmdName) { continue } + $bareName = $cmdName -replace '^[^\\]+\\', '' # Microsoft.PowerShell.Utility\Write-Host -> Write-Host + if (-not $hostCmdletByName.ContainsKey($bareName)) { continue } + $hits.Add(@{ Ast = $c; Name = $hostCmdletByName[$bareName]; What = ("calls host cmdlet " + $cmdName) }) } foreach ($m in $AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.MemberExpressionAst] }, $true)) { if ($m.Static -and $m.Expression -is [System.Management.Automation.Language.TypeExpressionAst] -and @@ -789,12 +818,13 @@ function Sample-Worker { [Console]::WriteLine('z') $Host.UI.WriteLine('w') $width = $Host.Name + Microsoft.PowerShell.Utility\Write-Host 'q' } '@ $rule9SampleAst = [System.Management.Automation.Language.Parser]::ParseInput($rule9SampleText, [ref]$null, [ref]$null) $rule9Sample = & $hostCmdletFinder $rule9SampleAst 'Sample-Worker' $rule9SampleLines = @($rule9Sample | ForEach-Object { $_.Line } | Sort-Object) -Test-Assert (($rule9SampleLines -join ',') -eq '2,3,5,6,7') ("Rule 9 flags Write-Host, Read-Host, an alias (cls), [Console]:: and `$Host.UI, and leaves Write-Output and other `$Host members alone (flagged lines " + ($rule9SampleLines -join ',') + ", expected 2,3,5,6,7)") +Test-Assert (($rule9SampleLines -join ',') -eq '2,3,5,6,7,9') ("Rule 9 flags Write-Host, Read-Host, an alias (cls), [Console]::, `$Host.UI and a module-qualified name, and leaves Write-Output and other `$Host members alone (flagged lines " + ($rule9SampleLines -join ',') + ", expected 2,3,5,6,7,9)") $rule9 = Get-RunspaceReachableFindings -Finder $hostCmdletFinder Test-Assert (@($rule9).Count -eq 0) "no runspace-reachable function or worker body calls a host cmdlet" diff --git a/_Harness/ZipParallelProcessing.Harness.ps1 b/_Harness/ZipParallelProcessing.Harness.ps1 index 2e9eb49..7755a2d 100644 --- a/_Harness/ZipParallelProcessing.Harness.ps1 +++ b/_Harness/ZipParallelProcessing.Harness.ps1 @@ -581,21 +581,65 @@ Test-Assert ($null -ne $h6.Error -and $h6.Error -match "'Steps'") "a scriptblock $h7 = Invoke-ScenarioHJobs -Job (New-ScenarioHJob @{ Config = [pscustomobject]@{ Name = 'x'; OnDone = { 7 } } }) Test-Assert ($null -ne $h7.Error -and $h7.Error -match "'Config'") "a scriptblock held in a [pscustomobject] property is refused" -# A collection shared by reference across jobs is read once per run, not -# once per job: later jobs may run while workers are writing to it. -if (-not ([System.Management.Automation.PSTypeName]'ZppEnumerationCounter').Type) { - Add-Type -TypeDefinition @' -public class ZppEnumerationCounter : System.Collections.IEnumerable { - public int Enumerations; - public System.Collections.IEnumerator GetEnumerator() { Enumerations++; return new object[] { "a", "b" }.GetEnumerator(); } +# Test doubles as PowerShell classes (built in memory - no Add-Type compile). +class ZppCountingCollection : System.Collections.ICollection { + [int]$Enumerations + [int] get_Count() { return 2 } + [bool] get_IsSynchronized() { return $false } + [object] get_SyncRoot() { return $this } + [void] CopyTo([System.Array]$array, [int]$index) { } + [System.Collections.IEnumerator] GetEnumerator() { $this.Enumerations++; return @('a', 'b').GetEnumerator() } } -'@ +class ZppThrowingCollection : System.Collections.ICollection { + [bool]$AsModified + ZppThrowingCollection([bool]$asModified) { $this.AsModified = $asModified } + [int] get_Count() { return 1 } + [bool] get_IsSynchronized() { return $false } + [object] get_SyncRoot() { return $this } + [void] CopyTo([System.Array]$array, [int]$index) { } + [System.Collections.IEnumerator] GetEnumerator() { + if ($this.AsModified) { throw [System.InvalidOperationException]::new('Collection was modified; enumeration operation may not execute.') } + throw [System.UnauthorizedAccessException]::new('simulated read failure') + } } -$counterH = [ZppEnumerationCounter]::new() + +# A collection shared by reference across jobs is read once per run, not +# once per job: later jobs may run while workers are writing to it. +$counterH = [ZppCountingCollection]::new() $seenH = [System.Collections.Generic.HashSet[object]]::new() $keysH = @(@(foreach ($i in 1..3) { Get-ZipJobScriptBlockKey -Job @{ JobId = $i; Shared = $counterH } -Seen $seenH }) | Where-Object { $null -ne $_ }) Test-Assert ($keysH.Count -eq 0 -and $counterH.Enumerations -eq 1) ("a collection shared by three jobs is enumerated once, not per job (enumerated " + $counterH.Enumerations + " times)") +# Same for a shared [pscustomobject]: its properties are read once. +$probeH = @{ Reads = 0 } +$sharedObjH = [pscustomobject]@{ Name = 'shared' } +$sharedObjH | Add-Member -MemberType ScriptProperty -Name Probe -Value { $probeH.Reads++; 'x' }.GetNewClosure() +$seenH2 = [System.Collections.Generic.HashSet[object]]::new() +foreach ($i in 1..3) { $null = Get-ZipJobScriptBlockKey -Job @{ JobId = $i; State = $sharedObjH } -Seen $seenH2 } +Test-Assert ($probeH.Reads -eq 1) ("a [pscustomobject] shared by three jobs is read once, not per job (read " + $probeH.Reads + " times)") + +# A consuming enumerable is left alone - reading it would take the items +# the worker is meant to get. +$queueH = [System.Collections.Concurrent.BlockingCollection[object]]::new() +$queueH.Add('first'); $queueH.Add('second'); $queueH.CompleteAdding() +$null = Get-ZipJobScriptBlockKey -Job @{ JobId = 1; Work = $queueH.GetConsumingEnumerable() } -Seen ([System.Collections.Generic.HashSet[object]]::new()) +Test-Assert ($queueH.Count -eq 2) ("checking a job doesn't consume a GetConsumingEnumerable() value (" + $queueH.Count + " of 2 items left)") + +# Scriptblocks inside struct entries (KeyValuePair) are found. +$pairsH = [System.Collections.Generic.List[System.Collections.Generic.KeyValuePair[string,object]]]::new() +$pairsH.Add([System.Collections.Generic.KeyValuePair[string,object]]::new('plain', 1)) +$pairsH.Add([System.Collections.Generic.KeyValuePair[string,object]]::new('hook', { 10 })) +$kvpKeyH = Get-ZipJobScriptBlockKey -Job @{ JobId = 1; Pairs = $pairsH } -Seen ([System.Collections.Generic.HashSet[object]]::new()) +Test-Assert ($kvpKeyH -eq 'Pairs') ("a scriptblock in a KeyValuePair's value is found (got: " + $kvpKeyH + ")") + +# A collection being modified is let through; any other read failure is +# reported, naming the key, instead of silently passing. +$modifiedKeyH = Get-ZipJobScriptBlockKey -Job @{ JobId = 1; Busy = [ZppThrowingCollection]::new($true) } -Seen ([System.Collections.Generic.HashSet[object]]::new()) +Test-Assert ($null -eq $modifiedKeyH) "a collection that's being modified (InvalidOperationException) is let through" +$readErrH = $null +try { $null = Get-ZipJobScriptBlockKey -Job @{ JobId = 1; Broken = [ZppThrowingCollection]::new($false) } -Seen ([System.Collections.Generic.HashSet[object]]::new()) } catch { $readErrH = $_.ToString() } +Test-Assert ($null -ne $readErrH -and $readErrH -match "'Broken'" -and $readErrH -match 'simulated read failure') ("any other read failure is reported with the key and the cause (got: " + $readErrH + ")") + # A provider job refused mid-run lets the jobs already running finish # normally, then throws - it doesn't stop them or cancel the caller's token. $resultsH9 = [System.Collections.Generic.List[hashtable]]::new() diff --git a/module-descriptions.md b/module-descriptions.md index 13b0bc7..12d6865 100644 --- a/module-descriptions.md +++ b/module-descriptions.md @@ -490,14 +490,14 @@ A shared runspace-pool dispatcher. It runs a worker scriptblock once per job in | Function | What it does | |---|---| -| `Get-ZipJobScriptBlockKey` | Returns the first key in a job hashtable whose value is, or holds, a scriptblock (up to four levels down, through arrays, lists, hashtables and `[pscustomobject]` properties); `$null` if there is none. | +| `Get-ZipJobScriptBlockKey` | Returns the first key in a job hashtable whose value is, or holds, a scriptblock (up to four levels down, through collections, key/value pairs and `[pscustomobject]` properties); `$null` if there is none. | | `Invoke-ZipParallelProcessing` | Runs `-WorkerScript` for each job in a runspace pool of at most `-MaxParallel`, feeds every result to `-ResultHandler` on the main thread, and returns `@{ ForceStoppedCount = n }`. | **How it works** - Jobs come from exactly one of two sources: `-JobParams` (a fixed list of parameter hashtables) or `-NextJobProvider` (a scriptblock called on the main thread that returns the next hashtable, or `$null` when finished). Supplying both or neither throws. An empty `-JobParams` list returns immediately. - Each job's hashtable keys are passed to the worker as named parameters. The function adds three more: `CancelToken` (from `-Cts`), `LogMutex` and `ConsoleQueue`. A worker script must declare those parameters. -- Every job is checked before it starts: a value that is, or holds, a scriptblock (up to four levels down, through arrays, lists, hashtables and `[pscustomobject]` properties) is refused with a message naming the key (see the worker-runspace rules above). A `-JobParams` list is checked whole before anything runs, so nothing starts. A refused `-NextJobProvider` job stops new jobs from starting; the jobs already running finish and report normally, and then the call throws. Each collection is read once per call, so one passed by reference to every job is checked with the first job, before workers are writing to it. To hand a worker code, pass it as a string and build it inside the worker with `[scriptblock]::Create()`. +- Every job is checked before it starts: a value that is, or holds, a scriptblock (up to four levels down, through collections, key/value pairs and `[pscustomobject]` properties) is refused with a message naming the key (see the worker-runspace rules above). Only materialized collections are read (anything implementing `ICollection`, `ICollection` or `IReadOnlyCollection`); a lazy or consuming enumerable such as `GetConsumingEnumerable()` is passed through unread, since reading it would take the items the worker is meant to get. A `-JobParams` list is checked whole before anything runs, so nothing starts. A refused `-NextJobProvider` job stops new jobs from starting; the jobs already running finish and report normally, and then the call throws. Each collection or custom object is read once per call, so one passed by reference to every job is checked with the first job, before workers are writing to it. A collection that reports it is being modified is let through; any other failure to read a value throws, naming the key. To hand a worker code, pass it as a string and build it inside the worker with `[scriptblock]::Create()`. - The pool is built from a fresh `InitialSessionState`. Only the functions named in `-SharedFunctionNames` are copied into it (by reading each function's definition and adding a function entry). A name that doesn't resolve throws a message naming the entry. Nothing else from the calling script exists inside a worker. - Each run creates its own log mutex (`Initialize-LogMutex`) and a `ConcurrentQueue` for console output. The previous `$script:logMutex` is restored, and the new mutex disposed, when the call ends. - The main loop keeps the pool filled up to `-MaxParallel`, then on each pass: checks `Test-AbortSignal`, flushes the console queue (`Flush-ConsoleQueue`), calls `-PollCallback` if given, and harvests completed jobs. It sleeps 100 ms only when it has nothing to start.