Skip to content

[Az.Accounts] Use mutex to prevent cross process file I/O #12281

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.

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
merged 4 commits into from
Jun 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions src/Accounts/Accounts/ChangeLog.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
-->
## Upcoming Release

* Fixed an issue that may cause authentication errors in multi-process scenarios such as running multiple Azure PowerShell cmdlets using `Start-Job` [#9448]

## Version 1.9.0
* Supported discovering environment setting by default and adding environment via `Add-AzEnvironment`
* Update preloaded assemblies [#12024], [#11976]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,18 @@ public abstract class ProtectedFileProvider : IFileProvider, IDisposable
public const int MaxTries = 30;
static readonly TimeSpan RetryInterval = TimeSpan.FromMilliseconds(500);
protected Stream _stream;
object _initializationLock = new object();

/// <summary>
/// Use a Mutex to prevent cross-process file I/O
/// </summary>
/// <returns></returns>
private static readonly Mutex _initializationLock = new Mutex(false, @"Local\AzurePowerShellProtectedFileProviderInit");
public string FilePath { get; set; }

protected IDataStore DataStore { get; set; }

/// <summary>
///
///
/// </summary>
public Stream Stream
{
Expand Down Expand Up @@ -87,7 +92,8 @@ public StreamWriter CreateWriter()

protected virtual void InitializeStream()
{
lock (_initializationLock)
_initializationLock.WaitOne();
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe we should use Mutex here, because TryGetStreamLock() may open files for write exclusively, see https://github.com/Azure/azure-powershell-common/blob/master/src/Authentication.Abstractions/DiskDataStore.cs#L324

try
{
if (_stream == null)
{
Expand All @@ -96,10 +102,13 @@ protected virtual void InitializeStream()
{
throw new UnauthorizedAccessException(string.Format(Resources.FileLockFailure, FilePath));
}

_stream = stream;
}
}
finally
{
_initializationLock.ReleaseMutex();
Copy link
Member

Choose a reason for hiding this comment

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

how about put _initializationLock.ReleaseMutex() in finally block?

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. Done

}
}

protected abstract Stream AcquireLock(string filePath);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,14 @@
// limitations under the License.
// ----------------------------------------------------------------------------------

using Hyak.Common;
using Microsoft.Azure.Commands.Common.Authentication.Abstractions;
using Microsoft.Azure.Commands.Common.Authentication.Properties;
using Microsoft.IdentityModel.Clients.ActiveDirectory;
using System;
using System.IO;
using System.Security.Cryptography;
using System.Threading;

#if NETSTANDARD
namespace Microsoft.Azure.Commands.Common.Authentication.Core
Expand All @@ -41,7 +43,10 @@ public class ProtectedFileTokenCache : TokenCache, IAzureTokenCache
#endif
"TokenCache.dat");

private static readonly object fileLock = new object();
/// <summary>
/// A mutex to prevent IO to token cache file across threads / processes.
/// </summary>
private static readonly Mutex fileLock = new Mutex(false, @"Local\AzurePowerShellAdalTokenCacheFile");

private static readonly Lazy<ProtectedFileTokenCache> instance = new Lazy<ProtectedFileTokenCache>(() => new ProtectedFileTokenCache());

Expand Down Expand Up @@ -123,12 +128,13 @@ void EnsureStateSaved()

private void ReadFileIntoCache(string cacheFileName = null)
{
if(cacheFileName == null)
if (cacheFileName == null)
{
cacheFileName = ProtectedFileTokenCache.CacheFileName;
}

lock (fileLock)
fileLock.WaitOne();
try
{
if (_store.FileExists(cacheFileName))
{
Expand All @@ -150,11 +156,15 @@ private void ReadFileIntoCache(string cacheFileName = null)
}
}
}
finally
{
fileLock.ReleaseMutex();
}
}

private void WriteCacheIntoFile(string cacheFileName = null)
{
if(cacheFileName == null)
if (cacheFileName == null)
{
cacheFileName = ProtectedFileTokenCache.CacheFileName;
}
Expand All @@ -165,19 +175,25 @@ private void WriteCacheIntoFile(string cacheFileName = null)
var dataToWrite = Serialize();
#endif

lock(fileLock)
fileLock.WaitOne();
try
{
if (HasStateChanged)
{
_store.WriteFile(cacheFileName, dataToWrite);
HasStateChanged = false;
HasStateChanged = false;
}
}
finally
{
fileLock.ReleaseMutex();
}
}

private void EnsureCacheFile(string cacheFileName = null)
{
lock (fileLock)
fileLock.WaitOne();
try
{
if (_store.FileExists(cacheFileName))
{
Expand Down Expand Up @@ -207,6 +223,10 @@ private void EnsureCacheFile(string cacheFileName = null)
#endif
_store.WriteFile(cacheFileName, dataToWrite);
}
finally
{
fileLock.ReleaseMutex();
}
}
}
}
9 changes: 7 additions & 2 deletions src/Accounts/Authentication/AzureSessionInitializer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#if NETSTANDARD
using Microsoft.Azure.Commands.Common.Authentication.Core;
#endif
using Hyak.Common;

namespace Microsoft.Azure.Commands.Common.Authentication
{
Expand Down Expand Up @@ -73,8 +74,10 @@ static IAzureTokenCache InitializeTokenCache(IDataStore store, string cacheDirec
var cachePath = Path.Combine(cacheDirectory, cacheFile);
result = new ProtectedFileTokenCache(cachePath, store);
}
catch
catch (Exception ex)
{
TracingAdapter.Information("[AzureSessionInitializer]: Cannot initialize token cache in 'CurrentUser' mode. Falling back to 'Process' mode.");
TracingAdapter.Information($"[AzureSessionInitializer]: Message: {ex.Message}; Stacktrace: {ex.StackTrace}");
}
}

Expand Down Expand Up @@ -159,10 +162,12 @@ static ContextAutosaveSettings InitializeSessionSettings(IDataStore store, strin
store.WriteFile(autoSavePath, JsonConvert.SerializeObject(result));
}
}
catch
catch (Exception ex)
{
// ignore exceptions in reading settings from disk
result.Mode = ContextSaveMode.Process;
TracingAdapter.Information("[AzureSessionInitializer]: Cannot read settings from disk. Falling back to 'Process' mode.");
TracingAdapter.Information($"[AzureSessionInitializer]: Message: {ex.Message}; Stacktrace: {ex.StackTrace}");
}

return result;
Expand Down