SslStream tests can hit Assert and fail completely

[wfurt]

https://helix.dot.net/api/2019-06-17/jobs/ef6bb735-70e8-4269-9758-93a9fda98c7d/workitems/System.Net.Security.Tests/console

    System.Net.Security.Tests.SslStreamNetworkStreamTest.SslStream_NegotiateClientCertificateAsync_PendingDecryptedData_Throws [SKIP]
      Condition(s) not met: "SupportsRenegotiation"
Process terminated due to "Expected 7 <= 0
   at System.Diagnostics.DebugProvider.Fail(String message, String detailMessage) in /_/src/libraries/System.Private.CoreLib/src/System/Diagnostics/DebugProvider.cs:line 22
   at System.Diagnostics.Debug.Fail(String message, String detailMessage) in /_/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Debug.cs:line 131
   at System.Diagnostics.Debug.Assert(Boolean condition, String message, String detailMessage) in /_/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Debug.cs:line 95
   at System.Diagnostics.Debug.Assert(Boolean condition, String message) in /_/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Debug.cs:line 84
   at System.Diagnostics.Debug.Assert(Boolean condition, AssertInterpolatedStringHandler& message) in /_/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Debug.cs:line 88
   at System.Net.ArrayBuffer.Discard(Int32 byteCount) in /_/src/libraries/Common/src/System/Net/ArrayBuffer.cs:line 67
   at System.Net.Security.SslStream.SslBuffer.DiscardEncrypted(Int32 byteCount) in /_/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.cs:line 152
   at System.Net.Security.SslStream.ProcessBlob(Int32 frameSize) in /_/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs:line 498
   at System.Net.Security.SslStream.<ReceiveBlobAsync>d__167`1[[System.Net.Security.AsyncReadWriteAdapter, System.Net.Security, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].MoveNext() in /_/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs:line 487
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1[[System.Net.Security.ProtocolToken, System.Net.Security, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a],[System.Net.Security.SslStream.<ReceiveBlobAsync>d__167`1[[System.Net.Security.AsyncReadWriteAdapter, System.Net.Security, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]], System.Net.Security, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].ExecutionContextCallback(Object s) in /_/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncTaskMethodBuilderT.cs:line 286
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state) in /_/src/libraries/System.Private.CoreLib/src/System/Threading/ExecutionContext.cs:line 183
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1[[System.Net.Security.ProtocolToken, System.Net.Security, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a],[System.Net.Security.SslStream.<ReceiveBlobAsync>d__167`1[[System.Net.Security.AsyncReadWriteAdapter, System.Net.Security, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]], System.Net.Security, Version=7.0.0.0, 

may be regression from #64747

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

Issue Details

---

https://helix.dot.net/api/2019-06-17/jobs/ef6bb735-70e8-4269-9758-93a9fda98c7d/workitems/System.Net.Security.Tests/console

    System.Net.Security.Tests.SslStreamNetworkStreamTest.SslStream_NegotiateClientCertificateAsync_PendingDecryptedData_Throws [SKIP]
      Condition(s) not met: "SupportsRenegotiation"
Process terminated due to "Expected 7 <= 0
   at System.Diagnostics.DebugProvider.Fail(String message, String detailMessage) in /_/src/libraries/System.Private.CoreLib/src/System/Diagnostics/DebugProvider.cs:line 22
   at System.Diagnostics.Debug.Fail(String message, String detailMessage) in /_/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Debug.cs:line 131
   at System.Diagnostics.Debug.Assert(Boolean condition, String message, String detailMessage) in /_/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Debug.cs:line 95
   at System.Diagnostics.Debug.Assert(Boolean condition, String message) in /_/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Debug.cs:line 84
   at System.Diagnostics.Debug.Assert(Boolean condition, AssertInterpolatedStringHandler& message) in /_/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Debug.cs:line 88
   at System.Net.ArrayBuffer.Discard(Int32 byteCount) in /_/src/libraries/Common/src/System/Net/ArrayBuffer.cs:line 67
   at System.Net.Security.SslStream.SslBuffer.DiscardEncrypted(Int32 byteCount) in /_/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.cs:line 152
   at System.Net.Security.SslStream.ProcessBlob(Int32 frameSize) in /_/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs:line 498
   at System.Net.Security.SslStream.<ReceiveBlobAsync>d__167`1[[System.Net.Security.AsyncReadWriteAdapter, System.Net.Security, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].MoveNext() in /_/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs:line 487
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1[[System.Net.Security.ProtocolToken, System.Net.Security, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a],[System.Net.Security.SslStream.<ReceiveBlobAsync>d__167`1[[System.Net.Security.AsyncReadWriteAdapter, System.Net.Security, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]], System.Net.Security, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].ExecutionContextCallback(Object s) in /_/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncTaskMethodBuilderT.cs:line 286
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state) in /_/src/libraries/System.Private.CoreLib/src/System/Threading/ExecutionContext.cs:line 183
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1[[System.Net.Security.ProtocolToken, System.Net.Security, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a],[System.Net.Security.SslStream.<ReceiveBlobAsync>d__167`1[[System.Net.Security.AsyncReadWriteAdapter, System.Net.Security, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]], System.Net.Security, Version=7.0.0.0, 

may be regression from #64747

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

[danmoseley]

As an aside, it's unfortunate that a failed assert causes the whole test library to terminate. It is possible for test libraries to twiddle this, so that the individual test fails but others still run, it is done here (there may be a better way)

runtime/src/libraries/System.Diagnostics.Debug/tests/DebugTests.cs

Line 67 in 6e05d78

FieldInfo failCoreHook = typeof(DebugProvider).GetField("s_FailCore", BindingFlags.Static | BindingFlags.NonPublic);

@stephentoub I wonder whether we should do this across all our test libraries.

[wfurt]

Note the Assert comes from production code not test or test library.

[stephentoub]

it's unfortunate that a failed assert causes the whole test library to terminate

Just to be clear, a failed assert in the production code, not a test assert. Those production asserts are things that should never fail, and if they do, it's generally either a serious problem or a faulty assert. With the rarity that they cause a test suite to fail and the possible severity of when they do, and the fact that if they do, there's a reasonable chance the process is somehow corrupted and other tests continuing to run could easily lead to other test failures that mask the original problem, I'm happy with the status quo of crashing at that exact moment, hopefully with a good dump from the resulting FailFast.

there may be a better way

If we really wanted to, we'd plug in via Trace.Listeners and supply an alternative handler. I'm skeptical of doing so, however.

[wfurt]

Most of the tests we have are functional e.g. test that for given input we produce expected output. We don't have many (or maybe any) tests that would stress concurrent behavior. While this is not generally applicable to all libraries it feels like for networking this would be valuable and could be good improvement. That would increase our chances to catch problems like this one during development cycle.

[rzikm]

That's weird, this should not be possible. According to the stack trace and the assert message, the _buffer is empty, but after EnsureFullTlsFrameAsync (called in ReceiveBlobAsync lower in the call stack), frameSize >= _buffer.EncryptedLength should always hold.

Did this happen in main, or is nome PR?

[wfurt]

Happened on #65435. Look at #43686 @rzikm

[danmoseley]

Just to be clear, a failed assert in the production code, not a test assert. Those production asserts are things that should never fail, and if they do, it's generally either a serious problem or a faulty assert. With the rarity that they cause a test suite to fail and the possible severity of when they do, and the fact that if they do, there's a reasonable chance the process is somehow corrupted and other tests continuing to run could easily lead to other test failures that mask the original problem, I'm happy with the status quo of crashing at that exact moment, hopefully with a good dump from the resulting FailFast.

I think the infrequency is the best argument here. With respect to severity, one could argue (perhaps reasonably actually) that we should tear down the test process on a NullReferenceException. We have a fair number of Debug.Asserts that test for those.

[stephentoub]

We have a fair number of Debug.Asserts that test for those.

And many more since we annotated for nullable reference types, since if you have a local the compiler doesn't know is non-null but you do, you can either use ! or Debug.Assert(local is not null) to suppress it, and we often choose the latter in order to get that added debug-only validation / tear-down.