[System] Avoid an NRE when Close() and Write() are concurrent.
authorJonathan Pryor <jonpryor@vt.edu>
Fri, 1 Nov 2013 20:39:42 +0000 (16:39 -0400)
committerJonathan Pryor <jonpryor@vt.edu>
Fri, 1 Nov 2013 20:56:53 +0000 (16:56 -0400)
commit55f400b6446024adef5dcc8cc690b6951af98125
tree3986b5ce1c71415033ba4b3ffc3596151db24940
parent06693aa3bb552013d57b07b9cf53c45fe4efa94f
[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) <Module>: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+<SendAsyncWorker>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+<AccessGoogleButton_Click>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+<SendAsync>c__async0.<>m__0(System.Object l)
   at System.Threading.CancellationToken+<Register>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.)
mcs/class/System/System.Net/WebConnection.cs