Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[QUIC] Fix rooted connection when hanshake failed #87328

Merged
merged 7 commits into from
Jun 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Net.Quic;

namespace Microsoft.Quic;

internal unsafe partial struct QUIC_NEW_CONNECTION_INFO
{
public override string ToString()
=> $"{{ {nameof(QuicVersion)} = {QuicVersion}, {nameof(LocalAddress)} = {LocalAddress->ToIPEndPoint()}, {nameof(RemoteAddress)} = {RemoteAddress->ToIPEndPoint()} }}";
}

internal unsafe partial struct QUIC_LISTENER_EVENT
{
public override string ToString()
=> Type switch
{
QUIC_LISTENER_EVENT_TYPE.NEW_CONNECTION
=> $"{{ {nameof(NEW_CONNECTION.Info)} = {{ {nameof(QUIC_NEW_CONNECTION_INFO.QuicVersion)} = {NEW_CONNECTION.Info->QuicVersion}, {nameof(QUIC_NEW_CONNECTION_INFO.LocalAddress)} = {NEW_CONNECTION.Info->LocalAddress->ToIPEndPoint()}, {nameof(QUIC_NEW_CONNECTION_INFO.RemoteAddress)} = {NEW_CONNECTION.Info->RemoteAddress->ToIPEndPoint()} }} }}",
_ => string.Empty
};
}

internal unsafe partial struct QUIC_CONNECTION_EVENT
{
public override string ToString()
=> Type switch
{
QUIC_CONNECTION_EVENT_TYPE.CONNECTED
=> $"{{ {nameof(CONNECTED.SessionResumed)} = {CONNECTED.SessionResumed} }}",
QUIC_CONNECTION_EVENT_TYPE.SHUTDOWN_INITIATED_BY_TRANSPORT
=> $"{{ {nameof(SHUTDOWN_INITIATED_BY_TRANSPORT.Status)} = {SHUTDOWN_INITIATED_BY_TRANSPORT.Status}, {nameof(SHUTDOWN_INITIATED_BY_TRANSPORT.ErrorCode)} = {SHUTDOWN_INITIATED_BY_TRANSPORT.ErrorCode} }}",
QUIC_CONNECTION_EVENT_TYPE.SHUTDOWN_INITIATED_BY_PEER
=> $"{{ {nameof(SHUTDOWN_INITIATED_BY_PEER.ErrorCode)} = {SHUTDOWN_INITIATED_BY_PEER.ErrorCode} }}",
QUIC_CONNECTION_EVENT_TYPE.SHUTDOWN_COMPLETE
=> $"{{ {nameof(SHUTDOWN_COMPLETE.HandshakeCompleted)} = {SHUTDOWN_COMPLETE.HandshakeCompleted}, {nameof(SHUTDOWN_COMPLETE.PeerAcknowledgedShutdown)} = {SHUTDOWN_COMPLETE.PeerAcknowledgedShutdown}, {nameof(SHUTDOWN_COMPLETE.AppCloseInProgress)} = {SHUTDOWN_COMPLETE.AppCloseInProgress} }}",
QUIC_CONNECTION_EVENT_TYPE.LOCAL_ADDRESS_CHANGED
=> $"{{ {nameof(LOCAL_ADDRESS_CHANGED.Address)} = {LOCAL_ADDRESS_CHANGED.Address->ToIPEndPoint()} }}",
QUIC_CONNECTION_EVENT_TYPE.PEER_ADDRESS_CHANGED
=> $"{{ {nameof(PEER_ADDRESS_CHANGED.Address)} = {PEER_ADDRESS_CHANGED.Address->ToIPEndPoint()} }}",
QUIC_CONNECTION_EVENT_TYPE.PEER_STREAM_STARTED
=> $"{{ {nameof(PEER_STREAM_STARTED.Flags)} = {PEER_STREAM_STARTED.Flags} }}",
QUIC_CONNECTION_EVENT_TYPE.PEER_CERTIFICATE_RECEIVED
=> $"{{ {nameof(PEER_CERTIFICATE_RECEIVED.DeferredStatus)} = {PEER_CERTIFICATE_RECEIVED.DeferredStatus}, {nameof(PEER_CERTIFICATE_RECEIVED.DeferredErrorFlags)} = {PEER_CERTIFICATE_RECEIVED.DeferredErrorFlags} }}",
_ => string.Empty
};
}

internal unsafe partial struct QUIC_STREAM_EVENT
{
public override string ToString()
=> Type switch
{
QUIC_STREAM_EVENT_TYPE.START_COMPLETE
=> $"{{ {nameof(START_COMPLETE.Status)} = {START_COMPLETE.Status}, {nameof(START_COMPLETE.ID)} = {START_COMPLETE.ID}, {nameof(START_COMPLETE.PeerAccepted)} = {START_COMPLETE.PeerAccepted} }}",
QUIC_STREAM_EVENT_TYPE.RECEIVE
=> $"{{ {nameof(RECEIVE.AbsoluteOffset)} = {RECEIVE.AbsoluteOffset}, {nameof(RECEIVE.TotalBufferLength)} = {RECEIVE.TotalBufferLength}, {nameof(RECEIVE.Flags)} = {RECEIVE.Flags} }}",
QUIC_STREAM_EVENT_TYPE.SEND_COMPLETE
=> $"{{ {nameof(SEND_COMPLETE.Canceled)} = {SEND_COMPLETE.Canceled} }}",
QUIC_STREAM_EVENT_TYPE.PEER_SEND_ABORTED
=> $"{{ {nameof(PEER_SEND_ABORTED.ErrorCode)} = {PEER_SEND_ABORTED.ErrorCode} }}",
QUIC_STREAM_EVENT_TYPE.PEER_RECEIVE_ABORTED
=> $"{{ {nameof(PEER_RECEIVE_ABORTED.ErrorCode)} = {PEER_RECEIVE_ABORTED.ErrorCode} }}",
QUIC_STREAM_EVENT_TYPE.SEND_SHUTDOWN_COMPLETE
=> $"{{ {nameof(SEND_SHUTDOWN_COMPLETE.Graceful)} = {SEND_SHUTDOWN_COMPLETE.Graceful} }}",
QUIC_STREAM_EVENT_TYPE.SHUTDOWN_COMPLETE
=> $"{{ {nameof(SHUTDOWN_COMPLETE.ConnectionShutdown)} = {SHUTDOWN_COMPLETE.ConnectionShutdown}, {nameof(SHUTDOWN_COMPLETE.ConnectionShutdownByApp)} = {SHUTDOWN_COMPLETE.ConnectionShutdownByApp}, {nameof(SHUTDOWN_COMPLETE.ConnectionClosedRemotely)} = {SHUTDOWN_COMPLETE.ConnectionClosedRemotely}, {nameof(SHUTDOWN_COMPLETE.ConnectionErrorCode)} = {SHUTDOWN_COMPLETE.ConnectionErrorCode}, {nameof(SHUTDOWN_COMPLETE.ConnectionCloseStatus)} = {SHUTDOWN_COMPLETE.ConnectionCloseStatus} }}",
_ => string.Empty
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,13 @@ internal static unsafe IPEndPoint ToIPEndPoint(this ref QuicAddr quicAddress, Ad
return new Internals.SocketAddress(addressFamilyOverride ?? SocketAddressPal.GetAddressFamily(addressBytes), addressBytes).GetIPEndPoint();
}

internal static unsafe QuicAddr ToQuicAddr(this IPEndPoint iPEndPoint)
internal static unsafe QuicAddr ToQuicAddr(this IPEndPoint ipEndPoint)
{
// TODO: is the layout same for SocketAddress.Buffer and QuicAddr on all platforms?
QuicAddr result = default;
Span<byte> rawAddress = MemoryMarshal.AsBytes(MemoryMarshal.CreateSpan(ref result, 1));

Internals.SocketAddress address = IPEndPointExtensions.Serialize(iPEndPoint);
Internals.SocketAddress address = IPEndPointExtensions.Serialize(ipEndPoint);
Debug.Assert(address.Size <= rawAddress.Length);

address.Buffer.AsSpan(0, address.Size).CopyTo(rawAddress);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,18 @@ public ResettableValueTaskSource(bool runContinuationsAsynchronously = true)
/// </summary>
public bool IsCompleted => (State)Volatile.Read(ref Unsafe.As<State, byte>(ref _state)) == State.Completed;

// TODO: Revisit this with https://github.com/dotnet/runtime/issues/79818 and https://github.com/dotnet/runtime/issues/79911
public bool KeepAliveReleased
{
get
{
lock (this)
{
return !_keepAlive.IsAllocated;
}
}
}

/// <summary>
/// Tries to get a value task representing this task source. If this task source is <see cref="State.None"/>, it'll also transition it into <see cref="State.Awaiting"/> state.
/// It prevents concurrent operations from being invoked since it'll return <c>false</c> if the task source was already in <see cref="State.Awaiting"/> state.
Expand Down Expand Up @@ -153,7 +165,7 @@ private bool TryComplete(Exception? exception, bool final)
// Unblock the current task source and in case of a final also the final task source.
if (exception is not null)
{
// Set up the exception stack strace for the caller.
// Set up the exception stack trace for the caller.
exception = exception.StackTrace is null ? ExceptionDispatchInfo.SetCurrentStackTrace(exception) : exception;
if (state == State.None ||
state == State.Awaiting)
Expand Down
63 changes: 12 additions & 51 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics;
using System.Net.Security;
using System.Net.Sockets;
using System.Runtime.CompilerServices;
Expand Down Expand Up @@ -329,8 +330,6 @@ private async ValueTask FinishConnectAsync(QuicClientConnectionOptions options,

internal ValueTask FinishHandshakeAsync(QuicServerConnectionOptions options, string targetHost, CancellationToken cancellationToken = default)
{
ObjectDisposedException.ThrowIf(_disposed == 1, this);

if (_connectedTcs.TryInitialize(out ValueTask valueTask, this, cancellationToken))
{
_canAccept = options.MaxInboundBidirectionalStreams > 0 || options.MaxInboundUnidirectionalStreams > 0;
Expand Down Expand Up @@ -473,19 +472,13 @@ private unsafe int HandleEventConnected(ref CONNECTED_DATA data)

if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(this, $"{this} Received event CONNECTED {LocalEndPoint} -> {RemoteEndPoint}");
NetEventSource.Info(this, $"{this} Connection connected {LocalEndPoint} -> {RemoteEndPoint} for {_negotiatedApplicationProtocol} protocol");
}

_connectedTcs.TrySetResult();
return QUIC_STATUS_SUCCESS;
}
private unsafe int HandleEventShutdownInitiatedByTransport(ref SHUTDOWN_INITIATED_BY_TRANSPORT_DATA data)
{
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(this, $"{this} Received event SHUTDOWN_INITIATED_BY_TRANSPORT with {nameof(data.Status)}={data.Status}");
}

// TODO: we should propagate transport error code.
// https://github.com/dotnet/runtime/issues/72666
Exception exception = ExceptionDispatchInfo.SetCurrentStackTrace(ThrowHelper.GetExceptionForMsQuicStatus(data.Status));
Expand All @@ -495,52 +488,29 @@ private unsafe int HandleEventShutdownInitiatedByTransport(ref SHUTDOWN_INITIATE
}
private unsafe int HandleEventShutdownInitiatedByPeer(ref SHUTDOWN_INITIATED_BY_PEER_DATA data)
{
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(this, $"{this} Received event SHUTDOWN_INITIATED_BY_PEER_DATA with {nameof(data.ErrorCode)}={data.ErrorCode}");
}

_acceptQueue.Writer.TryComplete(ExceptionDispatchInfo.SetCurrentStackTrace(ThrowHelper.GetConnectionAbortedException((long)data.ErrorCode)));
return QUIC_STATUS_SUCCESS;
}
private unsafe int HandleEventShutdownComplete()
{
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(this, $"{this} Received event SHUTDOWN_INITIATED_BY_PEER_DATA");
}

_acceptQueue.Writer.TryComplete(ExceptionDispatchInfo.SetCurrentStackTrace(ThrowHelper.GetOperationAbortedException()));
Exception exception = ExceptionDispatchInfo.SetCurrentStackTrace(ThrowHelper.GetOperationAbortedException());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will allocate exception and stack in all cases right?I would hope we should be able to figure out the connection state and do it only when needed. I'm ok if we do it separately as improvement to unblock asp.net.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The allocation was there before the change, I'm just re-using the same exception in another place now.
As this happens once per a connection, I'm not overly concerned about the cost. But it would be a nice improvement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the comment. Where was the exception allocated before for HandleEventShutdownComplete? And I agree that this is not critical since that is once per connection. But walking the stack may note be cheap as allocation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_acceptQueue.Writer.TryComplete(ExceptionDispatchInfo.SetCurrentStackTrace(ThrowHelper.GetOperationAbortedException()));

_acceptQueue.Writer.TryComplete(exception);
_connectedTcs.TrySetException(exception);
_shutdownTcs.TrySetResult();
return QUIC_STATUS_SUCCESS;
}
private unsafe int HandleEventLocalAddressChanged(ref LOCAL_ADDRESS_CHANGED_DATA data)
{
_localEndPoint = data.Address->ToIPEndPoint();
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(this, $"{this} Received event LOCAL_ADDRESS_CHANGED with {nameof(data.Address)}={_localEndPoint}");
}

return QUIC_STATUS_SUCCESS;
}
private unsafe int HandleEventPeerAddressChanged(ref PEER_ADDRESS_CHANGED_DATA data)
{
_remoteEndPoint = data.Address->ToIPEndPoint();
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(this, $"{this} Received event LOCAL_ADDRESS_CHANGED with {nameof(data.Address)}={_remoteEndPoint}");
}

return QUIC_STATUS_SUCCESS;
}
private unsafe int HandleEventPeerStreamStarted(ref PEER_STREAM_STARTED_DATA data)
{
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(this, $"{this} Received event PEER_STREAM_STARTED");
}

QuicStream stream = new QuicStream(_handle, data.Stream, data.Flags, _defaultStreamErrorCode);
if (!_acceptQueue.Writer.TryWrite(stream))
{
Expand All @@ -557,11 +527,6 @@ private unsafe int HandleEventPeerStreamStarted(ref PEER_STREAM_STARTED_DATA dat
}
private unsafe int HandleEventPeerCertificateReceived(ref PEER_CERTIFICATE_RECEIVED_DATA data)
{
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(this, $"{this} Received event PEER_CERTIFICATE_RECEIVED_DATA");
}

try
{
return _sslConnectionOptions.ValidateCertificate((QUIC_BUFFER*)data.Certificate, (QUIC_BUFFER*)data.Chain, out _remoteCertificate);
Expand All @@ -573,16 +538,6 @@ private unsafe int HandleEventPeerCertificateReceived(ref PEER_CERTIFICATE_RECEI
}
}

private int HandleConnectionEvent(QUIC_CONNECTION_EVENT_TYPE type)
{
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(this, $"{this} Received event {type}");
}

return QUIC_STATUS_SUCCESS;
}

private unsafe int HandleConnectionEvent(ref QUIC_CONNECTION_EVENT connectionEvent)
=> connectionEvent.Type switch
{
Expand All @@ -594,7 +549,7 @@ private unsafe int HandleConnectionEvent(ref QUIC_CONNECTION_EVENT connectionEve
QUIC_CONNECTION_EVENT_TYPE.PEER_ADDRESS_CHANGED => HandleEventPeerAddressChanged(ref connectionEvent.PEER_ADDRESS_CHANGED),
QUIC_CONNECTION_EVENT_TYPE.PEER_STREAM_STARTED => HandleEventPeerStreamStarted(ref connectionEvent.PEER_STREAM_STARTED),
QUIC_CONNECTION_EVENT_TYPE.PEER_CERTIFICATE_RECEIVED => HandleEventPeerCertificateReceived(ref connectionEvent.PEER_CERTIFICATE_RECEIVED),
_ => HandleConnectionEvent(connectionEvent.Type),
_ => QUIC_STATUS_SUCCESS,
};

#pragma warning disable CS3016
Expand All @@ -616,6 +571,11 @@ private static unsafe int NativeCallback(QUIC_HANDLE* connection, void* context,

try
{
// Process the event.
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(instance, $"{instance} Received event {connectionEvent->Type} {connectionEvent->ToString()}");
}
return instance.HandleConnectionEvent(ref *connectionEvent);
}
catch (Exception ex)
Expand Down Expand Up @@ -654,6 +614,7 @@ public async ValueTask DisposeAsync()

// Wait for SHUTDOWN_COMPLETE, the last event, so that all resources can be safely released.
await valueTask.ConfigureAwait(false);
Debug.Assert(_connectedTcs.IsCompleted);
_handle.Dispose();

_configuration?.Dispose();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ private static unsafe int NativeCallback(QUIC_HANDLE* listener, void* context, Q
// Process the event.
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(instance, $"{instance} Received event {listenerEvent->Type}");
NetEventSource.Info(instance, $"{instance} Received event {listenerEvent->Type} {listenerEvent->ToString()}");
}
return instance.HandleListenerEvent(ref *listenerEvent);
}
Expand Down
Loading