Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adityapatwardhan merged 5 commits into from
Nov 28, 2018
Merged

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

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a70f6b6
Fix race condition to access powershell.config.json
iSazonov Nov 13, 2018
f79a4f5
Use var
iSazonov Nov 14, 2018
3f4e604
Reduce an impact of the inter process race condition
iSazonov Nov 14, 2018
e5462e7
Add private modifier
iSazonov Nov 23, 2018
3556792
Fix CodeFactor issues
iSazonov Nov 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion src/System.Management.Automation/engine/PSConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,10 @@ internal PSKeyword GetLogKeywords()
fileLock.EnterReadLock();
try
{
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))
Copy link
Member

@daxian-dbw daxian-dbw Nov 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@iSazonov iSazonov Nov 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@daxian-dbw daxian-dbw Nov 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@iSazonov iSazonov Nov 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I change anything in the PR?

Copy link
Member

@daxian-dbw daxian-dbw Nov 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Collaborator Author

@iSazonov iSazonov Nov 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@iSazonov iSazonov Nov 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Member

@TravisEz13 TravisEz13 Nov 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@daxian-dbw daxian-dbw Nov 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Collaborator Author

@iSazonov iSazonov Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

using (var streamReader = new StreamReader(readerStream))
using (var jsonReader = new JsonTextReader(streamReader))
{
var settings = new JsonSerializerSettings() { TypeNameHandling = TypeNameHandling.None, MaxDepth = 10 };
Expand All @@ -376,6 +379,29 @@ internal PSKeyword GetLogKeywords()
return defaultValue;
}

private FileStream WaitForFile(string fullPath, FileMode mode, FileAccess access, FileShare share)
{
const int MaxTries = 5;
for (int numTries = 0; numTries < MaxTries; numTries++)
{
try
{
return new FileStream(fullPath, mode, access, share);
}
catch (IOException)
{
if (numTries == (MaxTries - 1))
{
throw;
}

Thread.Sleep(50);
}
}

throw new IOException(nameof(WaitForFile));
}

/// <summary>
/// Update a value in the configuration file.
/// </summary>
Expand Down