From 8074757b1d03fd4dfe1b08f25017362d39d13d0f Mon Sep 17 00:00:00 2001 From: wfurt Date: Sat, 20 Mar 2021 13:35:32 -0700 Subject: [PATCH 1/4] catch exceptions in callbacks from native code --- .../Security/Pal.OSX/SafeDeleteSslContext.cs | 74 +++++++++++-------- 1 file changed, 45 insertions(+), 29 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs index 06ecc8d9ed1d72..9bbb59064f3168 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs @@ -14,6 +14,11 @@ namespace System.Net internal sealed class SafeDeleteSslContext : SafeDeleteContext { private const int InitialBufferSize = 2048; + // mapped from OSX error codes + private const int OSStatus_writErr = -20; + private const int OSStatus_readErr = -19; + private const int OSStatus_noErr =0; + private const int OSStatus_errSSLWouldBlock = -9803; private SafeSslHandle _sslContext; private Interop.AppleCrypto.SSLReadFunc _readCallback; private Interop.AppleCrypto.SSLWriteFunc _writeCallback; @@ -155,48 +160,59 @@ protected override void Dispose(bool disposing) private unsafe int WriteToConnection(void* connection, byte* data, void** dataLength) { - ulong length = (ulong)*dataLength; - Debug.Assert(length <= int.MaxValue); + try + { + ulong length = (ulong)*dataLength; + Debug.Assert(length <= int.MaxValue); - int toWrite = (int)length; - var inputBuffer = new ReadOnlySpan(data, toWrite); + int toWrite = (int)length; + var inputBuffer = new ReadOnlySpan(data, toWrite); - _outputBuffer.EnsureAvailableSpace(toWrite); - inputBuffer.CopyTo(_outputBuffer.AvailableSpan); - _outputBuffer.Commit(toWrite); + _outputBuffer.EnsureAvailableSpace(toWrite); + inputBuffer.CopyTo(_outputBuffer.AvailableSpan); + _outputBuffer.Commit(toWrite); + // Since we can enqueue everything, no need to re-assign *dataLength. - // Since we can enqueue everything, no need to re-assign *dataLength. - const int noErr = 0; - return noErr; + return OSStatus_noErr; + } + catch + { + return OSStatus_writErr; + } } private unsafe int ReadFromConnection(void* connection, byte* data, void** dataLength) { - const int noErr = 0; - const int errSSLWouldBlock = -9803; - ulong toRead = (ulong)*dataLength; - - if (toRead == 0) + try { - return noErr; - } + ulong toRead = (ulong)*dataLength; + + if (toRead == 0) + { + return OSStatus_noErr; + } - uint transferred = 0; + uint transferred = 0; - if (_inputBuffer.ActiveLength == 0) - { - *dataLength = (void*)0; - return errSSLWouldBlock; - } + if (_inputBuffer.ActiveLength == 0) + { + *dataLength = (void*)0; + return OSStatus_errSSLWouldBlock; + } - int limit = Math.Min((int)toRead, _inputBuffer.ActiveLength); + int limit = Math.Min((int)toRead, _inputBuffer.ActiveLength); - _inputBuffer.ActiveSpan.Slice(0, limit).CopyTo(new Span(data, limit)); - _inputBuffer.Discard(limit); - transferred = (uint)limit; + _inputBuffer.ActiveSpan.Slice(0, limit).CopyTo(new Span(data, limit)); + _inputBuffer.Discard(limit); + transferred = (uint)limit; - *dataLength = (void*)transferred; - return noErr; + *dataLength = (void*)transferred; + return OSStatus_noErr; + } + catch + { + return OSStatus_readErr; + } } internal void Write(ReadOnlySpan buf) From 19332536ab819821fb39894636accd6e1c4cdfeb Mon Sep 17 00:00:00 2001 From: wfurt Date: Sat, 20 Mar 2021 13:48:35 -0700 Subject: [PATCH 2/4] style update --- .../src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs index 9bbb59064f3168..1f179be6e4d71c 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs @@ -13,12 +13,12 @@ namespace System.Net { internal sealed class SafeDeleteSslContext : SafeDeleteContext { - private const int InitialBufferSize = 2048; // mapped from OSX error codes private const int OSStatus_writErr = -20; private const int OSStatus_readErr = -19; - private const int OSStatus_noErr =0; + private const int OSStatus_noErr = 0; private const int OSStatus_errSSLWouldBlock = -9803; + private const int InitialBufferSize = 2048; private SafeSslHandle _sslContext; private Interop.AppleCrypto.SSLReadFunc _readCallback; private Interop.AppleCrypto.SSLWriteFunc _writeCallback; From 660b70b8f377741c2d6341ed08f62eb96c48a1e0 Mon Sep 17 00:00:00 2001 From: wfurt Date: Tue, 23 Mar 2021 16:27:51 -0700 Subject: [PATCH 3/4] add traces for failures --- .../System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs index 1f179be6e4d71c..759569ba08273f 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs @@ -175,8 +175,10 @@ private unsafe int WriteToConnection(void* connection, byte* data, void** dataLe return OSStatus_noErr; } - catch + catch (Exception e) { + if (NetEventSource.Log.IsEnabled()) + NetEventSource.Error(this, $"WritingToConnection failed: {e.Message}"); return OSStatus_writErr; } } @@ -209,8 +211,10 @@ private unsafe int ReadFromConnection(void* connection, byte* data, void** dataL *dataLength = (void*)transferred; return OSStatus_noErr; } - catch + catch (Exception e) { + if (NetEventSource.Log.IsEnabled()) + NetEventSource.Error(this, $"ReadFromConnectionfailed: {e.Message}"); return OSStatus_readErr; } } From d898cafb26253bca7fdcda03b3e55b420dc6566e Mon Sep 17 00:00:00 2001 From: wfurt Date: Sat, 27 Mar 2021 16:32:06 -0700 Subject: [PATCH 4/4] add comment --- .../src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs index 759569ba08273f..44fc356c33668a 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs @@ -160,6 +160,9 @@ protected override void Dispose(bool disposing) private unsafe int WriteToConnection(void* connection, byte* data, void** dataLength) { + // We don't pool these buffers and we can't because there's a race between their us 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. try { ulong length = (ulong)*dataLength;