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.

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

Avoid some defensive copies on readonly structs in System.Net.Quic #101133

Merged
merged 9 commits into from
Apr 25, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Collections.Generic;
using System.Collections.Concurrent;
using System.Collections.ObjectModel;
using System.Security.Authentication;
using System.Net.Security;
using System.Runtime.InteropServices;
using System.Security.Cryptography.X509Certificates;
using System.Threading;
using Microsoft.Quic;
Expand Down Expand Up @@ -47,6 +49,8 @@ private static bool GetConfigurationCacheEnabled()
public readonly List<SslApplicationProtocol> ApplicationProtocols;
public readonly QUIC_ALLOWED_CIPHER_SUITE_FLAGS AllowedCipherSuites;

[UnscopedRef] private readonly ReadOnlySpan<byte> SettingsSpan => MemoryMarshal.AsBytes(new ReadOnlySpan<QUIC_SETTINGS>(in Settings));

public CacheKey(QUIC_SETTINGS settings, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection<X509Certificate2>? intermediates, List<SslApplicationProtocol> alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites)
{
CertificateThumbprints = certificate == null ? new List<byte[]>() : new List<byte[]> { certificate.GetCertHash() };
Expand Down Expand Up @@ -98,7 +102,7 @@ public bool Equals(CacheKey other)

return
Flags == other.Flags &&
Settings.Equals(other.Settings) &&
SettingsSpan.SequenceEqual(other.SettingsSpan) &&
AllowedCipherSuites == other.AllowedCipherSuites;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ internal sealed class MsQuicContextSafeHandle : MsQuicSafeHandle
/// Holds a weak reference to the managed instance.
/// It allows delegating MsQuic events to the managed object while it still can be collected and finalized.
/// </summary>
private readonly GCHandle _context;
private GCHandle _context;
Copy link
Member

Choose a reason for hiding this comment

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

Unintended leftover change?

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 one should stay, otherwise the .Free() is executed on a defensive copy

It probably is not a big deal since ReleaseHandle runs exactly once, but we still make sure to cleanup dangling pointers.


/// <summary>
/// Optional parent safe handle, used to increment/decrement reference count with the lifetime of this instance.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public int CopyFrom(ReadOnlySpan<QUIC_BUFFER> quicBuffers, int totalLength, bool
int totalCopied = 0;
for (int i = 0; i < quicBuffers.Length; ++i)
{
Span<byte> quicBuffer = quicBuffers[i].Span;
ReadOnlySpan<byte> quicBuffer = quicBuffers[i].Span;
if (totalLength < quicBuffer.Length)
{
quicBuffer = quicBuffer.Slice(0, totalLength);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ namespace Microsoft.Quic
{
internal unsafe partial struct QUIC_BUFFER
{
public Span<byte> Span => new(Buffer, (int)Length);
public readonly Span<byte> Span => new(Buffer, (int)Length);
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 remember exactly, but if this is generated code from MsQuic, it should be fixed there as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

right, I thought this was added manually by us (since the QUIC_BUFFER definition is in another auto-generated file). I will follow up to make the change in their repo as well.

}

internal partial class MsQuic
Expand Down
Loading