@iSazonov Could you re-add the backport consider labels? Or it does not matter.

@kborowinski Thanks for your contribution!

@PowerShell/powershell-maintainers triage decision:
For 7.4, I don't think a fix that doesn't affect the actual script results makes the bar for an LTS. The regression risk would be higher than the potential gains here.

@TravisEz13

I understand the decision not to backport this PR to the LTS version, but I would like to clarify that the actual script resultsare affected inPS 7.4.X and up.

Ifirst reported this issue on the7Zip4PowerShell repo, where the unpacking process was crashing mid-task.
I proposed a PR that wasmerged to implement a workaround that swallows NREs coming from progress pane:

try{worker.Progress.StatusDescription="Finished";worker.Progress.RecordType=ProgressRecordType.Completed;WriteProgress(worker.Progress);}catch(NullReferenceException){// Possible bug in PowerShell 7.4.0 leading to a null reference exception being thrown on ProgressPane completion// This is not happening on PowerShell 5.1}

This issue affects all scripts that write to the console from multiple threads, leading to potential crashes in the LTS version of PowerShell. Affected users are forced to disable the progress bar either globally or per script.

@kborowinski I had the bot open PRs for the backports and added your comments. These still need to be triaged, but we are moving over to a new process that helps us track all the information we need.

1. @kborowinski It would be good if you know exactly when it was introduced, that will help us with our decision.

@TravisEz13 In comment on the original issue@iSazonovpinpointed it to PR#13758PowerShell 7.2.0-preview.2 and#15864PowerShell 7.2.0-preview.9 whenPSAnsiRendering was made stable.

Root Cause

I debugged the code and found that the race condition was caused byConsoleHostUserInterface.PreWrite calls_progPane?.Hide() without locking the_instanceLock.

The NRE happens because_savedRegion gets set to null while theHide() method is running. However,_savedRegion is set to null only in theHide() method, and that means at least twoHide() are running in parallel, which is unexpected.

Looking closely,Hide() has 5 call sites, and 3 of them are guarded bylock (_instanceLock). The other 2 un-guarded call sites areConsoleHostUserInterface.PreWrite() andConsoleHostUserInterface.PreRead(). ThePreWrite() method gets called inWriteToCosnole method, which is the most low-level method called by allWrite-* commands, includingWrite-Verbose.

The root cause becomes clear give the above -- callingWrite-Verbose in the middle of on-goingWrite-Progress potentially causes theHide() method to be called in parallel, which corrupts the state of theProgressPane.

So, the potential problem still exists inCosnoleHostUserInterface.PreRead(),ConsoleHostUserInterface.PostWrite(), andConsoleHostUserInterface.PostRead(). The latter 2 call_progPane?.Show() without locking_instanceLock.

Thoughts on this fix

The safest and complete fix would be addinglock (_instanceLock) toPreWrite(),PreRead(),PostWrite(), andPostRead(), so as to make all calls toHide andShow guarded by the same lock.

However, after discussing with@SeeminglyScience and thinking about this more, we decide to not make further changes to the original fix. Here is why:

1. By looking into the code more closely, it seems allWrite-* actions (including$host.UI.Write*) that can be triggered by a user go throughWriteImpl, which has been guarded by the original fix. Other calls toWriteToCosnole outside ofWriteImpl are for reading input, prompting user with choices, and etc., not related to aWrite-* action from user. So, it feels to me that guardWriteImpl is sufficient for the cases where a race condition could happen.

2. Comparatively, even though adding locks toPreWrite andPostWrite would be a safer and more complete fix, it's not really needed practically in cases whereWriteToConsole is called because of reading input or prompting users. Furthermore, it will result in acquiring locks twice in theWriteToConsole method, which will make it less performant in general.

3. RegardingPreRead andPostRead, they are used only when reading input, and it's unlikely to happen while progress rendering is ongoing. So, again, even though adding locks there would be a complete fix, it seems unnecessary practically and will degrade the performance for sure.

The safest and complete fix would be addinglock (_instanceLock) toPreWrite(),PreRead(),PostWrite(), andPostRead(), so as to make call calls toHide andShow guarded by the same lock.

@daxian-dbw Thank you for your investigations! The reason I didn't dare to do this was for fear of a deadlock. There are too many combinations. Even a change in one place in the future could potentially open up scenarios impossible today.
Steve and I had a short exchange about this a few years ago with the conclusion to wait for feedback. Now that such report has been received, we have corrected the specific scenario. If there are more complex scenarios with problems, then it may be more correct not to saturate the code with locks, but to change the approach more fundamentally.

@SeeminglyScience did look into the feasibility to make the progress rendering lock-less, using atomic primitives. But it turned out to be a too big effort and too low priority for the team to commit on.