Skip to content

delay disposal of array buffres if operation is in progress #49657

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wfurt wants to merge 2 commits into from
Closed

delay disposal of array buffres if operation is in progress #49657

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dae5522
delay disposal of array buffres if operation is in progress
wfurt Mar 15, 2021
f62248c
feedback from review
wfurt Mar 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 78 additions & 6 deletions src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Net.Security;
using System.Security.Authentication;
using System.Security.Cryptography.X509Certificates;
using System.Threading;
using Microsoft.Win32.SafeHandles;

namespace System.Net
Expand All @@ -19,6 +20,13 @@ internal sealed class SafeDeleteSslContext : SafeDeleteContext
private Interop.AppleCrypto.SSLWriteFunc _writeCallback;
private ArrayBuffer _inputBuffer = new ArrayBuffer(InitialBufferSize);
private ArrayBuffer _outputBuffer = new ArrayBuffer(InitialBufferSize);
// state values for the _pendingXXX varaibles bellow
private const int NoIOPending = 0;
private const int NativeIOPending =1;
private const int ManagedIOPending = 2;
private const int Disposed = 3;
private int _pendingInput;
private int _pendingOutput;

public SafeSslHandle SslContext => _sslContext;

Expand Down Expand Up @@ -144,8 +152,16 @@ protected override void Dispose(bool disposing)
SafeSslHandle sslContext = _sslContext;
if (null != sslContext)
{
_inputBuffer.Dispose();
_outputBuffer.Dispose();
if (Interlocked.Exchange(ref _pendingInput, Disposed) == NoIOPending)
{
_inputBuffer.Dispose();
}

if (Interlocked.Exchange(ref _pendingOutput, Disposed) == NoIOPending)
{
_outputBuffer.Dispose();
}
Copy link
Member

@stephentoub stephentoub Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to wrap ArrayBuffer in a ReservableArrayBuffer, or something like that? Then it would have methods like Rent/Return/Dispose, which would internally do the right synchronization on a ref count or such field.


sslContext.Dispose();
Copy link
Member

@stephentoub stephentoub Mar 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this PR is focused on on the buffers, but what does it mean to dispose of the sslContext while that's still in use as well? Is all inappropriate use there handled by this being a SafeHandle, such that actual release of the underlying handle is delayed until all interop with the handle completes (typical SafeHandle stuff)?

Copy link
Member Author

@wfurt wfurt Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can take a look. There was previous discussion about using SafeHandles in SslStream. Perhaps that can go together. The buffer part is regression caused by #32338.

}
}
Expand All @@ -158,13 +174,24 @@ private unsafe int WriteToConnection(void* connection, byte* data, void** dataLe
ulong length = (ulong)*dataLength;
Debug.Assert(length <= int.MaxValue);

if (Interlocked.Exchange(ref _pendingOutput, NativeIOPending) == Disposed)
Copy link
Member

@stephentoub stephentoub Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this use CompareExchange?

Copy link
Member Author

@wfurt wfurt Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really want to prevent Dispose from running and we want to detect in IO functions what dispose was called to run deferred cleanup. We don't really need to distinguish what is running.

Now looking at this again. It is not safe to manipulate the ArrayBuffer concurrently. We would need lock here to prevent corruption. But the code works because the general flow is like:

  • we append received frame to queue.
  • we call native code and that consumers the bytes
  • it can write more data to the other queue
  • we check the queue when finish and we send data to underlying stream

If we go with ReservableArrayBuffer this would turn into counter as you said.
I like it better than the custom state machine so I'll make change as you recommended.

{
const int writErr = -20;
Copy link
Member

@stephentoub stephentoub Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this -20 and the below -19 numbers come from? Are those arbitrary error values, or do they map to something else?

Copy link
Member Author

@wfurt wfurt Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they are osStatus enums
https://www.osstatus.com/search/results?platform=all&framework=all&search=-19

same as noErr below.

Copy link
Member

@stephentoub stephentoub Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename then to const int OSStatus_writErr or something like that?

Or maybe introduce an OSStatus enum somewhere under interop?

Copy link
Member Author

@wfurt wfurt Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this does not show up in other places as we map the actual values in PAL via

runtime/src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/pal_ssl.c

Lines 278 to 280 in d6ea28d

static PAL_TlsIo OSStatusToPAL_TlsIo(OSStatus status)
{
switch (status)

Looking at this again, I can probably use errSSLClosedAbort = -9806 for both and move it up to top (with noErr) and add comment to it. And use the OSStatus_ prefix. Since this is disposed, I don't think the actually value matters that much.

return writErr;
}

int toWrite = (int)length;
var inputBuffer = new ReadOnlySpan<byte>(data, toWrite);

_outputBuffer.EnsureAvailableSpace(toWrite);
inputBuffer.CopyTo(_outputBuffer.AvailableSpan);
_outputBuffer.Commit(toWrite);

if (Interlocked.CompareExchange(ref _pendingOutput, NoIOPending, NativeIOPending) == Disposed)
{
_outputBuffer.Dispose();
}

// Since we can enqueue everything, no need to re-assign *dataLength.
const int noErr = 0;
return noErr;
Expand All @@ -181,10 +208,21 @@ private unsafe int ReadFromConnection(void* connection, byte* data, void** dataL
return noErr;
}

if (Interlocked.Exchange(ref _pendingInput, NativeIOPending) == Disposed)
Copy link
Member

@stephentoub stephentoub Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this use CompareExchange?

{
const int readErr = -19;
return readErr;
}

uint transferred = 0;

if (_inputBuffer.ActiveLength == 0)
{
if (Interlocked.Exchange(ref _pendingInput, NoIOPending) == Disposed)
Copy link
Member

@stephentoub stephentoub Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this use CompareExchange?

I'll stop commenting on these. Basically, all the ones that are comparing against Disposed seem... suspicious.

{
_inputBuffer.Dispose();
}

*dataLength = (void*)0;
return errSSLWouldBlock;
}
Expand All @@ -195,28 +233,52 @@ private unsafe int ReadFromConnection(void* connection, byte* data, void** dataL
_inputBuffer.Discard(limit);
transferred = (uint)limit;

if (Interlocked.Exchange(ref _pendingInput, NoIOPending) == Disposed)
{
_inputBuffer.Dispose();
}

*dataLength = (void*)transferred;
return noErr;
}

internal void Write(ReadOnlySpan<byte> buf)
{
if (Interlocked.Exchange(ref _pendingInput, ManagedIOPending) == Disposed)
{
throw new ObjectDisposedException(nameof(SafeDeleteSslContext));
}

_inputBuffer.EnsureAvailableSpace(buf.Length);
buf.CopyTo(_inputBuffer.AvailableSpan);
_inputBuffer.Commit(buf.Length);

if (Interlocked.CompareExchange(ref _pendingInput, NoIOPending, ManagedIOPending) == Disposed)
{
_inputBuffer.Dispose();
}
}

internal int BytesReadyForConnection => _outputBuffer.ActiveLength;

internal byte[]? ReadPendingWrites()
{
if (_outputBuffer.ActiveLength == 0)
if (Interlocked.Exchange(ref _pendingOutput, ManagedIOPending) == Disposed)
{
return null;
throw new ObjectDisposedException(nameof(SafeDeleteSslContext));
}

byte[] buffer = _outputBuffer.ActiveSpan.ToArray();
_outputBuffer.Discard(_outputBuffer.ActiveLength);
byte[]? buffer = null;
if (_outputBuffer.ActiveLength != 0)
{
buffer = _outputBuffer.ActiveSpan.ToArray();
_outputBuffer.Discard(_outputBuffer.ActiveLength);
}

if (Interlocked.CompareExchange(ref _pendingOutput, NoIOPending, ManagedIOPending) == Disposed)
{
_outputBuffer.Dispose();
}

return buffer;
}
Expand All @@ -228,11 +290,21 @@ internal int ReadPendingWrites(byte[] buf, int offset, int count)
Debug.Assert(count >= 0);
Debug.Assert(count <= buf.Length - offset);

if (Interlocked.Exchange(ref _pendingOutput, ManagedIOPending) == Disposed)
{
throw new ObjectDisposedException(nameof(SafeDeleteSslContext));
}

int limit = Math.Min(count, _outputBuffer.ActiveLength);

_outputBuffer.ActiveSpan.Slice(0, limit).CopyTo(new Span<byte>(buf, offset, limit));
_outputBuffer.Discard(limit);

if (Interlocked.CompareExchange(ref _pendingInput, NoIOPending, ManagedIOPending) == Disposed)
{
_outputBuffer.Dispose();
}

return limit;
}

Expand Down