Add lock and null check to remoting internals (#16542) by SergeyZalyadeev · Pull Request #16683 · PowerShell/PowerShell

SergeyZalyadeev · 2021-12-30T21:43:54Z

PR Summary

Lock and null check of the PrioritySendDataCollection.cs members to avoid race condition.

PR Context

Copy-Item cmdlet can throw unhandled exception that terminates the process.
The issue occures in WSManTransportManager internals.
The PrioritySendDataCollection class is used by 2 threads: first thread calls ReadOrRegisterCallback method, that gets _dataToBeSent field, when second thread calls Clear method, where _dataToBeSent member is disposed.

PR Checklist

ghost · 2021-12-30T21:44:06Z

All CLA requirements met.

PaulHigin · 2022-01-03T19:26:38Z

I am not comfortable adding extra locks here and possibly introducing deadlocks. This code has remained untouched for many years. We need to better understand of the race condition, @SergeyZalyadeev please add scenario information. The SerializedDataStreams already handle the dispose condition, and this may need to be extended for this particular scenario.

SergeyZalyadeev · 2022-01-10T09:19:54Z

@PaulHigin please take a look on the issue #16542
I can reproduce by using the following script

$User='localhost\Admin'
$password='Passw0rd1'

$attempts = 1000
$vm_ip_list = @('10.1.0.10', '10.1.0.11', '10.1.0.12', '10.1.0.13', '10.1.0.14', '10.1.0.15')

while(($attempts--)) {
	Write-Host "Attempt: $attempts"
	$vm_ip_list | foreach-object -Parallel { $cr = [pscredential]::new($User, (ConvertTo-SecureString $password -AsPlainText -Force));

		$sess = New-PSSession -ComputerName $_ -Credential $cr;
		Copy-Item -Path C:\Users\public\Downloads\50Mb.zip -Destination C:\50Mb.zip -ToSession $sess
}}

Dumps
https://pwshdumps.blob.core.windows.net/pub/pwsh.exe_220110_104242.zip
https://pwshdumps.blob.core.windows.net/pub/pwsh.exe_220109_223435.zip
https://pwshdumps.blob.core.windows.net/pub/pwsh721.exe_220110_131901.zip

PaulHigin

@SergeyZalyadeev Thanks for the scenario information and crash dumps. I agree with your analysis and fix, and have just a few minor comments. I would like to get this change in to 7.3 preview asap, after you make these minor fixes, so that the change can be well tested.

src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs

PaulHigin

LGTM

PaulHigin · 2022-01-11T20:27:38Z

@daxian-dbw Can you please review? I would like to get a second set of eyes on this change.

src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs

pull-request-quantifier-deprecated · 2022-01-13T10:32:15Z

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

Quantification details

Label      : Extra Small
Size       : +8 -4
Percentile : 4.8%

Total files changed: 1

Change summary by file extension:
.cs : +8 -4

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

Why proper sizing of changes matters

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

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

What can I do to optimize my changes

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

How to interpret the change counts in git diff output

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

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

src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs

iSazonov · 2022-01-14T04:11:06Z

@SergeyZalyadeev Thanks for your contribution!

floh96 · 2022-01-14T08:58:28Z

@PaulHigin this was merged without the cla signed

PaulHigin · 2022-01-14T16:30:30Z

@SergeyZalyadeev Can you please sign the CLA?

https://cla.opensource.microsoft.com/PowerShell/PowerShell?pullRequest=16683

Contributor License Agreement

…erShell#16683) * fix crash Copy-Item to remote session (PowerShell#16542) * update comments * remove lock (PowerShell#16542) Co-authored-by: Sergey Zalyadeev <sergey.zalyadeev@cayosoft.com>

ghost · 2022-02-24T23:33:37Z

🎉v7.3.0-preview.2 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

…erShell#16683) * fix crash Copy-Item to remote session (PowerShell#16542) * update comments * remove lock (PowerShell#16542) Co-authored-by: Sergey Zalyadeev <sergey.zalyadeev@cayosoft.com>

fix crash Copy-Item to remote session (PowerShell#16542)

35aaecf

SergeyZalyadeev requested a review from PaulHigin as a code owner December 30, 2021 21:43

ghost assigned anmenaga Dec 30, 2021

pull-request-quantifier-deprecated bot added the Extra Small label Dec 30, 2021

PaulHigin added the WG-Remoting PSRP issues with any transport layer label Jan 3, 2022

PaulHigin suggested changes Jan 11, 2022

View reviewed changes

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jan 11, 2022

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jan 11, 2022

update comments

6b6a69b

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jan 11, 2022

PaulHigin approved these changes Jan 11, 2022

View reviewed changes

PaulHigin assigned PaulHigin and unassigned anmenaga Jan 11, 2022

PaulHigin requested a review from daxian-dbw January 11, 2022 20:27

PaulHigin suggested changes Jan 11, 2022

View reviewed changes

src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs Outdated Show resolved Hide resolved

ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Jan 11, 2022

remove lock (PowerShell#16542)

bb6be38

PaulHigin approved these changes Jan 13, 2022

View reviewed changes

daxian-dbw reviewed Jan 13, 2022

View reviewed changes

src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs Show resolved Hide resolved

PaulHigin merged commit da7c52a into PowerShell:master Jan 13, 2022

Conversation

SergeyZalyadeev commented Dec 30, 2021

PR Summary

PR Context

PR Checklist

Uh oh!

ghost commented Dec 30, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PaulHigin commented Jan 3, 2022

Uh oh!

SergeyZalyadeev commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

PaulHigin commented Jan 11, 2022

Uh oh!

Uh oh!

pull-request-quantifier-deprecated bot commented Jan 13, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

Uh oh!

iSazonov commented Jan 14, 2022

Uh oh!

floh96 commented Jan 14, 2022

Uh oh!

PaulHigin commented Jan 14, 2022 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Feb 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ghost commented Dec 30, 2021 •

edited by ghost

Loading

SergeyZalyadeev commented Jan 10, 2022 •

edited

Loading

PaulHigin commented Jan 14, 2022 •

edited by unfurl-links bot

Loading