[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.)

diff --git a/mcs/class/System/System.Net/WebConnection.cs b/mcs/class/System/System.Net/WebConnection.cs
index cf07f572effc60a5d98307b4586cf7440d2c2c2c..2033fe9b79f238fbce95963314eec2f480eeacff 100644 (file)
--- 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";