catch exceptions in callbacks from native code on OSX in SafeDeleteSslContext

[wfurt]

This is alternative to #49657. Based on the feedback, I tried different approach and this is simple, scoped and effective - I did several hundred run (with #49573 clearing) on the whole test set and I did not see any problem. The two functions are called from native OS code and if we throw there is no good way to handle that. With this change, we simply return error code and that will eventually bubble up in OSX Pal.

I think longer term we will need to change SslStream to avoid using SafeHandle after dispose to address #28171 and concerns from #47944. That will be separate change independent of the OSX specific handling.

fixes #49600
fixes #43686
contributes to #49573

[ghost]

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This is alternative to #49657. Based on the feedback, I tried different approach and this is simple, scoped and effective - I did several hundred run (with #49573 clearing) on the whole test set and I did not see any problem. The two functions are called from native OS code and if we throw there is no good way to handle that. With this change, we simply return error code and that will eventually bubble up in OSX Pal.

I think longer term we will need to change SslStream to avoid using SafeHandle after dispose to address #28171 and concerns from #47944. That will be separate change independent of the OSX specific handling.

fixes #49600
fixes #43686
contributes to #49573

Author:	wfurt
Assignees:	wfurt
Labels:	area-System.Net.Security, os-mac-os-x
Milestone:	-

[geoffkizer]

This approach seems fine as long as we are never pooling the buffers here. It doesn't look like we are currently, but can we at least add a comment somewhere about this?

[geoffkizer]

It seems weird to me that we are managing input and output buffers on the SafeHandle itself here. Ideally we would just use the buffers on SslStream itself.

But that's beyond the scope of this PR. And this is MacOS anyway so I'm not too worried about the perf/memory usage here.

[wfurt]

It seems weird to me that we are managing input and output buffers on the SafeHandle itself here. Ideally we would just use the buffers on SslStream itself.

In case of Linux, this is handled inside input/output BIO. We write to it and then the data disappear into unmanaged space.
For Windows we pass buffers directly so there is no queuing.

[davidfowl]

@geoffkizer is this OK?

[geoffkizer]

Can we add a quick comment somewhere re this?

This approach seems fine as long as we are never pooling the buffers here. It doesn't look like we are currently, but can we at least add a comment somewhere about this?

Aside from that it looks fine to me.

[wfurt]

Do you mean comment about not throwing in native callback or more specifically about the buffers?

[geoffkizer]

Specifically about the buffers. Just a comment along the lines of this:

"We don't pool these buffers and we can't because there's a race between their use in the native read/write callbacks and being disposed when the SafeHandle is disposed. This race is benign currently, but if we were to pool the buffers we would have a potential use-after-free issue."