SSLStream Fixing GC Hole by Drawaes · Pull Request #24799 · dotnet/corefx

Drawaes · 2017-10-22T15:49:04Z

No description provided.

Drawaes · 2017-10-22T16:05:47Z

@dotnet-bot Test Outerloop Linux x64 Release Build

Drawaes · 2017-10-22T16:39:59Z

@dotnet-bottest Outerloop Linux x64 Debug Build

Drawaes · 2017-10-22T17:56:04Z

fixes #24775 #24722
/cc @stephentoub @jkotas @Priya91

Drawaes · 2017-10-22T17:57:47Z

Failing tests are consistent

ALPN on Debian 90
And a IPV4 - IPV6 on Ubuntu 17.04

No crashes or segfaults I can see anymore.

Drawaes · 2017-10-22T18:01:42Z

/cc @danmosemsft
I think this solves the issue we have been seeing across the repo

jkotas · 2017-10-22T19:12:43Z

cc @janvorli

stephentoub · 2017-10-22T19:40:15Z

-            fixed (byte* sp = server)
-            {
-                return Interop.Ssl.SslSelectNextProto(out outp, out outlen, (IntPtr)sp, (uint)server.Length, inp, inlen) == Interop.Ssl.OPENSSL_NPN_NEGOTIATED ?
+            return Interop.Ssl.SslSelectNextProto(out outp, out outlen, (IntPtr)protocols.AddrOfPinnedObject(), (uint)server.Length, inp, inlen) == Interop.Ssl.OPENSSL_NPN_NEGOTIATED ?


Is the issue then that SslSelectNextProto uses the passed in buffer beyond the end of the synchronous call to the method?

Yes exactly that ^^^^

Actually not exactly that... its that you pass it back out with the "out IntPtr"

I see. The docs say:

The out value will point into either server or client, so it should be copied immediately.

This change ensures the server input is appropriately immovable. Is that already true for the client input?

Yes because the client input comes from OpenSSL this method is only called on the server side. As we don't store this but retrieve the value later from OpenSSL itself the lifetime of the client buffer is fine.

stephentoub · 2017-10-22T19:41:20Z

+            }
+
            GCHandle protocols = GCHandle.FromIntPtr(arg);
+            if (!protocols.IsAllocated || protocols.Target == null)


In what situation would it be freed? If there's a race condition where it could be freed before this callback, is it possible it could be freed during?

Well not anymore, before there was, I could remove that code in theory. I actually put it in, in my first round of trying to find the issue, thought it was worth leaving in for now. A Debug.Assert might suffice, which was my original idea.

#24389 (comment)

stephentoub · 2017-10-22T19:41:51Z

            SslGetAlpnSelected(ssl, out protocol, out len);

-            if (len == 0)
+            if (len < 1)


len might be negative?

who knows its coming from unmanaged code, I was merely adding extra safety. It might be worth me explaining the hole

@Priya91, CryptoNative_SslGet0AlpnSelected should be initializing len always; any of our shims that are used with out vars should initialize the value to the default. Otherwise the calling C# code using out might end up with garbage in the value yet C# will allow the value to be used.

@Drawaes, I don't think this len < 1 is correct or the right fix. The way the native shim function is currently coded, if for example HAVE_OPENSSL_ALPN isn't defined or if !API_EXISTS(SSL_get0_alpn_selected), the P/Invoke is a nop and len won't be modified, leaving it at whatever garbage was in len from the stack here, which means len could be anything, including a garbage value >=1. The right fix would seem to be to ensure that CryptoNative_SslGet0AlpnSelected initializes len to 0 if it's not calling SSL_get0_alpn_selected, and then this call site can return to a check for == 0.

@Drawaes Will you be making this change in this PR?

Drawaes · 2017-10-22T19:44:47Z

The issue was this

Unmanaged code calls the callback -> callback fixes the alpn buffer and calls the unmanaged select ALPN method -> this method just returns a POINTER into the server buffer and length....

Finally the Managed callback returns this pointer ... this means that the unmanaged code now has a interior pointer into a managed buffer that is no longer fixed.

stephentoub · 2017-10-22T19:47:21Z

-                if (_sslAuthenticationOptions.AlpnProtocolsHandle.IsAllocated)
-                {
-                    _sslAuthenticationOptions.AlpnProtocolsHandle.Free();
-                }


Is the Close method that contains the remaining Free always going to be called? e.g. is it called by both a Dispose/Close method and a finalizer?

I believe its part of an Handle that is always called from my scanning of it.

I'll be honest the pinning could be made tighter I suspect. Need to be careful though because I am not 100% the original supported renegotiation .... (Which there are no tests to catch). Because what happened if you freed the GCHandle, then called renegotiate and the callback was called? There was no catch if that was unallocated. However now it will last for the lifetime of the context.

I just double checked that close is always called from SslStream.Dispose(disposing true) which comes from AuthenticatedStream which comes from Stream. I can't actually see a finalizer in that chain. So it might be missing finalisation. A better option might be to just hook into the SslContext handle as that already runs a finalizer... or to just add one to SslStream.

I can look at adding this to the SslContext as the callback is registered with that so freeing it just after the context is freed will ensure it's never run with an unpinned buffer.

Drawaes · 2017-10-22T20:01:36Z

I suspect that the Debian failure is due to it doing something different for selection that "Standard" OpenSSL, I would suggest that I write a matching method so that its consistent, its better .net does the same thing across distros... happy to do it in another PR ?

stephentoub · 2017-10-23T12:57:20Z

+            outp = IntPtr.Zero;
+            outlen = 0;
+
+            if (arg == IntPtr.Zero)


In what situation will it be null? Could you add a comment?

I might remove that check now. I have just moved the GCHandle to be inside the SslCTX handle and tied directly to it's lifetime, so now this method should never be called after the handle is freed. I would perhaps change these to debug.asserts "just in case" but will remove the actual check.

stephentoub · 2017-10-23T13:09:31Z

-        {
-            get { return handle == IntPtr.Zero; }
-        }
+        public GCHandle AlpnHandle { get => _alpnHandle; set => _alpnHandle = value; }


Could just be:

public GCHandle AlpnHandle { get; set; }

and then you wouldn't need to explicitly define the field.

its a struct right? what happens if you free the struct from the get? isn't a copy that now isn't set to free?

I have been caught out on GCHandle before with Readonly, in that the copy updates the "Free" but the original doesn't thus causing a double free and an exception.
(I could be wrong as well :) but I assume that the Get will cause a copy).

I don't understand the questions. The code:

public GCHandle AlpnHandle { get; set; }

is functionally identical to:

private GCHandle _alpnHandle; public GCHandle AlpnHandle { get => _alpnHandle; set => _alpnHandle = value; }

The only difference is what the name of the field is, as the compiler will generate that field for you in the first case.

if(_alpnHandle.IsAllocated) { _alpnHandle.Free(); }

If I do that, and then call IsAllocated straight after and its a property, wont isallocated still be true?

Ah, you're concerned about a subsequent call to ReleaseHandle seeing AlpnHandle.IsAllocated as true and trying to free it again? Yes, that could happen, so if that's the concern, then yeah, you could stick with what you have. Though ReleaseHandle should not be called multiple times.

Your call, I don't mind either way

What you have is fine.

stephentoub

Thanks for tracking this down and fixing it, @Drawaes!

stephentoub · 2017-10-23T15:18:15Z

            SslGetAlpnSelected(ssl, out protocol, out len);

-            if (len < 1)
+            if (len == 0)


What about the native change to go along with this?

Your too quick, I was using Github to transfer files to my unix box to make the change :P

Drawaes · 2017-10-23T15:29:02Z

Fixed the check for < 0 and added a *len = 0; to the native method.

stephentoub · 2017-10-23T15:31:37Z


 extern "C" void CryptoNative_SslGet0AlpnSelected(SSL* ssl, const uint8_t** protocol, uint32_t* len)
 {
+    *len = 0;


Maybe make it:

#ifdef HAVE_OPENSSL_ALPN if (API_EXISTS(SSL_get0_alpn_selected)) { SSL_get0_alpn_selected(ssl, protocol, len); } else #endif { *protocol = NULL; *len = 0; } }

?

(protocol should be initialized, too)

hahah, okay

stephentoub · 2017-10-23T15:41:57Z

-            fixed (byte* sp = server)
-            {
-                return Interop.Ssl.SslSelectNextProto(out outp, out outlen, (IntPtr)sp, (uint)server.Length, inp, inlen) == Interop.Ssl.OPENSSL_NPN_NEGOTIATED ?
+            return Interop.Ssl.SslSelectNextProto(out outp, out outlen, (IntPtr)protocols.AddrOfPinnedObject(), (uint)server.Length, inp, inlen) == Interop.Ssl.OPENSSL_NPN_NEGOTIATED ?


I may be confusing it with another API, but doesn't AddrOfPinnedObject return an IntPtr? I'm wondering what the cast is for.

Nit: @Drawaes, the cast is back? I'd thought you'd removed it, but it appears to have been reverted?

It is my bad, I was resetting/dropping test changes to figure out what was going on and I killed that as I had merged locally that change into a cleanup commit. I am reverting that.

stephentoub · 2017-10-23T15:42:32Z

        {
            Interop.Ssl.SslCtxDestroy(handle);
            SetHandle(IntPtr.Zero);
+            if(_alpnHandle.IsAllocated)


Nit: space after if

stephentoub

Thanks!

stephentoub · 2017-10-23T16:20:48Z

@dotnet-bot test Outerloop Linux x64 Debug Build please

stephentoub · 2017-10-23T17:16:08Z

@Drawaes, this assert is getting hit:
https://github.com/dotnet/corefx/pull/24799/files#diff-8e59f86b12ee01ddf687a1e3c5c34dbdR336

2017-10-23 16:51:14,203: INFO: proc(54): run_and_log_output: Output:    at System.Diagnostics.Debug.Assert(Boolean condition, String message, String detailMessage) in /root/coreclr/src/mscorlib/shared/System/Diagnostics/Debug.cs:line 97
2017-10-23 16:51:14,203: INFO: proc(54): run_and_log_output: Output:    at Interop.OpenSsl.AlpnServerSelectCallback(IntPtr ssl, IntPtr& outp, Byte& outlen, IntPtr inp, UInt32 inlen, IntPtr arg) in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_true_prtest/src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs:line 336
2017-10-23 16:51:14,203: INFO: proc(54): run_and_log_output: Output:    at Interop.Ssl.SslDoHandshake(SafeSslHandle ssl)
2017-10-23 16:51:14,203: INFO: proc(54): run_and_log_output: Output:    at Interop.Ssl.SslDoHandshake(SafeSslHandle ssl)
2017-10-23 16:51:14,203: INFO: proc(54): run_and_log_output: Output:    at Interop.OpenSsl.DoSslHandshake(SafeSslHandle context, Byte[] recvBuf, Int32 recvOffset, Int32 recvCount, Byte[]& sendBuf, Int32& sendCount) in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_true_prtest/src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs:line 160
2017-10-23 16:51:14,203: INFO: proc(54): run_and_log_output: Output:    at System.Net.Security.SslStreamPal.HandshakeInternal(SafeFreeCredentials credential, SafeDeleteContext& context, SecurityBuffer inputBuffer, SecurityBuffer outputBuffer, SslAuthenticationOptions sslAuthenticationOptions) in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_true_prtest/src/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs:line 160
2017-10-23 16:51:14,204: INFO: proc(54): run_and_log_output: Output:    at System.Net.Security.SslStreamPal.AcceptSecurityContext(SafeFreeCredentials& credential, SafeDeleteContext& context, SecurityBuffer[] inputBuffers, SecurityBuffer outputBuffer, SslAuthenticationOptions sslAuthenticationOptions) in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_true_prtest/src/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs:line 40
2017-10-23 16:51:14,204: INFO: proc(54): run_and_log_output: Output:    at System.Net.Security.SecureChannel.GenerateToken(Byte[] input, Int32 offset, Int32 count, Byte[]& output) in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_true_prtest/src/System.Net.Security/src/System/Net/Security/SecureChannel.cs:line 801
2017-10-23 16:51:14,204: INFO: proc(54): run_and_log_output: Output:    at System.Net.Security.SecureChannel.NextMessage(Byte[] incoming, Int32 offset, Int32 count) in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_true_prtest/src/System.Net.Security/src/System/Net/Security/SecureChannel.cs:line 716
2017-10-23 16:51:14,204: INFO: proc(54): run_and_log_output: Output:    at System.Net.Security.SslState.StartSendBlob(Byte[] incoming, Int32 count, AsyncProtocolRequest asyncRequest) in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_true_prtest/src/System.Net.Security/src/System/Net/Security/SslState.cs:line 768
2017-10-23 16:51:14,204: INFO: proc(54): run_and_log_output: Output:    at System.Net.Security.SslState.ProcessReceivedBlob(Byte[] buffer, Int32 count, AsyncProtocolRequest asyncRequest) in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_true_prtest/src/System.Net.Security/src/System/Net/Security/SslState.cs:line 983
2017-10-23 16:51:14,204: INFO: proc(54): run_and_log_output: Output:    at System.Net.Security.SslState.ReadFrameCallback(AsyncProtocolRequest asyncRequest) in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_true_prtest/src/System.Net.Security/src/System/Net/Security/SslState.cs:line 1151
2017-10-23 16:51:14,204: INFO: proc(54): run_and_log_output: Output:    at System.Net.AsyncProtocolRequest.CompleteRequest(Int32 result) in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_true_prtest/src/System.Net.Security/src/System/Net/HelperAsyncResults.cs:line 109
2017-10-23 16:51:14,205: INFO: proc(54): run_and_log_output: Output:    at System.Net.FixedSizeReader.<ReadPacketAsync>d__1.MoveNext() in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_true_prtest/src/System.Net.Security/src/System/Net/FixedSizeReader.cs:line 72
2017-10-23 16:51:14,205: INFO: proc(54): run_and_log_output: Output:    at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state) in /root/coreclr/src/mscorlib/shared/System/Threading/ExecutionContext.cs:line 142
2017-10-23 16:51:14,205: INFO: proc(54): run_and_log_output: Output:    at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1.MoveNext() in /root/coreclr/src/mscorlib/src/System/Runtime/CompilerServices/AsyncMethodBuilder.cs:line 597
2017-10-23 16:51:14,205: INFO: proc(54): run_and_log_output: Output:    at System.Threading.Tasks.Task.RunContinuations(Object continuationObject) in /root/coreclr/src/mscorlib/src/System/Threading/Tasks/Task.cs:line 3283
2017-10-23 16:51:14,205: INFO: proc(54): run_and_log_output: Output:    at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot) in /root/coreclr/src/mscorlib/src/System/Threading/Tasks/Task.cs:line 2459
2017-10-23 16:51:14,205: INFO: proc(54): run_and_log_output: Output:    at System.Threading.ThreadPoolWorkQueue.Dispatch() in /root/coreclr/src/mscorlib/src/System/Threading/ThreadPool.cs:line 588

Drawaes · 2017-10-23T17:27:24Z

Yeah just seen that (why I love a good assert). I will check it after I am home from work/dinner ;)

Drawaes · 2017-10-24T02:43:20Z

This is a bit of a change, I am happy to back it out if you want. But after re-reading the code we can completely avoid the pin. This means that we don't hold that ugly pin for the lifetime of the connection which will cause fragmentation and bad things in the GC'd heap.

Instead what we do is the matching in c# (also means we completely avoid the conversion to array of the protocol list). It also means that we need no pinning, as we return a pointer into the client buffer if a match exists.

We also then avoid the issue that we saw earlier that one distro had different matching so we have a consistent match for all of .net.

Also we remove the OpenSSL Surface area we hit.

stephentoub · 2017-10-24T02:46:06Z

Can we do it as a follow-up PR? I'd like to get the crashes fixed asap.

Priya91 · 2017-10-24T23:37:53Z

        [Fact]
-        [PlatformSpecific(~TestPlatforms.OSX)]
+        [PlatformSpecific(TestPlatforms.Linux)]
        public void SslStream_StreamToStream_Alpn_NonMatchingProtocols_Fail()


@Drawaes Why are you excluding this test from Windows? This should run on all OSes except OSX.

because now it doesn't work, I am trying to get the maximum to pass,

this test as far as I can tell never did anything on any platform before because the tasks were not awaited.

Priya91 · 2017-10-24T23:38:39Z

-                Assert.ThrowsAsync<AuthenticationException>(() => { return client.AuthenticateAsClientAsync(clientOptions, CancellationToken.None); });
-                Assert.ThrowsAsync<AuthenticationException>(() => { return server.AuthenticateAsServerAsync(serverOptions, CancellationToken.None); });
+                Assert.True(DoHandshakeWithOptions(client, server, clientOptions, serverOptions));
+                Assert.Equal(default, client.NegotiatedApplicationProtocol);


This behavior is wrong, if the client and server fail to match alpn, it should fail the handshake as per the rfc. Why did you change this behavior?

I didn't change the behaviour, see above I don't believe the test was ever working.

once I figure out what is going on, I will just disable the tests with an issue.

Uhh, that's a bug, it should be async () => await

I addressed this in the cancellation PR #24849 You can undo these changes here.

Are you sure that fixes it, don't you need to await the result of ThrowAsync from memory the code is basically

public async static Task<T> ThrowsAsync<T>(Func<Task> testCode) where T : Exception { try { await testCode(); Assert.Throws<T>(() => { }); // Use xUnit's default behavior. } catch (T exception) { return exception; } return null; }

without the

await Assert.ThrowAsync(
it still won't wait for the assert? Or am I wrong on that?

https://github.com/dotnet/corefx/blob/master/src/System.Net.Security/tests/FunctionalTests/ServerAsyncAuthenticateTest.cs#L66

i've been trying to get the implementation of ThrowsAsync, thanks for getting it. Yeah it looks like the throwsasync needs to be awaited.

so when i do, the windows tests hang, and the linux tests fail.I will disable with an issue and check them in.

Ok sounds good! thanks!

Just letting them run with async to make sure that need to be disabled on everything or just one OS. Then I will tidy up with an [Issue] tag

Priya91

You can either file bug for that non matching protocols test case and move on with this PR, or address that here.

Putting back the init code in the cpp file for the out params (lost in a reset)

Priya91 · 2017-10-25T00:49:39Z

+                Task t2 = Assert.ThrowsAsync<InvalidOperationException>(() => server.AuthenticateAsServerAsync(serverOptions, CancellationToken.None));
+
+                await Task.WhenAll(t1, t2);
            }


if t1, t2 don't complete this will result in hang no, should this be Assert.True(Task.WaitAll(t1,t2, timeout)), similarly below as well. There is a passingtesttimeout value in this project, you could use that.

no because then it will pass, as it won't wait for the asserts. There is a timeout already on the xunit tests, they don't hang for ever

It wont pass if waitall completed becoz of timeout, as it will return false, and assert.true will throw. If not this can cause long test times for the xunit timeout to be reached. Per test timeout is better than global one.

the only way to do this would be,

Task delay = Task.Delay(sometimeout); Task passingTask = Task.WhenAny(delay, Task.WhenAll(t1,t2)); Assert.NotEqual(passingTask, delay);

Why wouldn't task.waitall with timeout work?

Changed to wait all, seems odd still, you know that you can use a .json file in the solution folder to configure a timeout for all tests in a project rather than writing the code for each method.

Anyway updated to this as it seems the pattern currently used

Drawaes · 2017-10-25T00:59:34Z

Disabled test with active issue #24853

Priya91 · 2017-10-25T01:26:18Z

I debugged the non-matching protocol test failure on windows, it is throwing authenticationexception as expected, but the exception gets thrown as timeout aggregrate exception.

System.Net.Security.Tests.SslStreamAlpnTests.SslStream_StreamToStream_Alpn_NonMatchingProtocols_Fail [FAIL]
        System.AggregateException : One or more errors occurred. (VirtualNetwork: Timeout reading the next frame.) (A call to SSPI failed, see inner exception.)
        ---- System.TimeoutException : VirtualNetwork: Timeout reading the next frame.
        ---- System.Security.Authentication.AuthenticationException : A call to SSPI failed, see inner exception.
        -------- System.ComponentModel.Win32Exception : No common application protocol exists between the client and the server. Application protocol negotiation failed
        Stack Trace:
           E:\A\_work\1111\s\src\mscorlib\src\System\ThrowHelper.cs(224,0): at System.ThrowHelper.ThrowAggregateException(List`1 exceptions)
           E:\A\_work\1111\s\src\mscorlib\src\System\Threading\Tasks\Task.cs(4651,0): at System.Threading.Tasks.Task.WaitAllCore(Task[] tasks, Int32 millisecondsTimeout, CancellationToken cancellationToken)
           E:\A\_work\1111\s\src\mscorlib\src\System\Threading\Tasks\Task.cs(4502,0): at System.Threading.Tasks.Task.WaitAll(Task[] tasks)
           E:\corefx\src\System.Net.Security\tests\FunctionalTests\SslStreamAlpnTests.cs(201,0): at System.Net.Security.Tests.SslStreamAlpnTests.SslStream_StreamToStream_Alpn_NonMatchingProtocols_Fail()
           ----- Inner Stack Trace #1 (System.TimeoutException) -----
           E:\corefx\src\Common\tests\System\Net\VirtualNetwork\VirtualNetwork.cs(42,0): at System.Net.Test.Common.VirtualNetwork.ReadFrame(Boolean server, Byte[]& buffer)
           E:\corefx\src\Common\tests\System\Net\VirtualNetwork\VirtualNetworkStream.cs(118,0): at System.Net.Test.Common.VirtualNetworkStream.Read(Byte[] buffer, Int32 offset, Int32 count)
           E:\corefx\src\Common\tests\System\Net\VirtualNetwork\VirtualNetworkStream.cs(138,0): at System.Net.Test.Common.VirtualNetworkStream.<>c__DisplayClass27_0.<ReadAsync>b__0()
           E:\A\_work\1111\s\src\mscorlib\src\System\Threading\Tasks\future.cs(610,0): at System.Threading.Tasks.Task`1.InnerInvoke()
           E:\A\_work\1111\s\src\mscorlib\shared\System\Threading\ExecutionContext.cs(151,0): at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
           E:\A\_work\1111\s\src\mscorlib\src\System\Threading\Tasks\Task.cs(2440,0): at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot)
           --- End of stack trace from previous location where exception was thrown ---
           E:\A\_work\1111\s\src\mscorlib\src\System\Runtime\ExceptionServices\ExceptionServicesCommon.cs(130,0): at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           E:\A\_work\1111\s\src\mscorlib\src\System\Runtime\CompilerServices\TaskAwaiter.cs(152,0): at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
           E:\A\_work\1111\s\src\mscorlib\src\System\Runtime\CompilerServices\TaskAwaiter.cs(563,0): at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
           E:\corefx\src\System.Net.Security\src\System\Net\FixedSizeReader.cs(56,0): at System.Net.FixedSizeReader.<ReadPacketAsync>d__1.MoveNext()
           --- End of stack trace from previous location where exception was thrown ---
           E:\A\_work\1111\s\src\mscorlib\src\System\Runtime\ExceptionServices\ExceptionServicesCommon.cs(130,0): at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           E:\corefx\src\System.Net.Security\src\System\Net\Security\SslState.cs(768,0): at System.Net.Security.SslState.InternalEndProcessAuthentication(LazyAsyncResult lazyResult)
           E:\corefx\src\System.Net.Security\src\System\Net\Security\SslState.cs(739,0): at System.Net.Security.SslState.EndProcessAuthentication(IAsyncResult result)
           E:\corefx\src\System.Net.Security\src\System\Net\Security\SslStream.cs(194,0): at System.Net.Security.SslStream.EndAuthenticateAsClient(IAsyncResult asyncResult)
           E:\corefx\src\System.Net.Security\src\System\Net\Security\SslStream.cs(381,0): at System.Net.Security.SslStream.<>c.<AuthenticateAsClientAsync>b__44_1(IAsyncResult iar)
           E:\A\_work\1111\s\src\mscorlib\src\System\Threading\Tasks\FutureFactory.cs(533,0): at System.Threading.Tasks.TaskFactory`1.FromAsyncCoreLogic(IAsyncResult iar, Func`2 endFunction, Action`1 endAction, Task`1 promise, Boolean requiresSynchronization)
           ----- Inner Stack Trace #2 (System.Security.Authentication.AuthenticationException) -----
           E:\A\_work\1111\s\src\mscorlib\src\System\Runtime\ExceptionServices\ExceptionServicesCommon.cs(130,0): at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           E:\corefx\src\System.Net.Security\src\System\Net\Security\SslState.cs(1030,0): at System.Net.Security.SslState.StartSendAuthResetSignal(ProtocolToken message, AsyncProtocolRequest asyncRequest, ExceptionDispatchInfo exception, CancellationToken cancellationToken)
           E:\corefx\src\System.Net.Security\src\System\Net\Security\SslState.cs(841,0): at System.Net.Security.SslState.CheckCompletionBeforeNextReceive(ProtocolToken message, AsyncProtocolRequest asyncRequest, CancellationToken cancellationToken)
           E:\corefx\src\System.Net.Security\src\System\Net\Security\SslState.cs(826,0): at System.Net.Security.SslState.StartSendBlob(Byte[] incoming, Int32 count, AsyncProtocolRequest asyncRequest, CancellationToken cancellationToken)
           E:\corefx\src\System.Net.Security\src\System\Net\Security\SslState.cs(1011,0): at System.Net.Security.SslState.ProcessReceivedBlob(Byte[] buffer, Int32 count, AsyncProtocolRequest asyncRequest, CancellationToken cancellationToken)
           E:\corefx\src\System.Net.Security\src\System\Net\Security\SslState.cs(1185,0): at System.Net.Security.SslState.ReadFrameCallback(AsyncProtocolRequest asyncRequest)
           --- End of stack trace from previous location where exception was thrown ---
           E:\A\_work\1111\s\src\mscorlib\src\System\Runtime\ExceptionServices\ExceptionServicesCommon.cs(130,0): at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           E:\corefx\src\System.Net.Security\src\System\Net\Security\SslState.cs(768,0): at System.Net.Security.SslState.InternalEndProcessAuthentication(LazyAsyncResult lazyResult)
           E:\corefx\src\System.Net.Security\src\System\Net\Security\SslState.cs(739,0): at System.Net.Security.SslState.EndProcessAuthentication(IAsyncResult result)
           E:\corefx\src\System.Net.Security\src\System\Net\Security\SslStream.cs(248,0): at System.Net.Security.SslStream.EndAuthenticateAsServer(IAsyncResult asyncResult)
           E:\corefx\src\System.Net.Security\src\System\Net\Security\SslStream.cs(416,0): at System.Net.Security.SslStream.<>c.<AuthenticateAsServerAsync>b__48_1(IAsyncResult iar)
           E:\A\_work\1111\s\src\mscorlib\src\System\Threading\Tasks\FutureFactory.cs(533,0): at System.Threading.Tasks.TaskFactory`1.FromAsyncCoreLogic(IAsyncResult iar, Func`2 endFunction, Action`1 endAction, Task`1 promise, Boolean requiresSynchronization)

Drawaes · 2017-10-25T01:30:08Z

Yeah I believe the timeout comes from the virtual network. So it's hanging but you are seeing the virtual network stream exit.

stephentoub · 2017-10-25T03:01:11Z

+            {
+                _sslAuthenticationOptions.AlpnProtocolsHandle.Free();
+            }
+


Nit: unnecessary blank line (in addition to the missing space after the if above

Yeah sorry was late, thought you meant a new line ... I have fixed.

stephentoub · 2017-10-25T04:32:55Z

@dotnet-bot test OSX x64 Debug Build please
@dotnet-bot test Windows x64 Debug Build please
@dotnet-bot test Outerloop Linux x64 Debug Build please

stephentoub · 2017-10-25T14:15:37Z

@Drawaes, doesn't the SslStream_StreamToStream_Alpn_Success test still need an [ActiveIssue] on it?

Drawaes · 2017-10-25T14:52:11Z

Yes I just ran out of time last night to sort it out (LDN timezone) should be sorted now. I launched a new issue for it as it's a different issue I suspect (I think it's that version of debians matching without looking or checking ;) )

stephentoub · 2017-10-25T14:53:15Z

Yes I just ran out of time last night to sort it out

Ah, sorry, I saw you pushed a commit after my comments and figured you'd just missed that one.

Drawaes · 2017-10-25T15:15:16Z

Don't code and not build when between meetings
@dotnet-bot test OSX x64 Debug Build please
@dotnet-bot test Windows x64 Debug Build please
@dotnet-bot test Outerloop Linux x64 Debug Build please

stephentoub · 2017-10-25T16:43:42Z

The Linux failure is https://github.com/dotnet/corefx/issues/24869 and is unrelated. I'm going to go ahead and merge this to unblock the rest of the branch. @Drawaes, thanks for getting this fixed.

* Fixing GC Hole * Moved back to original pin location * Added Finializer * Make tests async * Reacting to review Putting back the init code in the cpp file for the out params (lost in a reset) * Added active issue on the failure test * React to review * Added Active Issue for Linux success tests

* Fixing GC Hole * Moved back to original pin location * Added Finializer * Make tests async * Reacting to review Putting back the init code in the cpp file for the out params (lost in a reset) * Added active issue on the failure test * React to review * Added Active Issue for Linux success tests Commit migrated from dotnet/corefx@2bd69b9

Fixing GC Hole

8142301

dnfclas added the cla-already-signed label Oct 22, 2017

Drawaes mentioned this pull request Oct 22, 2017

[WIP] Checking Master with <XunitShowProgress>true</XunitShowProgress> Outerloop #24781

Closed

stephentoub reviewed Oct 22, 2017

View reviewed changes

stephentoub reviewed Oct 23, 2017

View reviewed changes

stephentoub approved these changes Oct 23, 2017

View reviewed changes

davidsh added the area-System.Net.Security label Oct 23, 2017

Priya91 reviewed Oct 24, 2017

View reviewed changes

Priya91 suggested changes Oct 24, 2017

View reviewed changes

Priya91 mentioned this pull request Oct 25, 2017

Implement cancellation policy on sslstream. #24849

Closed

Priya91 approved these changes Oct 25, 2017

View reviewed changes

Drawaes added 2 commits October 25, 2017 01:18

Make tests async

011add7

Reacting to review

651700b

Putting back the init code in the cpp file for the out params (lost in a reset)

Priya91 reviewed Oct 25, 2017

View reviewed changes

Added active issue on the failure test

1b4acd5

stephentoub reviewed Oct 25, 2017

View reviewed changes

React to review

b6c8fe6

Added Active Issue for Linux success tests

3cac46b

stephentoub merged commit 2bd69b9 into dotnet:master Oct 25, 2017

karelz added this to the 2.1.0 milestone Oct 28, 2017

karelz assigned Drawaes and Priya91 and unassigned Drawaes Oct 28, 2017

Drawaes mentioned this pull request Nov 7, 2017

Open ssl pfx aspnet/KestrelHttpServer#2150

Merged

Uh oh!

Conversation

Drawaes commented Oct 22, 2017

Uh oh!

Drawaes commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Drawaes commented Oct 22, 2017

Uh oh!

Drawaes commented Oct 22, 2017

Uh oh!

Drawaes commented Oct 22, 2017

Uh oh!

Drawaes commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Oct 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Drawaes commented Oct 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Drawaes commented Oct 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Drawaes Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Drawaes Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Drawaes commented Oct 22, 2017 •

edited

Loading

Drawaes commented Oct 22, 2017 •

edited

Loading

Drawaes Oct 23, 2017 •

edited

Loading

Drawaes Oct 23, 2017 •

edited

Loading

stephentoub Oct 23, 2017 •

edited

Loading

stephentoub Oct 25, 2017 •

edited

Loading