improve efficiency and correctness of SSL handshake #1949

wfurt · 2020-01-21T02:33:42Z

This is replacement for #1350. Instead of using shared buffer to hold data, it turns ProtocolToken to readOnly structure. Aside from allocation count there was no significant benefit of using shared buffer so I abandoned that part.

As part of #453 we start using _internalBuffer to read data for handshake. That was not done quite right as if SslStream is disposed while doing handshake, we may cause NRE. (#1291)
I could add locking like the other places but that complicates renegotiation - and that is common case for TLS1.3. So address feedback and improve stability I reused ArrayBuffers from HTTP2.
It pretty much does what I wanted, it can also optionally use shared buffers and it can grow buffer as needed. It also provides nice Span and Memory view on data.

To take advantage of it I added more Spanification to where #453 left it off. Instead of using ArraySegment I pushed ReadOnlySpan through Windows PAL in way similar to other platforms.

While this change is focused on Async path, I plan to come back and cleanup and unify with synchronous path. This also does not address locking issues discovered while testing TLS1.3.
Keeping negotiation buffers separate should hopefully help.

Wraith2 · 2020-01-22T12:48:03Z

src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs

            try
            {
-                Span<Interop.SspiCli.SecBuffer> inUnmanagedBuffer = stackalloc Interop.SspiCli.SecBuffer[inSecurityBufferDescriptor.cBuffers];
+                Span<Interop.SspiCli.SecBuffer> inUnmanagedBuffer = stackalloc Interop.SspiCli.SecBuffer[inSecBuffers.Count];


I believe the intention is for the stackalloc to be 1..3 entries long but there's no assertion or check that i can see and since count is unconditionally increased in SetNextBuffer it could in theory any value if someone found a way to abuse it.

Can you share an example of where/when it wouldn't be a small value?

Nothing obvious, as i said the intention is clearly that it should be 1..3 which causes no issue. I just thought it might be worth an assert or check.

I see. Sure, adding an assert would be fine.

There is Assert in SetNextBuffer so it won't go more than 3. I can also make it consistent and always allocate 3 since the rest of the logic is fixed anyway. I did not find a good way how to make the equivalent of fixed() in a more flexible way.

Not sure if hot path but having a constant-sized stack via (stackalloc Interop.SspiCli.SecBuffer[3])[0..inSecBuffers.Count] may use fewer instructions, if you do have this upper bound.

Right, if the max is actually 3, just use 3. The JIT is better at optimizing const-sized stackallocs. There's also no reason to slice it beyond that unless you actually use the length later. If you do use the length later, just stick with the variable-sized stackalloc.

I updated code to use const-sized allocation since the difference between 0 and 3 is really small. (and will be 2 in most cases)
I was wondering if '3' should be some named constant instead of magic number but I not sure what the name should be and where it should live.

wfurt · 2020-01-22T20:31:26Z

I pushed an update to fix another regression caused by #453. Renegotiation can happen in the synchronous path and in that case current code would hit assert. To fix that I plummed adapter through the new code path and updated the existing test to cover that case.

wfurt · 2020-01-22T20:33:09Z

contributes to #1720

…hake2

src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs

bartonjs · 2020-01-23T18:32:12Z

src/libraries/Common/src/System/Net/Security/SecurityBuffer.Windows.cs

+        }
+    }
+
+    [StructLayout(LayoutKind.Auto)]


Why Auto? (It's unusual to see this attribute, so I'm curious)

I copy that from the original SecurityBuffer as the intention was the same.

[StructLayout(LayoutKind.Auto)] internal struct SecurityBuffer internal struct SecurityBuffer

We generally use it when we may not have chosen the optimal layout for fields and want to allow the runtime to do so for us. That said, it's most beneficial when the type is going to end up on the heap, either on its own or as part of an array, such that you want to minimize the overall allocation size. But this is a ref struct... it'll never be on the heap.

src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs

src/libraries/Common/src/System/Net/ArrayBuffer.cs

src/libraries/Common/src/System/Net/Security/SecurityBuffer.Windows.cs

wfurt · 2020-01-24T09:08:32Z

all failing tests have no console output. This can be a Helix issue.
cc: @MattGal

MattGal · 2020-01-27T18:05:32Z

all failing tests have no console output. This can be a Helix issue.
cc: @MattGal

When I'm OOF it's good to tag @dotnet/dnceng . Looking at this now, hopefully logs are still there...

MattGal · 2020-01-27T18:13:43Z

@wfurt these are crashes where the file permissions of the core dumps being produced are somehow incorrect, resulting in "[Errno 18] Invalid cross-device link".

I filed https://github.com/dotnet/core-eng/issues/8662 to track this. Unfortunately the core dumps here are likely lost as the harness crashed trying to move them around on disk.

src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs

src/libraries/Common/src/System/Net/Security/SecurityBuffer.Windows.cs

…ndshake2

wfurt · 2020-02-04T08:16:19Z

All your concerns should be resolved @stephentoub. This is ready for another review round. The last updates bring the use of the IO Adapter as well as a fix for another regression caused by #453.

src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs

stephentoub · 2020-02-09T03:59:32Z

src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs

@@ -431,34 +431,38 @@ internal abstract partial class SafeDeleteContext : SafeHandle
            SafeFreeContextBuffer outFreeContextBuffer = null;
            try
            {
-                Span<Interop.SspiCli.SecBuffer> inUnmanagedBuffer = stackalloc Interop.SspiCli.SecBuffer[inSecurityBufferDescriptor.cBuffers];
-                inUnmanagedBuffer.Clear();
+                // Allocate always maximum to allow better code optimization.


Nit: comment isn't necessary

stephentoub · 2020-02-09T04:01:17Z

src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs

-                inUnmanagedBuffer.Clear();
+                // Allocate always maximum to allow better code optimization.
+                Span<Interop.SspiCli.SecBuffer> inUnmanagedBuffer = stackalloc Interop.SspiCli.SecBuffer[3];
+                for (int index = 0; index < inSecBuffers.Count; ++index)


Can we assert anything about the relationship between inSecBuffers.Count and 3?

stephentoub · 2020-02-09T04:03:16Z

src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs

+
+                    if (inSecBuffers.Count > 0 && inUnmanagedBuffer[0].pvBuffer == IntPtr.Zero)
+                    {
+                        inUnmanagedBuffer[0].pvBuffer = (IntPtr)pinnedToken0;


Wouldn't it be easier to move the earlier block that created and filled in the span to here? They wouldn't need to first assign IntPtr.Zero and then check for IntPtr.Zero, you'd just assign the right value to begin with (either the UnmanagedToken's handle or the pinned pointer).

stephentoub · 2020-02-09T04:07:01Z

src/libraries/Common/src/System/Net/Security/SecurityBuffer.Windows.cs

+        internal InputSecurityBuffer GetBuffer(int index)
+        {
+            Debug.Assert(index < 3);
+            return index == 0 ? _item0 : index == 1 ? _item1 : _item2;


It might help perf-wise if GetBuffer returned ref InputSecurityBuffer rather than InputSecurityBuffer. Could also be investigated subsequently.

stephentoub · 2020-02-09T04:10:16Z

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

+                if (result == null || result.IsCompleted)
+                {
+                    // Operation has completed.
+                    _handshakeBuffer.Dispose();


If result completes in failure/cancellation, this could end up running concurrently with the _handshakeBuffer.Dispose; _nestedAuth = 0; in the ContinueWith callback. That seems wrong, no?

stephentoub · 2020-02-09T04:13:22Z

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

+                    else
+                    {
+                        await InnerStream.WriteAsync(message.Payload, readAdapter.CancellationToken).ConfigureAwait(false);
+                    }


I don't understand why this if/else block is necessary. Why can't you just use the adapter to do the write? Is the issue that you have a read adapter but you need a write adapter? If so, you can't pass a write adapter in here?

Maybe the read and write adapters should be combined, so that we just have an SslSyncAdapter and an SslAsyncAdapter, each of which has both the read and write methods on it?

yes, combing them together would do the trick.

stephentoub · 2020-02-09T04:17:22Z

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

+                    int bytesRead = await task.ConfigureAwait(false);
+                    if (bytesRead == 0)
+                    {
+                        throw new IOException(SR.net_io_eof);


Why does this throw for EOF but the fast-path earlier successfully returns 0 for it? That means there's a race condition, where it may return 0 or may throw for EOF depending on how quickly the EOF is received.

stephentoub

I realize this has been in the works for a while, but the change is making me nervous, in large part because I'm having trouble following what's necessary for correctness and what's being done for perf optimizations. Can we split this into two pieces, one that's purely about fixing the assert we've been seeing in CI and making the implementation correct, and a second that follows that up that tries to improve perf / reduce allocation?

wfurt · 2020-02-09T04:22:07Z

yes, I can do that. It increased over time but I think it can be separated into smaller chunks.

stephentoub · 2020-02-09T04:30:18Z

Thanks.

wfurt · 2020-02-13T19:40:24Z

will be replaced by smaller changes.

wfurt added 5 commits January 17, 2020 01:39

snapshot

181ea8d

update

8157f40

osx

1e476b6

windows update

5d90789

remove dead code

a2ec343

wfurt added the area-System.Net.Security label Jan 21, 2020

wfurt self-assigned this Jan 21, 2020

wfurt added 2 commits January 21, 2020 11:27

fix http unit tests

b3a89db

remove dead code

2aeb19b

Wraith2 reviewed Jan 22, 2020

View reviewed changes

fix renegotiation and TLS1.3 improvements

1fcd5b3

Merge branch 'master' of https://github.com/dotnet/runtime into hands…

4647886

…hake2

samsosa mentioned this pull request Jan 22, 2020

HTTPs and "Connection: close" - huge performance degradation on windows dotnet/aspnetcore#18488

Closed

wfurt requested review from bartonjs and a team January 23, 2020 17:46

scalablecory reviewed Jan 23, 2020

View reviewed changes

src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs Outdated Show resolved Hide resolved

bartonjs reviewed Jan 23, 2020

View reviewed changes

scalablecory reviewed Jan 23, 2020

View reviewed changes

src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs Outdated Show resolved Hide resolved

scalablecory reviewed Jan 23, 2020

View reviewed changes

src/libraries/Common/src/System/Net/ArrayBuffer.cs Outdated Show resolved Hide resolved

scalablecory reviewed Jan 23, 2020

View reviewed changes

src/libraries/Common/src/System/Net/Security/SecurityBuffer.Windows.cs Show resolved Hide resolved

fix extra data

b57f102

wfurt changed the title ~~WIP: improve efficiency and correctness of SSL handshake~~ improve efficiency and correctness of SSL handshake Jan 24, 2020

stephentoub reviewed Feb 1, 2020

View reviewed changes

src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs Outdated Show resolved Hide resolved

stephentoub reviewed Feb 1, 2020

View reviewed changes

src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs Outdated Show resolved Hide resolved

stephentoub reviewed Feb 1, 2020

View reviewed changes

src/libraries/Common/src/System/Net/Security/SecurityBuffer.Windows.cs Show resolved Hide resolved

wfurt added 7 commits February 3, 2020 12:35

feedback from review

4ef1590

Merge branch 'handshake2' of https://github.com/wfurt/runtime into ha…

253a61f

…ndshake2

Merge branch 'handshake2' of https://github.com/wfurt/runtime into ha…

56caff2

…ndshake2

add missing line

322b853

Merge branch 'handshake2' of https://github.com/wfurt/runtime into ha…

7ee46a8

…ndshake2

fix two more places with missing ReadOnlySpan

e1cc5f7

fix handling of ForceAuthenticationAsync completion

71caee3

halter73 reviewed Feb 4, 2020

View reviewed changes

src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs Outdated Show resolved Hide resolved

remove unused variable

dc68696

wfurt mentioned this pull request Feb 9, 2020

AF: System.Net.Security.SslStream.ConsumeBufferedBytes (Linux/OSX x64 Debug) #20507

Closed

stephentoub closed this Feb 9, 2020

stephentoub reopened this Feb 9, 2020

stephentoub reviewed Feb 9, 2020

View reviewed changes

This was referenced Feb 10, 2020

use TestHelper to create client and server stream #32007

Merged

pass Span through Windows PAL #32008

Merged

use proper IO in ssl handshake #32013

Merged

wfurt closed this Feb 13, 2020

karelz added this to the 5.0.0 milestone Aug 18, 2020

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve efficiency and correctness of SSL handshake #1949

improve efficiency and correctness of SSL handshake #1949

wfurt commented Jan 21, 2020

Wraith2 Jan 22, 2020

stephentoub Jan 22, 2020

Wraith2 Jan 22, 2020

stephentoub Jan 22, 2020

wfurt Jan 22, 2020

scalablecory Jan 23, 2020

stephentoub Feb 1, 2020

wfurt Feb 3, 2020

wfurt commented Jan 22, 2020

wfurt commented Jan 22, 2020

bartonjs Jan 23, 2020

wfurt Jan 23, 2020

stephentoub Feb 1, 2020 •

edited

Loading

wfurt commented Jan 24, 2020

MattGal commented Jan 27, 2020

MattGal commented Jan 27, 2020

wfurt commented Feb 4, 2020

stephentoub Feb 9, 2020

stephentoub Feb 9, 2020 •

edited

Loading

stephentoub Feb 9, 2020

stephentoub Feb 9, 2020 •

edited

Loading

stephentoub Feb 9, 2020

stephentoub Feb 9, 2020

wfurt Feb 9, 2020

stephentoub Feb 9, 2020

stephentoub left a comment

wfurt commented Feb 9, 2020

stephentoub commented Feb 9, 2020

wfurt commented Feb 13, 2020

improve efficiency and correctness of SSL handshake #1949

improve efficiency and correctness of SSL handshake #1949

Conversation

wfurt commented Jan 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wfurt commented Jan 22, 2020

wfurt commented Jan 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Feb 1, 2020 • edited Loading

Choose a reason for hiding this comment

wfurt commented Jan 24, 2020

MattGal commented Jan 27, 2020

MattGal commented Jan 27, 2020

wfurt commented Feb 4, 2020

Choose a reason for hiding this comment

stephentoub Feb 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Feb 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

wfurt commented Feb 9, 2020

stephentoub commented Feb 9, 2020

wfurt commented Feb 13, 2020

stephentoub Feb 1, 2020 •

edited

Loading

stephentoub Feb 9, 2020 •

edited

Loading

stephentoub Feb 9, 2020 •

edited

Loading