From 996ac53f4c9de2b26b6672de4941a3f5d8477551 Mon Sep 17 00:00:00 2001 From: Dan Travison Date: Mon, 4 Dec 2017 13:47:15 -0800 Subject: [PATCH 1/4] Issue https://github.com/PowerShell/PowerShell/issues/4029 exposed two problems when failing to load libpsrpclient. WSManAPIDataCommon.ctor was not identifying the binary that failed DLLImport and PrioritySendDataCollection.Clear() would throw a NullReferenceException when called from the finalizer after an error path. This change addresses both of the above but does not address the DllImport failure. * Guard against null arrays in PrioritySendDataCollection.Clear() - called from finalizer in error paths. * Diagnosability: Log DllNotFoundException in WSManAPIDataCommon ctor and also include as the internal exception when throwing PSRemotingTransportException. --- .../remoting/fanin/PriorityCollection.cs | 33 ++++++++++++++----- .../remoting/fanin/WSManTransportManager.cs | 16 +++++++-- 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs b/src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs index 4e2ed03a9af..58fbd9d6782 100644 --- a/src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs +++ b/src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs @@ -49,6 +49,8 @@ internal class PrioritySendDataCollection // fragmentor used to serialize & fragment objects added to this collection. private Fragmentor _fragmentor; private object[] _syncObjects; + // synchronize access to _syncObjects and _dataToBeSent as a set. + private object _syncObject = new Object(); // callbacks used if no data is available at any time. // these callbacks are used to notify when data becomes available under @@ -154,15 +156,30 @@ internal void Add(RemoteDataObject data) /// internal void Clear() { - Dbg.Assert(null != _dataToBeSent, "Serialized streams are not initialized"); - lock (_syncObjects[(int)DataPriorityType.PromptResponse]) + /* Lock _syncObjects and _dataToBeSent as a set to ensure atomic cleanup. */ + lock (_syncObject) { - _dataToBeSent[(int)DataPriorityType.PromptResponse].Dispose(); - } + /* + NOTE: Error paths during initialization can cause _syncObjects to be null + causing an unhandled exception in finalize and a process crash. + */ + if (null != _syncObjects) + { + Dbg.Assert(null != _dataToBeSent, "Serialized streams are not initialized"); + lock (_syncObjects[(int)DataPriorityType.PromptResponse]) + { + _dataToBeSent[(int)DataPriorityType.PromptResponse].Dispose(); + } + _dataToBeSent = null; - lock (_syncObjects[(int)DataPriorityType.Default]) - { - _dataToBeSent[(int)DataPriorityType.Default].Dispose(); + lock (_syncObjects[(int)DataPriorityType.Default]) + { + _dataToBeSent[(int)DataPriorityType.Default].Dispose(); + } + + _dataToBeSent = null; + _syncObjects = null; + } } } @@ -895,4 +912,4 @@ internal virtual void Dispose(bool isDisposing) } #endregion -} \ No newline at end of file +} diff --git a/src/System.Management.Automation/engine/remoting/fanin/WSManTransportManager.cs b/src/System.Management.Automation/engine/remoting/fanin/WSManTransportManager.cs index b5725fe3cf2..bc9c88859f8 100644 --- a/src/System.Management.Automation/engine/remoting/fanin/WSManTransportManager.cs +++ b/src/System.Management.Automation/engine/remoting/fanin/WSManTransportManager.cs @@ -2602,9 +2602,19 @@ internal WSManAPIDataCommon() { ErrorCode = WSManNativeApi.WSManInitialize(WSManNativeApi.WSMAN_FLAG_REQUESTED_API_VERSION_1_1, ref _handle); } - catch (DllNotFoundException) - { - throw new PSRemotingTransportException(RemotingErrorIdStrings.WSManClientDllNotAvailable); + catch (DllNotFoundException ex) + { + PSEtwLog.LogOperationalError( + PSEventId.TransportError, + PSOpcode.Open, + PSTask.None, + PSKeyword.UseAlwaysOperational, + "WSManAPIDataCommon.ctor", + "WSManInitialize", + ex.HResult.ToString(CultureInfo.InvariantCulture), + ex.Message, + ex.StackTrace); + throw new PSRemotingTransportException(RemotingErrorIdStrings.WSManClientDllNotAvailable, ex); } // input / output streams common to all connections From 8e69dde6d11b71b14dfd71f3a7c21494dedb773d Mon Sep 17 00:00:00 2001 From: Dan Travison Date: Mon, 4 Dec 2017 15:01:02 -0800 Subject: [PATCH 2/4] Fix coding error in Clear() - setting _dataToBeSent to null too early. --- .../engine/remoting/fanin/PriorityCollection.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs b/src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs index 58fbd9d6782..df2a3bfa8f2 100644 --- a/src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs +++ b/src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs @@ -170,7 +170,6 @@ causing an unhandled exception in finalize and a process crash. { _dataToBeSent[(int)DataPriorityType.PromptResponse].Dispose(); } - _dataToBeSent = null; lock (_syncObjects[(int)DataPriorityType.Default]) { From 61ab3808d54239995a57f7fb9e453ea2d509a00e Mon Sep 17 00:00:00 2001 From: Dan Travison Date: Tue, 5 Dec 2017 13:31:32 -0800 Subject: [PATCH 3/4] rename _syncObjects to _dataSyncObjects and add comments for clarity --- .../remoting/fanin/PriorityCollection.cs | 30 +++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs b/src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs index df2a3bfa8f2..1362f5971fe 100644 --- a/src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs +++ b/src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs @@ -46,11 +46,17 @@ internal class PrioritySendDataCollection // actual data store(s) to store priority based data and its // corresponding sync objects to provide thread safety. private SerializedDataStream[] _dataToBeSent; + // array of sync objects, one for each element in _dataToBeSent + private object[] _dataSyncObjects; + + // synchronize access to _dataSyncObjects and _dataToBeSent as a set. + // while the elements in _dataSyncObjects are used to synchronize + // access to a specific element in _dataToBeSent, _syncObject is used + // to syncronize access to the set. + private object _syncObject = new Object(); + // fragmentor used to serialize & fragment objects added to this collection. private Fragmentor _fragmentor; - private object[] _syncObjects; - // synchronize access to _syncObjects and _dataToBeSent as a set. - private object _syncObject = new Object(); // callbacks used if no data is available at any time. // these callbacks are used to notify when data becomes available under @@ -97,11 +103,11 @@ internal Fragmentor Fragmentor // create serialized streams using fragment size. string[] names = Enum.GetNames(typeof(DataPriorityType)); _dataToBeSent = new SerializedDataStream[names.Length]; - _syncObjects = new object[names.Length]; + _dataSyncObjects = new object[names.Length]; for (int i = 0; i < names.Length; i++) { _dataToBeSent[i] = new SerializedDataStream(_fragmentor.FragmentSize); - _syncObjects[i] = new object(); + _dataSyncObjects[i] = new object(); } } } @@ -128,7 +134,7 @@ internal void Add(RemoteDataObject data, DataPriorityType priority) // make sure the only one object is fragmented and added to the collection // at any give time. This way the order of fragment is maintained // in the SendDataCollection(s). - lock (_syncObjects[(int)priority]) + lock (_dataSyncObjects[(int)priority]) { _fragmentor.Fragment(data, _dataToBeSent[(int)priority]); } @@ -156,28 +162,28 @@ internal void Add(RemoteDataObject data) /// internal void Clear() { - /* Lock _syncObjects and _dataToBeSent as a set to ensure atomic cleanup. */ + /* Lock _dataSyncObjects and _dataToBeSent as a set to ensure atomic cleanup. */ lock (_syncObject) { /* - NOTE: Error paths during initialization can cause _syncObjects to be null + NOTE: Error paths during initialization can cause _dataSyncObjects to be null causing an unhandled exception in finalize and a process crash. */ - if (null != _syncObjects) + if (null != _dataSyncObjects) { Dbg.Assert(null != _dataToBeSent, "Serialized streams are not initialized"); - lock (_syncObjects[(int)DataPriorityType.PromptResponse]) + lock (_dataSyncObjects[(int)DataPriorityType.PromptResponse]) { _dataToBeSent[(int)DataPriorityType.PromptResponse].Dispose(); } - lock (_syncObjects[(int)DataPriorityType.Default]) + lock (_dataSyncObjects[(int)DataPriorityType.Default]) { _dataToBeSent[(int)DataPriorityType.Default].Dispose(); } _dataToBeSent = null; - _syncObjects = null; + _dataSyncObjects = null; } } } From 4d04401c24fac5b3e21a839622cbace8cc1af2fb Mon Sep 17 00:00:00 2001 From: Dan Travison Date: Tue, 2 Jan 2018 09:26:55 -0800 Subject: [PATCH 4/4] Remove _dataSyncObject --- .../remoting/fanin/PriorityCollection.cs | 40 +++++++++---------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs b/src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs index 1362f5971fe..552668fba40 100644 --- a/src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs +++ b/src/System.Management.Automation/engine/remoting/fanin/PriorityCollection.cs @@ -49,12 +49,6 @@ internal class PrioritySendDataCollection // array of sync objects, one for each element in _dataToBeSent private object[] _dataSyncObjects; - // synchronize access to _dataSyncObjects and _dataToBeSent as a set. - // while the elements in _dataSyncObjects are used to synchronize - // access to a specific element in _dataToBeSent, _syncObject is used - // to syncronize access to the set. - private object _syncObject = new Object(); - // fragmentor used to serialize & fragment objects added to this collection. private Fragmentor _fragmentor; @@ -162,28 +156,32 @@ internal void Add(RemoteDataObject data) /// internal void Clear() { - /* Lock _dataSyncObjects and _dataToBeSent as a set to ensure atomic cleanup. */ - lock (_syncObject) + /* + NOTE: Error paths during initialization can cause _dataSyncObjects to be null + causing an unhandled exception in finalize and a process crash. + Verify arrays and dataToBeSent objects before referencing. + */ + if (null != _dataSyncObjects && null != _dataToBeSent) { - /* - NOTE: Error paths during initialization can cause _dataSyncObjects to be null - causing an unhandled exception in finalize and a process crash. - */ - if (null != _dataSyncObjects) + int promptResponseIndex = (int)DataPriorityType.PromptResponse; + int defaultIndex = (int)DataPriorityType.Default; + + lock (_dataSyncObjects[promptResponseIndex]) { - Dbg.Assert(null != _dataToBeSent, "Serialized streams are not initialized"); - lock (_dataSyncObjects[(int)DataPriorityType.PromptResponse]) + if (null != _dataToBeSent[promptResponseIndex]) { - _dataToBeSent[(int)DataPriorityType.PromptResponse].Dispose(); + _dataToBeSent[promptResponseIndex].Dispose(); + _dataToBeSent[promptResponseIndex] = null; } + } - lock (_dataSyncObjects[(int)DataPriorityType.Default]) + lock (_dataSyncObjects[defaultIndex]) + { + if (null != _dataToBeSent[defaultIndex]) { - _dataToBeSent[(int)DataPriorityType.Default].Dispose(); + _dataToBeSent[defaultIndex].Dispose(); + _dataToBeSent[defaultIndex] = null; } - - _dataToBeSent = null; - _dataSyncObjects = null; } } }