SslStream.ReadAsyncInternal() throws misleading exception if stream is disposed while reading

[mikeharder]

Description

If SslStream is disposed while there is a concurrent call to ReadAsyncInternal(), a misleading exception may be thrown:

Unhandled exception. System.NotSupportedException:  This method may not be called when another read operation is pending.
   at System.Net.Security.SslStream.ReadAsyncInternal[TIOAdapter](Memory`1 buffer, CancellationToken cancellationToken)
   at System.Runtime.CompilerServices.PoolingAsyncValueTaskMethodBuilder`1.StateMachineBox`1.System.Threading.Tasks.Sources.IValueTaskSource<TResult>.GetResult(Int16 token)
   at System.Net.Security.SslStream.Read(Byte[] buffer, Int32 offset, Int32 count)

The exception message claims that "ReadAsyncInternal() was called when another read operation is pending", but this is misleading, since there were not concurrent calls to ReadAsyncInternal(). This exception message is raised due to the following codepaths:

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs

Lines 50 to 63 in 6ddd06c

	private void CloseInternal()
	{
	_exception = s_disposedSentinel;
	CloseContext();
	// Ensure a Read or Auth operation is not in progress,
	// block potential future read and auth operations since SslStream is disposing.
	// This leaves the _nestedRead = 1 and _nestedAuth = 1, but that's ok, since
	// subsequent operations check the _exception sentinel first
	if (Interlocked.Exchange(ref _nestedRead, 1) == 0 &&
	Interlocked.Exchange(ref _nestedAuth, 1) == 0)
	{
	_buffer.ReturnBuffer();
	}

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs

Lines 763 to 769 in 6ddd06c

	private async ValueTask<int> ReadAsyncInternal<TIOAdapter>(Memory<byte> buffer, CancellationToken cancellationToken)
	where TIOAdapter : IReadWriteAdapter
	{
	if (Interlocked.Exchange(ref _nestedRead, 1) == 1)
	{
	throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "read"));
	}

For customers trying to debug this issue, it would be very helpful if ReadAsyncInternal() could throw ObjectDisposedException in this case. This makes it clear the issue is dispose during read, not concurrent reads.

Maybe a code change like this?

if (Interlocked.Exchange(ref _nestedRead, 1) == 1)
{
    ThrowIfExceptionalOrNotAuthenticated();
    throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "read"));
}

Reproduction Steps

You should be able to repro this by concurrently calling Read() and Dispose() in a loop until you trigger the race condition. Alternatively, you can use the repro I created for the Azure SDK (which has a bug where it calls Dispose() concurrently during a Read()):

Azure/azure-sdk-for-net#32577

Expected behavior

SslStream throws ObjectDisposedException

Actual behavior

SslStream throws NotSupportedException

Regression?

No

Known Workarounds

No response

Configuration

.NET Runtime 7.0.0
Windows 10
x64

Other information

No response

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

Issue Details

---

Description

If SslStream is disposed while there is a concurrent call to ReadAsyncInternal(), a misleading exception may be thrown:

Unhandled exception. System.NotSupportedException:  This method may not be called when another read operation is pending.
   at System.Net.Security.SslStream.ReadAsyncInternal[TIOAdapter](Memory`1 buffer, CancellationToken cancellationToken)
   at System.Runtime.CompilerServices.PoolingAsyncValueTaskMethodBuilder`1.StateMachineBox`1.System.Threading.Tasks.Sources.IValueTaskSource<TResult>.GetResult(Int16 token)
   at System.Net.Security.SslStream.Read(Byte[] buffer, Int32 offset, Int32 count)

The exception message claims that "ReadAsyncInternal() was called when another read operation is pending", but this is misleading, since there were not concurrent calls to ReadAsyncInternal(). This exception message is raised due to the following codepaths:

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs

Lines 50 to 63 in 6ddd06c

	private void CloseInternal()
	{
	_exception = s_disposedSentinel;
	CloseContext();
	// Ensure a Read or Auth operation is not in progress,
	// block potential future read and auth operations since SslStream is disposing.
	// This leaves the _nestedRead = 1 and _nestedAuth = 1, but that's ok, since
	// subsequent operations check the _exception sentinel first
	if (Interlocked.Exchange(ref _nestedRead, 1) == 0 &&
	Interlocked.Exchange(ref _nestedAuth, 1) == 0)
	{
	_buffer.ReturnBuffer();
	}

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs

Lines 763 to 769 in 6ddd06c

	private async ValueTask<int> ReadAsyncInternal<TIOAdapter>(Memory<byte> buffer, CancellationToken cancellationToken)
	where TIOAdapter : IReadWriteAdapter
	{
	if (Interlocked.Exchange(ref _nestedRead, 1) == 1)
	{
	throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "read"));
	}

For customers trying to debug this issue, it would be very helpful if ReadAsyncInternal() could throw ObjectDisposedException in this case. This makes it clear the issue is dispose during read, not concurrent reads.

Maybe a code change like this?

if (Interlocked.Exchange(ref _nestedRead, 1) == 1)
{
    ThrowIfExceptionalOrNotAuthenticated();
    throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "read"));
}

Reproduction Steps

You should be able to repro this by concurrently calling Read() and Dispose() in a loop until you trigger the race condition. Alternatively, you can use the repro I created for the Azure SDK (which has a bug where it calls Dispose() concurrently during a Read()):

Azure/azure-sdk-for-net#32577

Expected behavior

SslStream throws ObjectDisposedException

Actual behavior

SslStream throws NotSupportedException

Regression?

No

Known Workarounds

No response

Configuration

.NET Runtime 7.0.0
Windows 10
x64

Other information

No response

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

[wfurt]

I look at it more closely it may be something different. I changed the value we set in closing but I still saw errors about concurrent read. Also since I have debug version of HTTP, I saw occasional Debug.Assert hits around read buffer.
So this really smells like concurrency problem.
Having that said, HttpStreams do not prevent concurrent access but we don't have anything in place AFAIK that would ensure proper handling. Do you have any thoughts on this @stephentoub?

I feel we can still make improvements in SslStream but the repro may be still failing.

[stephentoub]

It'd be challenging for us to make any strong guarantees about the thread-safety of using an SslStream and Dispose'ing it concurrently. People do and will, whether we document it as safe to do or not (we don't), so we should strive to minimize the impact of doing so in the name of defense in depth, but I suspect we'll still have a tail of issues that remain.

[wfurt]

closed by #79329