Mono.Net.Security.MonoTlsStream: Crash when stream appears to have already been disposed

[tdiehl]

Steps to Reproduce

Unfortunately I don't yet have a reproducible case, we are observing this crash in production release of an Xamarin iOS app.

Current Behavior

We are getting crashes MonoTlsStream when the stream is being disposed here.

I suspect the behavior may be because of a timeout or some work being cancelled, perhaps causing the stream to be disposed here first.

Expected Behavior

No crash.

On which platforms did you notice this

[x] iOS
[ ] macOS
[ ] Linux
[ ] Windows

Version Used:
Mono JIT compiler version 5.18.1.28 (2018-08/223ea7ef92e Tue May 21 12:03:39 EDT 2019)
Copyright (C) 2002-2014 Novell, Inc, Xamarin Inc and Contributors. www.mono-project.com
TLS:
SIGSEGV: altstack
Notification: kqueue
Architecture: amd64
Disabled: none
Misc: softdebug
Interpreter: yes
LLVM: yes(600)
Suspend: preemptive
GC: sgen (concurrent by default)

Stacktrace

Crashed: Thread Pool Worker
EXC_BAD_ACCESS KERN_INVALID_ADDRESS 0x0000000000000000

0  FaithlifeReaderApp             0x103db6b88 Mono_Net_Security_MonoTlsStream__CreateStreamd__17_MoveNext + 139 (MonoTlsStream.cs:139)
1  FaithlifeReaderApp             0x103db6b7c Mono_Net_Security_MonoTlsStream__CreateStreamd__17_MoveNext + 139 (MonoTlsStream.cs:139)
2  FaithlifeReaderApp             0x1044b63fc System_Runtime_CompilerServices_AsyncMethodBuilderCore_MoveNextRunner_InvokeMoveNext_object + 20745408
3  FaithlifeReaderApp             0x10443b2d0 System_Threading_ExecutionContext_RunInternal_System_Threading_ExecutionContext_System_Threading_ContextCallback_object_bool + 20241300
4  FaithlifeReaderApp             0x10443b11c System_Threading_ExecutionContext_Run_System_Threading_ExecutionContext_System_Threading_ContextCallback_object_bool + 20240864
5  FaithlifeReaderApp             0x1044b62bc System_Runtime_CompilerServices_AsyncMethodBuilderCore_MoveNextRunner_Run + 20745088
6  FaithlifeReaderApp             0x10445eb94 System_Threading_Tasks_AwaitTaskContinuation_RunOrScheduleAction_System_Action_bool_System_Threading_Tasks_Task_ + 20386904
7  FaithlifeReaderApp             0x1044587c0 System_Threading_Tasks_Task_FinishContinuations + 20361348
8  FaithlifeReaderApp             0x104456d80 System_Threading_Tasks_Task_FinishStageThree + 20354628
9  FaithlifeReaderApp             0x104456cc0 System_Threading_Tasks_Task_FinishStageTwo + 20354436
10 FaithlifeReaderApp             0x104456988 System_Threading_Tasks_Task_Finish_bool + 20353612
11 FaithlifeReaderApp             0x104454470 System_Threading_Tasks_Task_TrySetException_object + 20344116
12 FaithlifeReaderApp             0x104665fec System_Runtime_CompilerServices_AsyncTaskMethodBuilder_1_System_Threading_Tasks_VoidTaskResult_SetException_System_Exception + 22513840
13 FaithlifeReaderApp             0x1044b4618 System_Runtime_CompilerServices_AsyncTaskMethodBuilder_SetException_System_Exception + 20737756
14 FaithlifeReaderApp             0x103db3840 Mono_Net_Security_MobileAuthenticatedStream__ProcessAuthenticationd__62_MoveNext + 396 (MobileAuthenticatedStream.cs:396)
15 FaithlifeReaderApp             0x1044b63fc System_Runtime_CompilerServices_AsyncMethodBuilderCore_MoveNextRunner_InvokeMoveNext_object + 20745408
16 FaithlifeReaderApp             0x10443b2d0 System_Threading_ExecutionContext_RunInternal_System_Threading_ExecutionContext_System_Threading_ContextCallback_object_bool + 20241300
17 FaithlifeReaderApp             0x10443b11c System_Threading_ExecutionContext_Run_System_Threading_ExecutionContext_System_Threading_ContextCallback_object_bool + 20240864
18 FaithlifeReaderApp             0x1044b62bc System_Runtime_CompilerServices_AsyncMethodBuilderCore_MoveNextRunner_Run + 20745088
19 FaithlifeReaderApp             0x10445eb94 System_Threading_Tasks_AwaitTaskContinuation_RunOrScheduleAction_System_Action_bool_System_Threading_Tasks_Task_ + 20386904
20 FaithlifeReaderApp             0x1044587c0 System_Threading_Tasks_Task_FinishContinuations + 20361348
21 FaithlifeReaderApp             0x104456d80 System_Threading_Tasks_Task_FinishStageThree + 20354628
22 FaithlifeReaderApp             0x1044519e8 System_Threading_Tasks_Task_1_TResult_REF_TrySetResult_TResult_REF + 20333228
23 FaithlifeReaderApp             0x1044b4c38 System_Runtime_CompilerServices_AsyncTaskMethodBuilder_1_TResult_REF_SetResult_TResult_REF + 20739324
24 FaithlifeReaderApp             0x103dae570 Mono_Net_Security_AsyncProtocolRequest__StartOperationd__23_MoveNext + 193 (AsyncProtocolRequest.cs:193)
25 FaithlifeReaderApp             0x1044b63fc System_Runtime_CompilerServices_AsyncMethodBuilderCore_MoveNextRunner_InvokeMoveNext_object + 20745408
26 FaithlifeReaderApp             0x10443b2d0 System_Threading_ExecutionContext_RunInternal_System_Threading_ExecutionContext_System_Threading_ContextCallback_object_bool + 20241300
27 FaithlifeReaderApp             0x10443b11c System_Threading_ExecutionContext_Run_System_Threading_ExecutionContext_System_Threading_ContextCallback_object_bool + 20240864
28 FaithlifeReaderApp             0x1044b62bc System_Runtime_CompilerServices_AsyncMethodBuilderCore_MoveNextRunner_Run + 20745088
29 FaithlifeReaderApp             0x10445eb94 System_Threading_Tasks_AwaitTaskContinuation_RunOrScheduleAction_System_Action_bool_System_Threading_Tasks_Task_ + 20386904
30 FaithlifeReaderApp             0x1044587c0 System_Threading_Tasks_Task_FinishContinuations + 20361348
31 FaithlifeReaderApp             0x104456d80 System_Threading_Tasks_Task_FinishStageThree + 20354628
32 FaithlifeReaderApp             0x104456cc0 System_Threading_Tasks_Task_FinishStageTwo + 20354436
33 FaithlifeReaderApp             0x104456988 System_Threading_Tasks_Task_Finish_bool + 20353612
34 FaithlifeReaderApp             0x104454470 System_Threading_Tasks_Task_TrySetException_object + 20344116
35 FaithlifeReaderApp             0x104665fec System_Runtime_CompilerServices_AsyncTaskMethodBuilder_1_System_Threading_Tasks_VoidTaskResult_SetException_System_Exception + 22513840
36 FaithlifeReaderApp             0x1044b4618 System_Runtime_CompilerServices_AsyncTaskMethodBuilder_SetException_System_Exception + 20737756
37 FaithlifeReaderApp             0x103daebc0 Mono_Net_Security_AsyncProtocolRequest__ProcessOperationd__24_MoveNext + 198 (AsyncProtocolRequest.cs:198)
38 FaithlifeReaderApp             0x1044b63fc System_Runtime_CompilerServices_AsyncMethodBuilderCore_MoveNextRunner_InvokeMoveNext_object + 20745408
39 FaithlifeReaderApp             0x10443b2d0 System_Threading_ExecutionContext_RunInternal_System_Threading_ExecutionContext_System_Threading_ContextCallback_object_bool + 20241300
40 FaithlifeReaderApp             0x10443b11c System_Threading_ExecutionContext_Run_System_Threading_ExecutionContext_System_Threading_ContextCallback_object_bool + 20240864
41 FaithlifeReaderApp             0x1044b62bc System_Runtime_CompilerServices_AsyncMethodBuilderCore_MoveNextRunner_Run + 20745088
42 FaithlifeReaderApp             0x10445eb94 System_Threading_Tasks_AwaitTaskContinuation_RunOrScheduleAction_System_Action_bool_System_Threading_Tasks_Task_ + 20386904
43 FaithlifeReaderApp             0x1044587c0 System_Threading_Tasks_Task_FinishContinuations + 20361348
44 FaithlifeReaderApp             0x104456d80 System_Threading_Tasks_Task_FinishStageThree + 20354628
45 FaithlifeReaderApp             0x104456cc0 System_Threading_Tasks_Task_FinishStageTwo + 20354436
46 FaithlifeReaderApp             0x104456988 System_Threading_Tasks_Task_Finish_bool + 20353612
47 FaithlifeReaderApp             0x104454470 System_Threading_Tasks_Task_TrySetException_object + 20344116
48 FaithlifeReaderApp             0x103ef8f0c System_Runtime_CompilerServices_AsyncTaskMethodBuilder_1_System_Nullable_1_int_SetException_System_Exception + 662 (AsyncMethodBuilder.cs:662)
49 FaithlifeReaderApp             0x103daf06c Mono_Net_Security_AsyncProtocolRequest__InnerReadd__25_MoveNext + 260 (AsyncProtocolRequest.cs:260)
50 FaithlifeReaderApp             0x1044b63fc System_Runtime_CompilerServices_AsyncMethodBuilderCore_MoveNextRunner_InvokeMoveNext_object + 20745408
51 FaithlifeReaderApp             0x10443b2d0 System_Threading_ExecutionContext_RunInternal_System_Threading_ExecutionContext_System_Threading_ContextCallback_object_bool + 20241300
52 FaithlifeReaderApp             0x10443b11c System_Threading_ExecutionContext_Run_System_Threading_ExecutionContext_System_Threading_ContextCallback_object_bool + 20240864
53 FaithlifeReaderApp             0x1044b62bc System_Runtime_CompilerServices_AsyncMethodBuilderCore_MoveNextRunner_Run + 20745088
54 FaithlifeReaderApp             0x10445eb94 System_Threading_Tasks_AwaitTaskContinuation_RunOrScheduleAction_System_Action_bool_System_Threading_Tasks_Task_ + 20386904
55 FaithlifeReaderApp             0x1044587c0 System_Threading_Tasks_Task_FinishContinuations + 20361348
56 FaithlifeReaderApp             0x104456d80 System_Threading_Tasks_Task_FinishStageThree + 20354628
57 FaithlifeReaderApp             0x104456cc0 System_Threading_Tasks_Task_FinishStageTwo + 20354436
58 FaithlifeReaderApp             0x104456988 System_Threading_Tasks_Task_Finish_bool + 20353612
59 FaithlifeReaderApp             0x104454470 System_Threading_Tasks_Task_TrySetException_object + 20344116
60 FaithlifeReaderApp             0x104651f24 System_Runtime_CompilerServices_AsyncTaskMethodBuilder_1_TResult_INT_SetException_System_Exception + 22431720
61 FaithlifeReaderApp             0x103db4768 Mono_Net_Security_MobileAuthenticatedStream__InnerReadd__85_MoveNext + 747 (MobileAuthenticatedStream.cs:747)
62 FaithlifeReaderApp             0x1044b63fc System_Runtime_CompilerServices_AsyncMethodBuilderCore_MoveNextRunner_InvokeMoveNext_object + 20745408
63 FaithlifeReaderApp             0x10443b2d0 System_Threading_ExecutionContext_RunInternal_System_Threading_ExecutionContext_System_Threading_ContextCallback_object_bool + 20241300
64 FaithlifeReaderApp             0x10443b11c System_Threading_ExecutionContext_Run_System_Threading_ExecutionContext_System_Threading_ContextCallback_object_bool + 20240864
65 FaithlifeReaderApp             0x1044b62bc System_Runtime_CompilerServices_AsyncMethodBuilderCore_MoveNextRunner_Run + 20745088
66 FaithlifeReaderApp             0x10445eb94 System_Threading_Tasks_AwaitTaskContinuation_RunOrScheduleAction_System_Action_bool_System_Threading_Tasks_Task_ + 20386904
67 FaithlifeReaderApp             0x1044587c0 System_Threading_Tasks_Task_FinishContinuations + 20361348
68 FaithlifeReaderApp             0x104456d80 System_Threading_Tasks_Task_FinishStageThree + 20354628
69 FaithlifeReaderApp             0x104456cc0 System_Threading_Tasks_Task_FinishStageTwo + 20354436
70 FaithlifeReaderApp             0x104456988 System_Threading_Tasks_Task_Finish_bool + 20353612
71 FaithlifeReaderApp             0x104454470 System_Threading_Tasks_Task_TrySetException_object + 20344116
72 FaithlifeReaderApp             0x1046694e4 System_Threading_Tasks_TaskFactory_1_FromAsyncTrimPromise_1_TResult_INT_TInstance_REF_Complete_TInstance_REF_System_Func_3_TInstance_REF_System_IAsyncResult_TResult_INT_System_IAsyncResult_bool + 22527400
73 FaithlifeReaderApp             0x104669314 System_Threading_Tasks_TaskFactory_1_FromAsyncTrimPromise_1_TResult_INT_TInstance_REF_CompleteFromAsyncResult_System_IAsyncResult + 22526936
74 FaithlifeReaderApp             0x103e86374 System_Net_Sockets_SocketAsyncResult__c__Completeb__27_0_object + 157 (SocketAsyncResult.cs:157)
75 FaithlifeReaderApp             0x104443054 System_Threading_QueueUserWorkItemCallback_System_Threading_IThreadPoolWorkItem_ExecuteWorkItem + 20273432
76 FaithlifeReaderApp             0x104440df8 System_Threading_ThreadPoolWorkQueue_Dispatch + 20264636
77 FaithlifeReaderApp             0x10477a200 ObjCRuntime_Runtime_ThreadPoolDispatcher_System_Func_1_bool + 288 (Runtime.cs:288)
78 FaithlifeReaderApp             0x104442e88 System_Threading__ThreadPoolWaitCallback_PerformWaitCallback + 20272972
79 FaithlifeReaderApp             0x1046812e8 wrapper_runtime_invoke_object_runtime_invoke_dynamic_intptr_intptr_intptr_intptr + 22625196
80 FaithlifeReaderApp             0x102f8328c mono_jit_runtime_invoke + 3120 (mini-runtime.c:3120)
81 FaithlifeReaderApp             0x1030280ec mono_runtime_try_invoke + 2960 (object.c:2960)
82 FaithlifeReaderApp             0x103059c10 worker_callback + 284 (threadpool.c:284)
83 FaithlifeReaderApp             0x103057980 worker_thread + 476 (threadpool-worker-default.c:476)
84 FaithlifeReaderApp             0x103062110 start_wrapper_internal + 1155 (threads.c:1155)
85 FaithlifeReaderApp             0x103061f94 start_wrapper + 1217 (threads.c:1217)
86 libsystem_pthread.dylib        0x215e542c0 _pthread_body + 128
87 libsystem_pthread.dylib        0x215e54220 _pthread_start + 44
88 libsystem_pthread.dylib        0x215e57cdc thread_start + 4

[marek-safar]

/cc @steveisok

[filipnavara]

Sounds suspiciously similar to #10488. If that is the case then updating to newer version should resolve the issue.

[steveisok]

Yeah, it does. @baulig, please confirm.

[baulig]

I reviewed the code and it turns out that we're actually lacking a Dispose () call in the second catch block (which I just fixed in #16233).

Apart from that, I don't see how we could possibly throw here. Do you have any test case or some way to reproduce this?

[tdiehl]

Unfortunately, I don't have a reproducible test at the moment.

We've been running for the past couple of weeks with a custom build of Mono in our Xamarin apps that uses a simple null check, which has eliminated this particular crash. Naturally I'd prefer not to have to resort to that permanently. :)

[digvijaypundir1]

Unfortunately, I don't have a reproducible test at the moment.

We've been running for the past couple of weeks with a custom build of Mono in our Xamarin apps that uses a simple null check, which has eliminated this particular crash. Naturally I'd prefer not to have to resort to that permanently. :)

I am also having the same crash. I don't know about how we can do it in our solution. As mono is the part of .net framework. Is it open source and we can modify directly?
Or there is any other solution for that as you stated custom build for Mono? Is it simply take the latest using git and change use that in our project?

Hope solution will come in next release of mono.

[ppvictory]

Also experiencing the same crash here. It look's like we'll have to wait until the next release of Xamarin.iOS to use the latest mono and see if this fixes this crash.

[pachecosoftware]

We too are experiencing this issue. Crashyltics is reporting that it has affected over 1000 users over the past week, so it seems to be a pretty significant issue for us. @tdiehl, can you elaborate regarding how you were able to use your patched version of Mono in your Xamarin.iOS app?

[pachecosoftware]

We just upgraded to the latest and greatest versions of Xamarin iOS and Mono but are still seeing this issue. Any idea on when this fix might get included in a new build of Mono?

[steveisok]

@baulig The dispose fix should be in stable. Can you please do another pass on this issue.

[pachecosoftware]

@steveisok, thanks for the update. Does @tdiehl 's workaround of adding the null check need to be included in the next build in addition to the dispose fix that @baulig worked on in order to fully resolve this issue?

[steveisok]

@pachecosoftware according to the PR @baulig merged, the null check is unnecessary. https://github.com/mono/mono/pull/16233/files#r314017987

If I'm understanding your comments correctly, you're seeing otherwise?

[pachecosoftware]

@steveisok, Thanks for providing the link to the PR. I'm admittedly not familiar with the code in question, but after reading through the comments and looking through the before and after versions of the code, I'm still a bit confused on how adding the missing sslStream.Dispose() (on line 145 in the latest version of the code) solves the underlying issue that @tdiehl was addressing with his patch to work around the situation where somehow the sslStream is null inside the finally block (now refactored to be part of the catch block).

One way I could see that the sslStream variable could potentially be null is if the call to provider.CreateSslStream (networkStream, false, settings) on line 107 ever returns null. That scenario would set the sslStream variable to null on 107, presumably causing a NullReferenceException to be thrown on line 117, which would be caught by the catch block on line 125, but the sslStream variable would remain null and throw another NullReferenceException on line 135. I'm not sure if it's possible for any implementation of the MonoTlsProvider to return null, but if it is, then it would seem that we would still need a null check at line 135 to guard against that scenario?

[tdiehl]

We updated to the latest stable in Visual Studio 8.3 and still see the issue, though the NRE is now thrown at line 135. I'm in the process of building a custom version of xamarin-macios and applying the same null check on the sslStream. @pachecosoftware, I essentially then copy the Xamarin.iOS.framework built System.dll over the version in /Library/Frameworks.

Applying this patch does however appear to cause another slightly different crash, one appears to maybe be here.

Finally, we still don't have a consistent repro of the issue. However, we have occasionally seen the problem happen while debugging within Xcode (we have a hybrid app). I'm attaching some screenshots as they show the complete stack trace as well as some additional logs. The problem appears to possibly be related to timed out requests.

MonoTlsStream-StackTrace.zip

[baulig]

I've looked at that code over and over again and while @steveisok's #17270 certainly doesn't hurt, it is also completely unnecessary - as to whether or not might potentially be damaging, I'm not sure about that.

That sslStream = provider.CreateSslStream (networkStream, false, settings) on line 107 can never return null - it also shouldn't ever throw because the implementation is essentially return new ...

And then in the entire body of that method, sslStream is clearly non-null wherever it's being accesses, there's no way how it could possibly get null. Which makes that PR superfluous.

---

However, we are seeing people complain about NRE's in that method, so we need to think outside the box here. And in fact, there is one scenario where this actually could happen - but unfortunately we don't have enough information about it at the moment.

So instead of landing @steveisok's PR, I would suggest to test a theory instead - it won't make the problem go away, but it will make the crashes more verbose and also confirm my theory.

---

The only scenario in which sslStream could possibly be null is by that method being called concurrently from multiple threads. In theory, this should never happen, but I don't see another scenario in which a NRE could happen here.

This function is clearly designed to be called only once and it's especially not reentrant.

Hypothetically if one thread is in that await sslStream.WriteAsync (....) call and then another thread reenters the method, the call to AuthenticateAsClientAsync () is guaranteed to throw because the connection has already been established, thus entering the catch block that will dispose and then set sslStream to null. This will then also abort the first thread's WriteAsync() call with an exception (underlying stream being closed) and when it's then hitting the second catch block, it will find the sslStream already being null'ed out.

To protect against this scenario - and to confirm my hypothesis - I suggest that we add some Interlocked.CompareExchange() to the top of that method.

What do you think?

[pachecosoftware]

@baulig, were you able to make your proposed change? If this truly is a multi-threading issue, do we have any idea how this particular scenario could be happening? @tdiehl has suggested that perhaps it's related to requests timing out? In our app, we are reusing a single instance of HttpClient throughout the app and there are undoubtedly multiple threads making requests simultaneously. We have the timeout value set to 20 seconds. We are also using the Xamarin.FFImageLoading for image loading/caching, which is itself making HTTPS requests to load images, and I believe its default timeout value is 15 seconds.

[jahmai-ca]

This hits our apps every so often. After reading the code it does seem like a race condition between one thread creating the stream and another thread disposing the stream.

That is, MonoTlsStream.CreateStream() takes a very long time due to sslStream.AuthenticateAsClientAsync, then MonoTlsStream.Dispose() is called nulling sslStream, then MonoTlsStream.CreateStream() throws at sslStream.Dispose().

For us, this can happen if there are comms issues and there is a forced cleanup of objects to cancel a pending request. In this particular case though it appears to happen either internally to mono or from a 3rd party library.

@steveisok Had the right idea, but the fix only reduces the likelihood of the occurrence thanks to the null check, there is still the slight possibility of instruction pointers on two threads lining up such that both null checks pass and then one thread disposes and assigns null, and then the other thread tries to dispose and crashes. I think that fix plus some locking would fix the issue.

[steveisok]

@jahmai Thanks for your thoughts and I agree that seems like what is happening. I suspect MonoTlsStream.CreateStream() is being called multiple times for the same resources and/or it's being disposed of first and then the exception handler runs when a request times out, for example.

mono/mcs/class/System/System.Net/WebConnection.cs

Lines 419 to 446 in 450d474

	void CloseSocket ()
	{
	lock (this) {
	Debug ($"WC CLOSE SOCKET: Cnc={ID} NS={networkStream} TLS={monoTlsStream}");
	if (networkStream != null) {
	try {
	networkStream.Dispose ();
	} catch { }
	networkStream = null;
	}
	if (monoTlsStream != null) {
	try {
	monoTlsStream.Dispose ();
	} catch { }
	monoTlsStream = null;
	}
	if (socket != null) {
	try {
	socket.Dispose ();
	} catch { }
	socket = null;
	}
	monoTlsStream = null;
	}
	}

[jahmai-ca]

The tricky thing about NullReferenceExceptions is that exception trapping libraries like Crashlytics actually treat them as fatal signals so you can't even catch them in managed code.

[pachecosoftware]

@jahmai, as you noted, this issue has been especially frustrating because despite adding all kinds of exception handling to our app around network interactions, the app crashes rather than propagating the exception so that our code can handle it. @steveisok, are there plans to modify your fix (#17270) to add locking? Beyond that, is there an ETA on getting this released? We continue to get complaints from our users who keep running into this issue.

[steveisok]

Yes, we will address this soon. I don't have an ETA at this time.

[pachecosoftware]

Thanks, @steveisok!

[pachecosoftware]

@baulig and @steveisok, I checked out the commit for this fix but didn't see any additional locking added as @jahmai suggested above to avoid any potential race conditions involving two threads entering the null check at the same time. Did you just think the likelihood of this occurring was so small that it didn't justify the potential performance hit locking could introduce?

[steveisok]

@pachecosoftware I asked @baulig to have a quick follow up PR as I think it's pretty cheap to get in even though the likelihood is small

[pachecosoftware]

@steveisok Sounds great! Thanks! Is there any kind of ETA for when Mono 6.6.0 will be ready for release?

[jahmai-ca]

Seems this has been closed without any defense against race conditions?

[ppvictory]

Speaking of Mono 6.6.0 this issue isn't listed as being resolved in the mono 6.6.0 release https://www.mono-project.com/docs/about-mono/releases/6.6.0/. Was it included in that release or are we looking at a later release to include the fix?

[steveisok]

@ppvictory We merged this into 2019-10, which is mono 6.8.0. When that version is released, we will include the fix in the release notes.

[baulig]

Seems this has been closed without any defense against race conditions?

@jahmai Did I make a mistake in #17270 or is there any place where another race condition is happening?

[akoeplinger]

@baulig there's still a potential race condition in

mono/mcs/class/System/Mono.Net.Security/MonoTlsStream.cs

Lines 159 to 163 in 56f397b

	void CloseSslStream () {
	if (sslStream != null) {
	sslStream.Dispose ();
	sslStream = null;
	}

If two threads call CloseStream and one of them reaches sslStream = null after the other did the null-check then calling Dispose will still fail.

[baulig]

Calling Dispose() twice should never be an issue - and SslStream already guards against that.

Yes, one could add some Interlocked.Exchange around it - but that will only even do anything in a Debug build - and there are plenty of other places in the BCL where we're having this exact same pattern.

[jahmai-ca]

@baulig That's not the problem. The problem is one thread calling sslStream = null; then another thread calling sslStream.Dispose(); and throwing NullReferenceException.

[baulig]

Which is not something you'd ever encounter in real life at all. But if it's so important to you, feel free to make a PR.

[jahmai-ca]

Explain. This issue is predicated on a thread race condition between two threads attempting to destroy the sslStream concurrently. The fix narrows the chances but doesn’t eliminate it.

[akoeplinger]

I opened a PR adding some locking: #18250

[pachecosoftware]

@akoeplinger Thanks so much for adding this. This bug was wreaking havoc in our app, so I ended up building a custom version of Mono adding in what is essentially identical code to what you have in your PR and it has eliminated the incidence of this issue for us.

[baulig]

Thank you so much for this and my apologies for being so rude to both of you earlier on.

[akoeplinger]

@pachecosoftware thanks for the confirmation, good to hear that the PR should fix the issue. I merged it and backported to our 2019-10 branch (== Mono 6.8) so it should get into that release. Xamarin.iOS is also currently tracking that branch for the VS 16.5 release.