Mono: Mono.Net.Security.MonoTlsStream: Crash when stream appears to have already been disposed
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
tdiehl
All 38 comments
/cc @steveisok
marek-safar
on 24 Jul 2019
Sounds suspiciously similar to https://github.com/mono/mono/issues/10488. If that is the case then updating to newer version should resolve the issue.
filipnavara
on 24 Jul 2019
Yeah, it does. @baulig, please confirm.
steveisok
on 24 Jul 2019
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 https://github.com/mono/mono/pull/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?
baulig
on 14 Aug 2019
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. :)
tdiehl
on 20 Aug 2019
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.
digvijaypundir1
on 20 Aug 2019
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.
ppvictory
on 17 Sep 2019
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
on 18 Sep 2019
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?
pachecosoftware
on 2 Oct 2019
@baulig The dispose fix should be in stable. Can you please do another pass on this issue.
steveisok
on 2 Oct 2019
@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?
pachecosoftware
on 10 Oct 2019
@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?
steveisok
on 10 Oct 2019
@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?
pachecosoftware
on 10 Oct 2019
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.
tdiehl
on 10 Oct 2019
I've looked at that code over and over again and while @steveisok's https://github.com/mono/mono/pull/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?
baulig
on 14 Oct 2019
@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.
pachecosoftware
on 24 Oct 2019
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.
jahmai
on 27 Oct 2019
@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.
https://github.com/mono/mono/blob/450d4746849d228c85748d9ab4ef68f90ac0c7f2/mcs/class/System/System.Net/WebConnection.cs#L419-L446
steveisok
on 29 Oct 2019
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.
jahmai
on 4 Nov 2019
@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 (https://github.com/mono/mono/pull/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.
pachecosoftware
on 5 Nov 2019
Yes, we will address this soon. I don't have an ETA at this time.
steveisok
on 5 Nov 2019
Thanks, @steveisok!
pachecosoftware
on 5 Nov 2019
@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?
pachecosoftware
on 19 Nov 2019
@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
steveisok
on 19 Nov 2019
@steveisok Sounds great! Thanks! Is there any kind of ETA for when Mono 6.6.0 will be ready for release?
pachecosoftware
on 19 Nov 2019
Seems this has been closed without any defense against race conditions?
jahmai
on 10 Dec 2019
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?
ppvictory
on 11 Dec 2019
@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.
steveisok
on 11 Dec 2019
Seems this has been closed without any defense against race conditions?
@jahmai Did I make a mistake in https://github.com/mono/mono/pull/17270 or is there any place where another race condition is happening?
baulig
on 11 Dec 2019
@baulig there's still a potential race condition in https://github.com/mono/mono/blob/56f397bef3f6ac7b42f1004efd601aa97b37661b/mcs/class/System/Mono.Net.Security/MonoTlsStream.cs#L159-L163
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.
akoeplinger
on 18 Dec 2019
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.
baulig
on 18 Dec 2019
@baulig That's not the problem. The problem is one thread calling sslStream = null; then another thread calling sslStream.Dispose(); and throwing NullReferenceException.
jahmai
on 18 Dec 2019
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.
baulig
on 18 Dec 2019
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鈥檛 eliminate it.
jahmai
on 18 Dec 2019
I opened a PR adding some locking: https://github.com/mono/mono/pull/18250
akoeplinger
on 18 Dec 2019
@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.
pachecosoftware
on 19 Dec 2019
Thank you so much for this and my apologies for being so rude to both of you earlier on.
baulig
on 19 Dec 2019
@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.
akoeplinger
on 19 Dec 2019
Related issues
danroth27
路
39Comments
mandel-macaque
路
37Comments
filipnavara
路
29Comments
pettfre
路
37Comments
dbelcher
路
35Comments
Most helpful comment
@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.