Fix race condition to access powershell.config.json #8249

iSazonov · 2018-11-13T15:32:04Z

PR Summary

(Also there can be a race condition if some processes access the config file. I don't now how to fix.)

PR Checklist

anmenaga · 2018-11-13T21:54:56Z

Restarted builds because of "Get-PSGalleryApiAvailability : PowerShell Gallery is currently unavailable. Please try again later."

rjmholt

LGTM.

To prevent other processes from accessing, is it possible to take out an OS-level write lock on the json file? After searching for a bit on the web, it seems like new FileStream() with the right arguments already does this, no?

src/System.Management.Automation/engine/PSConfiguration.cs

iSazonov · 2018-11-14T04:16:56Z

@rjmholt A problem I indicated is that we don't process an exception if file is locked for read. Scenario: run two pwsh-s, one lock the config file, second is trying to access the file and - what? Wait and stop whole process? Currently we'll crash. On a system where many PowerShell processes run, this can cause unpredictable (rarely) crashes. (At minimum, we could not put an exclusive write lock.)

rjmholt · 2018-11-14T06:37:50Z

Hmmmm. Well given that the config file is changed at runtime by Set-ExecutionPolicy (and possibly others), maybe it should crash (more gracefully) -- given that config files may be changed by the PowerShell session, maybe they are session specific?

Otherwise we are forced to use some OS-level lock to get an atomic interleaving of PowerShell config writes?

iSazonov · 2018-11-14T07:13:33Z

I added a simple file unlock wait. I think it will resolve the inter process race condition in most of cases. Of cause an admin user can still exclusively open the file with read lock and crash pwsh processes. Seems it is an edge case.

iSazonov · 2018-11-14T09:35:41Z

MSFT team please review new commit.

iSazonov · 2018-11-15T04:30:53Z

@anmenaga @TravisEz13 @SteveL-MSFT @adityapatwardhan @daxian-dbw @rjmholt
Please review last commit. The PR block #8237 and my follow work.

anmenaga · 2018-11-15T18:58:20Z

I'm not a fan of this WaitForFile addition (and looped wait timers in general unless they are absolutely necessary).
Original problem was intermittently sharing read access to the file between PS processes. FileShare.ReadWrite is good enough for this. I would remove WaitForFile addition (especially considering that it only partially addresses the issue that it's trying to address).

rjmholt · 2018-11-15T19:08:41Z

Yes, sorry, only meant to discuss rather than request changes (couldn't find a good issue to discuss in)

iSazonov · 2018-11-16T03:39:44Z

Original problem was intermittently sharing read access to the file between PS processes.

Original problem is a bug that is in-process race condition. It is hosting scenarios. Fixed by first commit.
Second commit is for inter-process race condition. It is scenarios with many powershell processes (like background scheduled tasks). Fixed by second commit.

I think both commits is important because pwsh crashed.

looped wait timers in general unless they are absolutely necessary

The solution looks good for me because it is very rare race condition and we will never block a process for long especially since we cache powershell.config.json.

rjmholt · 2018-11-16T04:11:14Z

The looped wait timer approach I think looks good in comparison to the alternative: a corrupt powershell.config.json which ideally leads to a crash, but otherwise leads to one of the PowerShell processes incorrectly picking up a new configuration.

I get the impression though that perhaps a shared mutable configuration between processes is something we should reconsider as an idea?

iSazonov · 2018-11-16T04:22:50Z

I get the impression though that perhaps a shared mutable configuration between processes is something we should reconsider as an idea?

This could be addressed by PowerShell/PowerShell-RFC#111 too.

adityapatwardhan · 2018-11-21T00:57:10Z

@daxian-dbw Can you have a look at this?

iSazonov · 2018-11-22T04:30:19Z

@adityapatwardhan @daxian-dbw The PR block sequence of other PRs. Could you please continue?

daxian-dbw

Look mostly good to me. Please address the 3 comments.

daxian-dbw · 2018-11-23T06:55:52Z

src/System.Management.Automation/engine/PSConfiguration.cs

-                using (var streamReader = new StreamReader(fileName))
+                // The config file can be locked by another process
+                // so we wait some milliseconds in 'WaitForFile()' for recovery before stop current process.
+                using (var readerStream = WaitForFile(fileName, FileMode.Open, FileAccess.Read, FileShare.ReadWrite))


By having FileShare.ReadWrite, we allow the file to be opened for writing by another process. FileShare.ReadWrite could only possibly work in a logging scenario, where new content is always appended to the end of a file and you read the file through seek. I think it should be changed to FileShare.Read.

With FileShare.Read the PowerShell Core process will terminate if anybody open the file for writing. We aim to avoid this and open the file with minimal lock level. Also we don't keep the file opened: every time we create new StreamReader on very short time. There still can be a race condition if we read partially changed file but it is another issue - what PowerShell Core should do if the config file is broken.

IMO, this permission raises more confusion than the potential race condition it might mitigate. I think the right approach would be to return a default setting when the file cannot be opened after the retry, or when the file is corrupted (schema validation would be good but that would impact the startup time).

The current design is to only read the particular setting that is requested, which will result in opening the files multiple times but actually reading the whole file every time. I don't think this is the right thing to do. We probably should be very explicit that the setting file is read only one time at startup, and we process all settings that is present in the file. If the file cannot be opened after retry or the content is corrupted, we use the default settings and somehow convey the error to user (yes, we need to define the default settings first). In this way, the file is read only once, which means less race conditions, and less file access cost. Anyways, this should be a separate PR, not a change request for this PR. Here we only focus on the share permission.

Should I change anything in the PR?

IMO, this permission raises more confusion than the potential race condition it might mitigate. I think the right approach would be to return a default setting when the file cannot be opened after the retry, or when the file is corrupted (schema validation would be good but that would impact the startup time).

Yes, I think changing FileShare.ReadWrite to FileShare.Read :)

I am not sure that this resolve the race condition in our xUnit tests - I started the PR to get this.

Your xUnit tests actively modify the config file (write lock?) and this lead to crash other tests .
I agree that here we can only temporary resolve the problem and we should work on better solution. (Address in my RFC or make PR?)

@daxian-dbw returning a default settings file could result in security settings in the config file being bypassed.

Your xUnit tests actively modify the config file (write lock?) and this lead to crash other tests .

@iSazonov that's unfortunate to hear :( Then let's go with FileShare.ReadWrite for now.

@TravisEz13 I agree. It sounds to me crashing pwsh when config file parsing fails is more securer :)

@daxian-dbw @TravisEz13 Could you please add more feedback about "security settings" to my RFC? We could discuss this there. I don't like the "crashing pwsh" because it could be a DoS attack even by admin user.

src/System.Management.Automation/engine/PSConfiguration.cs

iSazonov · 2018-11-27T03:14:17Z

@adityapatwardhan Please merge the PR.

adityapatwardhan · 2018-11-27T18:09:23Z

@iSazonov Can you fix the two CodeFactor issues? Then the PR can be merged.

iSazonov · 2018-11-28T03:55:23Z

@adityapatwardhan CodeFactor issues was fixed.

Remove unneeded dotnet restore Add --no-restore Add extraParams for MacOS Fix TestRunspaceWithPowerShellAndInitialSessionState Fix race condition to access powershell.config.json Reduce an impact of the inter process race condition Fix race condition to access powershell.config.json (PowerShell#8249)

iSazonov requested review from SteveL-MSFT, TravisEz13, adityapatwardhan and daxian-dbw November 13, 2018 15:32

iSazonov requested a review from BrucePay as a code owner November 13, 2018 15:32

anmenaga approved these changes Nov 13, 2018

View reviewed changes

SteveL-MSFT approved these changes Nov 13, 2018

View reviewed changes

TravisEz13 approved these changes Nov 14, 2018

View reviewed changes

TravisEz13 added WG-Engine core PowerShell engine, interpreter, and runtime CL-Engine Indicates that a PR should be marked as an engine change in the Change Log labels Nov 14, 2018

TravisEz13 assigned adityapatwardhan Nov 14, 2018

rjmholt approved these changes Nov 14, 2018

View reviewed changes

src/System.Management.Automation/engine/PSConfiguration.cs Outdated Show resolved Hide resolved

Fix race condition to access powershell.config.json

a70f6b6

iSazonov force-pushed the fix-race-cond branch from e2b1fbb to a70f6b6 Compare November 14, 2018 05:24

Use var

f79a4f5

Reduce an impact of the inter process race condition

3f4e604

iSazonov mentioned this pull request Nov 14, 2018

Move xUnit tests to new folder #8237

Closed

11 tasks

TravisEz13 approved these changes Nov 16, 2018

View reviewed changes

daxian-dbw reviewed Nov 23, 2018

View reviewed changes

Add private modifier

e5462e7

daxian-dbw approved these changes Nov 26, 2018

View reviewed changes

Fix CodeFactor issues

3556792

adityapatwardhan merged commit 9381d03 into PowerShell:master Nov 28, 2018

iSazonov deleted the fix-race-cond branch November 29, 2018 03:06

daxian-dbw mentioned this pull request Feb 20, 2019

Reduce race condition probability for config file access #8779

Merged

11 tasks

Fix race condition to access powershell.config.json #8249

Fix race condition to access powershell.config.json #8249

Uh oh!

Conversation

iSazonov commented Nov 13, 2018 • edited by TravisEz13 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

anmenaga commented Nov 13, 2018

Uh oh!

rjmholt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iSazonov commented Nov 14, 2018

Uh oh!

rjmholt commented Nov 14, 2018

Uh oh!

iSazonov commented Nov 14, 2018

Uh oh!

iSazonov commented Nov 14, 2018

Uh oh!

iSazonov commented Nov 15, 2018

Uh oh!

anmenaga commented Nov 15, 2018

Uh oh!

rjmholt commented Nov 15, 2018

Uh oh!

iSazonov commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjmholt commented Nov 16, 2018

Uh oh!

iSazonov commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adityapatwardhan commented Nov 21, 2018

Uh oh!

iSazonov commented Nov 22, 2018

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Nov 23, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov Nov 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

TravisEz13 Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov Nov 27, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iSazonov commented Nov 27, 2018

iSazonov commented Nov 13, 2018 •

edited by TravisEz13

Loading

iSazonov commented Nov 16, 2018 •

edited

Loading

iSazonov commented Nov 16, 2018 •

edited

Loading

iSazonov Nov 23, 2018 •

edited

Loading

daxian-dbw Nov 26, 2018 •

edited

Loading

daxian-dbw Nov 26, 2018 •

edited

Loading