Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

[QUIC] Fix rooted connection when hanshake failed #87328

Merged
merged 7 commits into from
Jun 14, 2023
Merged

[QUIC] Fix rooted connection when hanshake failed #87328

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0373fa5
Added tests
ManickaP Jun 9, 2023
4b16a36
Fixed logging and minor typos
ManickaP Jun 9, 2023
2f394e0
Fix
ManickaP Jun 9, 2023
2e296cb
Fix test
ManickaP Jun 9, 2023
8a80680
"Added asserts checking GCHandle state"
ManickaP Jun 12, 2023
efa9839
Exclude test from mono
ManickaP Jun 13, 2023
d2c218a
Removed unnecessary _disposed checks
ManickaP Jun 14, 2023
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
71 changes: 71 additions & 0 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicExtensions.cs
View file
Open in desktop
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
};
}
4 changes: 2 additions & 2 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicHelpers.cs
View file
Open in desktop
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
61 changes: 12 additions & 49 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,8 @@ internal unsafe QuicConnection(QUIC_HANDLE* handle, QUIC_NEW_CONNECTION_INFO* in

private async ValueTask FinishConnectAsync(QuicClientConnectionOptions options, CancellationToken cancellationToken = default)
{
ObjectDisposedException.ThrowIf(_disposed == 1, this);
Copy link
Member

Choose a reason for hiding this comment

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

BTW I still feel this is dead code and should not be here. But that can be addresses separately.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was bad merge. I merged some files manually from an older branch and must have missed it. I removed it and looked for other places as well.


if (_connectedTcs.TryInitialize(out ValueTask valueTask, this, cancellationToken))
{
_canAccept = options.MaxInboundBidirectionalStreams > 0 || options.MaxInboundUnidirectionalStreams > 0;
Expand Down Expand Up @@ -473,19 +475,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 +491,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.

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Line 513 in 90c5f05

_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 +530,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 +541,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 +552,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 +574,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
2 changes: 1 addition & 1 deletion src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs
View file
Open in desktop
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