SafeDeleteSslContext.WriteToConnection throws System.ArgumentNullException

[bentoi]

Hard to reproduce but our CI tests sporadically fail with:

Unhandled Exception: System.ArgumentNullException: Value cannot be null.
   at System.Threading.Monitor.ReliableEnter(Object obj, Boolean& lockTaken)
   at System.Net.SafeDeleteSslContext.WriteToConnection(Void* connection, Byte* data, Void** dataLength)
   at Interop.AppleCrypto.SslHandshake(SafeSslHandle sslHandle)
   at System.Net.Security.SslStreamPal.PerformHandshake(SafeSslHandle sslHandle)
   at System.Net.Security.SslStreamPal.HandshakeInternal(SafeFreeCredentials credential, SafeDeleteContext& context, SecurityBuffer inputBuffer, SecurityBuffer outputBuffer, Boolean isServer, Boolean remoteCertRequired, String targetName)

The dotnet process crashes after this. This occurs with a test where a client opens an SSL connection asynchronously and then closes it -- possibly while the handshake is still in progress.

The crash dump showing the stacks: stacks.txt

This occurs with both .NET Core Runtime 2.0.6 and 2.1-preview.

[danmoseley]

@bartonjs

[stephentoub]

Looks like we're locking on an object that gets nulled out in Dispose:
https://github.com/dotnet/corefx/blob/master/src/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs#L132
https://github.com/dotnet/corefx/blob/master/src/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs#L118

[karelz]

@rmkerr can you please take a look?

[bartonjs]

Since SafeHandle.IsClosed is only true when the reference count actually went to zero, the fix might be to guard the null-outs behind that value.

bool nullAllFields = false;

if (_sslContext != null)
{
    _sslContext.Dispose();
    nullAllFields = _sslContext.IsClosed;
    _sslContext = null;
}

if (nullAllFields)
{
    _toConnection = null;
    etc;
}

If !IsClosed then there's an outstanding P/Invoke to SSLRead or SSLWrite and therefore it's not safe to clean up the queues or the delegates. Just let them get (potentially) more slowly garbage collected.

Alternatively, never null out those fields and always let them take a potentially slower trip through garbage collection.

[stephentoub]

Alternatively, never null out those fields and always let them take a potentially slower trip through garbage collection.

I'd be inclined to keep it simple and just not null those out (still Dispose the context). It shouldn't measurably impact GC, and in general we don't promote nulling fields out during Dispose.

[rmkerr]

I think that not nulling out those fields sounds like a reasonable change. I'm in the process of setting up a mac now, and can test that fix as soon as I am done.

[rmkerr]

@stephentoub, @bartonjs, do you have any thoughts on testing this? I wasn't able to construct a repro to verify the change.

[bartonjs]

@rmkerr It'll be hard to make a repro that doesn't involve either freeze threads or introducing latency in the callbacks. Just removing all of the null-assignments in Dispose is sufficient to declare this fixed (by logical argument)

[rmkerr]

Great, thanks Jeremy. I'll open that PR now.