From: Jonathan Pryor Date: Fri, 1 Nov 2013 20:39:42 +0000 (-0400) Subject: [System] Avoid an NRE when Close() and Write() are concurrent. X-Git-Url: http://wien.tomnetworks.com/gitweb/?a=commitdiff_plain;h=55f400b6446024adef5dcc8cc690b6951af98125;p=mono.git [System] Avoid an NRE when Close() and Write() are concurrent. Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=15857 Scenario (for which I couldn't easily create a testcase, but can sketch out): Imagine using HttpClient...in a multithreaded manner: // shared: static CancellationTokenSource _cancellationTokenSource = new CancellationTokenSource (); // Thread 1 var request = new HttpRequestMessage(HttpMethod.Post, "https://www.google.com/") { Content = new StringContent (new string('x', 1024)), }; using (request) using (HttpClient httpClient = new HttpClient()) using (HttpResponseMessage response = await httpClient.SendAsync(request, _cancellationTokenSource.Token)) { string responseContent = await response.Content.ReadAsStringAsync(); Console.WriteLine ("Read {0} chars", responseContent.Length); } // Thread 2 _cancellationTokenSource.Cancel (); This isn't entirely farfetched; in the case of #15857, Thread 2 was the Main thread (due to the user clicking a button), while Thread 1 was the ThreadPool (because of async/await). The _occasional_ result: a TargetInvocationException exception: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.NullReferenceException: Object reference not set to an instance of an object at (wrapper delegate-invoke) :invoke_bool__this___HttpsClientStream (Mono.Security.Protocol.Tls.HttpsClientStream) at System.Reflection.MonoProperty.GetterAdapterFrame[HttpsClientStream,Boolean] (System.Reflection.Getter`2 getter, System.Object obj) at System.Reflection.MonoProperty.GetValue (System.Object obj, System.Object[] index) --- End of inner exception stack trace --- at System.Reflection.MonoProperty.GetValue (System.Object obj, System.Object[] index) at System.Net.WebConnection.Write (System.Net.HttpWebRequest request, System.Byte[] buffer, Int32 offset, Int32 size, System.String& err_msg) at System.Net.WebConnectionStream.WriteHeaders () at System.Net.WebConnectionStream.SetHeaders (System.Byte[] buffer) at System.Net.HttpWebRequest.SendRequestHeaders (Boolean propagate_error) --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw () at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1+ConfiguredTaskAwaiter[System.Net.Http.HttpResponseMessage].GetResult () at System.Net.Http.HttpClient+c__async0.MoveNext () --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw () at System.Runtime.CompilerServices.TaskAwaiter`1[System.Net.Http.HttpResponseMessage].GetResult () at HttpClientAsyncCancellationRepro.Activity1+c__async0.MoveNext () Huh? The reason why is because HttpClientHandler.SendAsync() registers a Cancel handler with the CancellationTokenSource, and the registered handler calls HttpWebRequest.Abort() (which eventually calls WebConnection.Close()). This causes WebConnection.nstream to _change_ while WebConnection.Write() is executing, causing WebConnection.nstream to go from non-null to null, which WebConnection.Write() does not expect: # WebConnection.CreateStream; from 5 Threadpool worker # WebConnection.Write; from 5 Threadpool worker # WebConnection.Close; from 1 at at System.Environment.get_StackTrace() at System.Net.WebConnection.Close(Boolean sendNext) at System.Net.WebConnection.Abort(System.Object sender, System.EventArgs args) at System.Net.WebConnection+AbortHelper.Abort(System.Object sender, System.EventArgs args) at System.Net.HttpWebRequest.Abort() at System.Net.Http.HttpClientHandler+c__async0.<>m__0(System.Object l) at System.Threading.CancellationToken+c__AnonStorey0.<>m__0() at System.Threading.CancellationTokenSource.Cancel(Boolean throwOnFirstException) at System.Threading.CancellationTokenSource.Cancel() at System.Threading.CancellationTokenSource.SafeLinkedCancel() at System.Threading.CancellationTokenSource.Cancel(Boolean throwOnFirstException) at System.Threading.CancellationTokenSource.Cancel() at HttpClientAsyncCancellationRepro.Activity1.CancelButton_Click(System.Object sender, System.EventArgs e) at Android.Views.View+IOnClickListenerImplementor.OnClick(Android.Views.View v) at Android.Views.View+IOnClickListenerInvoker.n_OnClick_Landroid_view_View_(IntPtr jnienv, IntPtr native__this, IntPtr native_v) at System.Object.e7b5b870-ea0f-434f-bc31-984f665a44ad(IntPtr , IntPtr , IntPtr ) # WebConnection.Write: checking piTrustFailure; from 5 Threadpool worker (Yay extra logging. Notice that WebConnection creates a stream from the ThreadPool, starts WebConnection.Write(), then before it can se piTrustFailure the instance is Close()d, which nulls out nstream.) Since WebConnection.Write() attempts to access the (now null) WebConnection.nstream field, it results in a NullReferenceException, which is wrapped into a TargetInvocationException by System.Reflection. The fix? As usual, "don't do that." In this case, WebConnection.Write() has already captured WebConnection.nstream into the local variable `s` (which thus can't be changed by another thread). Instead of using WebConnection.nstream, reuse the `s` local variable, which cannot be null, thus avoiding the NullReferenceException. (This solution could fail if HttpsClientStream ever throws e.g. ObjectDisposedException from HttpsClientStream.TrustFailure, but at present HttpsClientStream.TrustFailure never throws, so this is fine.) --- diff --git a/mcs/class/System/System.Net/WebConnection.cs b/mcs/class/System/System.Net/WebConnection.cs index cf07f572eff..2033fe9b79f 100644 --- a/mcs/class/System/System.Net/WebConnection.cs +++ b/mcs/class/System/System.Net/WebConnection.cs @@ -1166,10 +1166,10 @@ namespace System.Net // if SSL is in use then check for TrustFailure if (ssl) { #if SECURITY_DEP && MONOTOUCH - HttpsClientStream https = (nstream as HttpsClientStream); + HttpsClientStream https = (s as HttpsClientStream); if (https.TrustFailure) { #else - if ((bool) piTrustFailure.GetValue (nstream, null)) { + if ((bool) piTrustFailure.GetValue (s , null)) { #endif wes = WebExceptionStatus.TrustFailure; msg = "Trust failure";