Consolidate _handshakeBuffer and _internalBuffer in SslStream

[rzikm]

Fixes #52037.

I the two buffers have been merged together since they are never used in parallel (i.e. receciving appdata is not allowed during renegotiation etc.).

One thing that is kinda open still is when to rent/return the actual buffer, currently, the code rents the buffer during construction and keeps it until the SslStream is disposed. Maybe we can do better.

Should probably undergo performance testing before merging.

[ghost]

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

Issue Details

---

Fixes #52037.

I the two buffers have been merged together since they are never used in parallel (i.e. receciving appdata is not allowed during renegotiation etc.).

Will need performance testing before merging.

Author:	rzikm
Assignees:	-
Labels:	area-System.Net.Security
Milestone:	-

[geoffkizer]

One thing that is kinda open still is when to rent/return the actual buffer, currently, the code rents the buffer during construction and keeps it until the SslStream is disposed.

We intentionally return the buffer at certain times in order to reduce memory usage. In particular, when the user does a zero-byte read.

@wfurt did a bunch of work to enable this not too long ago (I think it was in 6.0?). We should not regress this.

Edit: Actually, the work from @wfurt was to read from multiple TLS frames if we can. I think we already supported the zero-byte read optimization before that.

Regardless, here's the relevant code for zero byte read handling: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs#L913

[wfurt]

it generally looks good to me. I left few comments.

[wfurt]

padding seems little bit strange to me. I would probably just track where the encrypted chunk starts.

[rzikm]

It was a small optimization, by keeping the padding length we have one less field to update when we discard data read by the user code.

[wfurt]

I don't know if that matters but we used separate size for handshake to limit memory used. While during transfer it is very likely you have nore data and you would be receiving full TLS frames, handshake messages are typically pretty small.
Using ReadBufferSize makes them come from same pool e.g. it may increase efficiency.

[wfurt]

Since SslBuffer is struct, I'm wondering if we can just initialized automatically in constructor and here simply have function to allocate/resize/validate the actual buffer. -> EnsureAvailableSpace()

[rzikm]

Done

[wfurt]

I'm wondering if we could just use EnsureAvailableSpace here.

[rzikm]

It is expected that if no data is buffered, the _buffer is disposed. We would still have to new the inner ArrayBuffer internally in that case.

[wfurt]

I know this is exciting code. But I'm wondering now if there is possibility this could confuse some of the cleanup logic.

[rzikm]

I don't think so, since the data are read immediately afterwards and there are those locks that prevent multiple concurrent reads or other conflicting operations.

[wfurt]

I don't know if this matters but the else would put next decrypted data to the beginning of the buffer.
The encrypted should be always same or bigger so it may not matter.

[rzikm]

I think it does not matter, since ResetReadBuffer is called before receiving and that function moves the data if there is any.

[wfurt]

We should not need this, right? Since there is only one buffer now EnsureFullTlsFrameAsync should do it.

[rzikm]

Removed

[wfurt]

I don't think is same. Even if there is space in the buffer, whatever was left may be at the end ... limiting size of next IO. It would be interesting so how this impacts perf but if the chunk is small and copy fast the previous code would allow to fill the buffer completely.

[rzikm]

EnsureAvailableSpace moves the data to the beginning of the buffer if enough available space can be gained that way (see ArrayBuffer.EnsureAvailableSpace)

I haven't seen any significant change in perf benchmarks (changes were up to 2% in either direction)

[rzikm]

I have tried to copy the non-async-machine path from FillHandshakeBufferAsync to EnsureFullTlsFrameAsync, but I did not manage to get it working (the tests would hang for resons unknown to me). Any help with that would be appreciated:

WIP version

        private ValueTask<int> EnsureFullTlsFrameAsync<TIOAdapter>(TIOAdapter adapter)
            where TIOAdapter : IReadWriteAdapter
        {
            int frameSize;
            if (HaveFullTlsFrame(out frameSize))
            {
                return ValueTask.FromResult(frameSize);
            }

            // We may have enough space to complete frame, but we may still do extra IO if the frame is small.
            // So we will attempt larger read - that is trade of with extra copy.
            // This may be updated at some point based on size of existing chunk, rented buffer and size of 'buffer'.
            _buffer.EnsureAvailableSpace(Math.Min(frameSize, InitialBufferSize) - _buffer.ActiveLength);

            while (_buffer.EncryptedLength < frameSize)
            {
                // there should be space left to read into
                Debug.Assert(_buffer.AvailableLength > 0, "_buffer.AvailableBytes > 0");

                // We either don't have full frame or we don't have enough data to even determine the size.
                ValueTask<int> t = adapter.ReadAsync(_buffer.AvailableMemory);
                if (!t.IsCompletedSuccessfully)
                {
                    return InternalEnsureFullTlsFrameAsync(adapter, t, frameSize);
                }
                int bytesRead = t.Result;
                if (bytesRead == 0)
                {
                    if (_buffer.EncryptedLength != 0)
                    {
                        // we got EOF in middle of TLS frame. Treat that as error.
                        throw new IOException(SR.net_io_eof);
                    }

                    return ValueTask.FromResult(0);
                }

                _buffer.Commit(bytesRead);
                if (frameSize == int.MaxValue && _buffer.EncryptedLength > TlsFrameHelper.HeaderSize)
                {
                    // recalculate frame size if needed e.g. we could not get it before.
                    frameSize = GetFrameSize(_buffer.EncryptedReadOnlySpan);
                    _buffer.EnsureAvailableSpace(frameSize - _buffer.ActiveLength);
                }
            }

            return ValueTask.FromResult(frameSize);

            async ValueTask<int> InternalEnsureFullTlsFrameAsync(TIOAdapter adapter, ValueTask<int> task, int frameSize)
            {
                while (true)
                {
                    int bytesRead = await task.ConfigureAwait(false);
                    if (bytesRead == 0)
                    {
                        if (_buffer.EncryptedLength != 0)
                        {
                            // we got EOF in middle of TLS frame. Treat that as error.
                            throw new IOException(SR.net_io_eof);
                        }

                        return 0;
                    }

                    _buffer.Commit(bytesRead);

                    if (_buffer.EncryptedLength >= frameSize)
                    {
                        break;
                    }

                    task = adapter.ReadAsync(_buffer.AvailableMemory);

                    if (frameSize == int.MaxValue && _buffer.EncryptedLength > TlsFrameHelper.HeaderSize)
                    {
                        // recalculate frame size if needed e.g. we could not get it before.
                        frameSize = GetFrameSize(_buffer.EncryptedReadOnlySpan);
                        _buffer.EnsureAvailableSpace(frameSize - _buffer.ActiveLength);
                    }
                }

                return frameSize;
            }
        }

[wfurt]

I have tried to copy the non-async-machine path from FillHandshakeBufferAsync to EnsureFullTlsFrameAsync, but I did

I don't think we need it. We abandoned it EnsureFullTlsFrameAsync for simplification.

[rzikm]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[wfurt]

we should remove it if not used.

[wfurt]

LGTM.
I left one comment to fix.

[rzikm]

There is one possibly related test failure in outerloop tests:
System.Net.Security.Tests.CertificateValidationRemoteServer.CertificateValidationRemoteServer_EndToEnd_Ok
but I am unable to reproduce the failure locally (the test passes)

&{ pushd C:\source\runtime\artifacts\bin\System.Net.Security.Tests\Debug\net7.0-windows\
>>   &"C:\source\runtime\artifacts\bin\testhost\net7.0-windows-Debug-x64\dotnet.exe" exec --runtimeconfig System.Net.Security.Tests.runtimeconfig.json --depsfile System.Net.Security.Tests.deps.json xunit.console.dll System.Net.Security.Tests.dll -xml testResults.xml -nologo -method System.Net.Security.Tests.CertificateValidationRemoteServer.CertificateValidationRemoteServer_EndToEnd_Ok -notrait category=failing
>>   popd }
  Discovering: System.Net.Security.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Net.Security.Tests (found 1 of 474 test case)
  Starting:    System.Net.Security.Tests (parallel test collections = on, max threads = 20)
  Finished:    System.Net.Security.Tests
=== TEST EXECUTION SUMMARY ===
   System.Net.Security.Tests  Total: 2, Errors: 0, Failed: 0, Skipped: 0, Time: 1.381s

[rzikm]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[rzikm]

CI failures are unrelated:

• System.Threading.ThreadPools.Tests.ThreadPoolTests.CooperativeBlockingCanCreateThreadsFaster - ThreadPoolTests.CooperativeBlockingCanCreateThreadsFaster timed out #64964
• System.Net.Security.Tests.TelemetryTest.EventSource_SuccessfulHandshake_LogsStartStop - EventSource_SuccessfulHandshake_LogsStartStop failures on Windows11 #63008
• System.Net.NetworkInformation.Tests.PingTest.SendPingToExternalHostWithLowTtlTest - System.Net.NetworkInformation.Tests.PingTest.SendPingToExternalHostWithLowTtlTest Failures #65065