Fix Guid handling (Azure#33881)

* Fix Guid handling

* Move to separate class

* using

* Optimize Guid creation

* Optimize big endian write

* Move to core

* Rename

* Fix

* Update parse handling

* Fix SB tests and revert Parse change

* PR fb

* Apply suggestions from code review

Co-authored-by: Daniel Marbach <daniel.marbach@openplace.net>

* Set out param

* Add helper

---------

Co-authored-by: danielmarbach <daniel.marbach@openplace.net>

Author: JoshLove-msft

sdk/core/Azure.Core/src/Shared/GuidUtilities.cs

@@ -0,0 +1,120 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System;
+using System.Buffers.Binary;
+using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
+
+namespace Azure.Core.Shared;
+
+/// <summary>
+/// The purpose of this class is to provide an allocation free way to parse and write Guids on .NET Standard 2.0, taking into account
+/// endianness.
+/// </summary>
+internal static class GuidUtilities
+{
+    private const int GuidSizeInBytes = 16;
+
+    public static bool TryParseGuidBytes(ReadOnlySpan<byte> bytes, out Guid guid)
+    {
+        if (bytes.Length != GuidSizeInBytes)
+        {
+            guid = default;
+            return false;
+        }
+
+        if (BitConverter.IsLittleEndian)
+        {
+            guid = MemoryMarshal.Read<Guid>(bytes);
+            return true;
+        }
+
+        // copied from https://github.com/dotnet/runtime/blob/9129083c2fc6ef32479168f0555875b54aee4dfb/src/libraries/System.Private.CoreLib/src/System/Guid.cs#L49
+        // slower path for BigEndian:
+        byte k = bytes[15];  // hoist bounds checks
+        int a = BinaryPrimitives.ReadInt32LittleEndian(bytes);
+        short b = BinaryPrimitives.ReadInt16LittleEndian(bytes.Slice(4));
+        short c = BinaryPrimitives.ReadInt16LittleEndian(bytes.Slice(6));
+        byte d = bytes[8];
+        byte e = bytes[9];
+        byte f = bytes[10];
+        byte g = bytes[11];
+        byte h = bytes[12];
+        byte i = bytes[13];
+        byte j = bytes[14];
+
+        guid = new Guid(a, b, c, d, e, f, g, h, i, j, k);
+        return true;
+    }
+
+    public static void WriteGuidToBuffer(Guid guid, Span<byte> buffer)
+    {
+        AssertBufferSize(buffer);
+
+        // Based on https://github.com/dotnet/runtime/blob/9129083c2fc6ef32479168f0555875b54aee4dfb/src/libraries/System.Private.CoreLib/src/System/Guid.cs#L836
+
+        if (BitConverter.IsLittleEndian)
+        {
+            MemoryMarshal.Write(buffer, ref guid);
+            return;
+        }
+
+        GuidData data = Unsafe.As<Guid, GuidData>(ref guid);
+
+        buffer[15] = data.K; // hoist bounds checks
+        BinaryPrimitives.WriteInt32LittleEndian(buffer, data.A);
+        BinaryPrimitives.WriteInt16LittleEndian(buffer.Slice(4), data.B);
+        BinaryPrimitives.WriteInt16LittleEndian(buffer.Slice(6), data.C);
+        buffer[8] = data.D;
+        buffer[9] = data.E;
+        buffer[10] = data.F;
+        buffer[11] = data.G;
+        buffer[12] = data.H;
+        buffer[13] = data.I;
+        buffer[14] = data.J;
+    }
+
+    private static void AssertBufferSize(Span<byte> buffer)
+    {
+        if (buffer.Length != GuidSizeInBytes)
+        {
+            throw new ArgumentException($"The length of the supplied buffer must be {GuidSizeInBytes}.", nameof(buffer));
+        }
+    }
+
+    // This struct has the fields layed out to be GUID-like in order to read the GUID fields
+    // to efficiently write them into memory without having to deal with endianness
+    // Do not rename or reorder the fields.
+    private readonly struct GuidData
+    {
+        public readonly int A;
+        public readonly short B;
+        public readonly short C;
+        public readonly byte D;
+        public readonly byte E;
+        public readonly byte F;
+        public readonly byte G;
+        public readonly byte H;
+        public readonly byte I;
+        public readonly byte J;
+        public readonly byte K;
+
+        // Creates a new GUID like struct initialized to the value represented by the
+        // arguments.  The bytes are specified like this to avoid endianness issues.
+        public GuidData(int a, short b, short c, byte d, byte e, byte f, byte g, byte h, byte i, byte j, byte k)
+        {
+            A = a;
+            B = b;
+            C = c;
+            D = d;
+            E = e;
+            F = f;
+            G = g;
+            H = h;
+            I = i;
+            J = j;
+            K = k;
+        }
+    }
+}

sdk/core/Azure.Core/tests/Azure.Core.Tests.csproj

@@ -49,6 +49,7 @@
     <Compile Include="..\..\Azure.Core.TestFramework\src\Instrumentation\ZeroPollingStrategy.cs" LinkBase="Shared\TestFramework" />
     <Compile Include="$(TestFrameworkSupportFiles)" LinkBase="Shared\TestFramework" />
     <Compile Include="..\src\Shared\ApiVersionString.cs" LinkBase="Shared" />
+    <Compile Include="..\src\Shared\GuidUtilities.cs" LinkBase="Shared" />
   </ItemGroup>
   <ItemGroup>
     <Compile Update="PemReaderTests.Keys.cs" DependentUpon="PemReaderTests.cs" />

sdk/core/Azure.Core/tests/GuidUtilitiesTests.cs

@@ -0,0 +1,45 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System;
+using Azure.Core.Shared;
+using NUnit.Framework;
+
+namespace Azure.Core.Tests;
+
+public class GuidUtilitiesTests
+{
+    [Test]
+    public void CanParseGuidBytes()
+    {
+        var input = Guid.NewGuid();
+
+        Assert.IsTrue(GuidUtilities.TryParseGuidBytes(input.ToByteArray(), out Guid output));
+        Assert.AreEqual(input, output);
+    }
+
+    [Test]
+    public void TryParseGuidBytesReturnsFalseOnInvalidInput()
+    {
+        Assert.IsFalse(GuidUtilities.TryParseGuidBytes(new byte[15], out Guid guid));
+        Assert.AreEqual(default(Guid), guid);
+    }
+
+    [Test]
+    public void CanWriteGuidBytes()
+    {
+        var input = Guid.NewGuid();
+        var buffer = new byte[16];
+
+        GuidUtilities.WriteGuidToBuffer(input, buffer);
+        CollectionAssert.AreEqual(input.ToByteArray(), buffer);
+    }
+
+    [Test]
+    public void WriteGuidBytesThrowsOnInvalidBuffer()
+    {
+        var input = Guid.NewGuid();
+        var buffer = new byte[15];
+        Assert.Throws<ArgumentException>(() => GuidUtilities.WriteGuidToBuffer(input, buffer));
+    }
+}

sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpMessageConverter.cs

@@ -10,6 +10,7 @@
 using System.Runtime.Serialization;
 using Azure.Core;
 using Azure.Core.Amqp;
+using Azure.Core.Shared;
 using Azure.Messaging.ServiceBus.Primitives;
 using Microsoft.Azure.Amqp;
 using Microsoft.Azure.Amqp.Encoding;
@@ -19,11 +20,6 @@ namespace Azure.Messaging.ServiceBus.Amqp
 {
     internal class AmqpMessageConverter
     {
-        /// <summary>
-        /// The size, in bytes, to use for extracting the delivery tag bytes into <see cref="Guid"/>.
-        /// </summary>
-        private const int GuidSizeInBytes = 16;
-
         /// <summary>The size, in bytes, to use as a buffer for stream operations.</summary>
         private const int StreamBufferSizeInBytes = 512;
 
@@ -554,33 +550,17 @@ public virtual ServiceBusReceivedMessage AmqpMessageToSBReceivedMessage(AmqpMess
             AmqpAnnotatedMessage annotatedMessage = AmqpMessageToAnnotatedMessage(amqpMessage, isPeeked);
 
             ServiceBusReceivedMessage sbMessage = new ServiceBusReceivedMessage(annotatedMessage);
-
-            // lock token
-
-            sbMessage.LockTokenGuid = ParseGuidBytes(amqpMessage.DeliveryTag);
+            if (GuidUtilities.TryParseGuidBytes(amqpMessage.DeliveryTag, out Guid lockToken))
+            {
+                // lock token
+                sbMessage.LockTokenGuid = lockToken;
+            };
 
             amqpMessage.Dispose();
 
             return sbMessage;
         }
 
-        public virtual Guid ParseGuidBytes(ReadOnlyMemory<byte> bytes)
-        {
-            if (bytes.Length == GuidSizeInBytes)
-            {
-                // Use TryRead to avoid allocating an array if we are on a little endian machine.
-                if (!BitConverter.IsLittleEndian || !MemoryMarshal.TryRead<Guid>(bytes.Span, out var lockTokenGuid))
-                {
-                    // Either we are on a big endian machine or the bytes were not a valid GUID.
-                    // Even if the bytes were not valid, use the Guid constructor to leverage the Guid validation rather than throwing ourselves.
-                    lockTokenGuid = new Guid(bytes.ToArray());
-                }
-                return lockTokenGuid;
-            }
-
-            return default;
-        }
-
         internal static bool TryGetAmqpObjectFromNetObject(object netObject, MappingType mappingType, out object amqpObject)
         {
             amqpObject = null;

sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpReceiver.cs

@@ -6,12 +6,12 @@
 using System.Collections.Generic;
 using System.Collections.ObjectModel;
 using System.Runtime.ExceptionServices;
-using System.Runtime.InteropServices;
 using System.Threading;
 using System.Threading.Tasks;
 using System.Transactions;
 using Azure.Core;
 using Azure.Core.Diagnostics;
+using Azure.Core.Shared;
 using Azure.Messaging.ServiceBus.Core;
 using Azure.Messaging.ServiceBus.Diagnostics;
 using Azure.Messaging.ServiceBus.Primitives;
@@ -476,10 +476,7 @@ private async Task DisposeMessageAsync(
             TimeSpan timeout)
         {
             byte[] bufferForLockToken = ArrayPool<byte>.Shared.Rent(SizeOfGuidInBytes);
-            if (!MemoryMarshal.TryWrite(bufferForLockToken, ref lockToken))
-            {
-                lockToken.ToByteArray().AsSpan().CopyTo(bufferForLockToken);
-            }
+            GuidUtilities.WriteGuidToBuffer(lockToken, bufferForLockToken);
 
             ArraySegment<byte> deliveryTag = new ArraySegment<byte>(bufferForLockToken, 0, SizeOfGuidInBytes);
             ReceivingAmqpLink receiveLink = null;

sdk/servicebus/Azure.Messaging.ServiceBus/src/Azure.Messaging.ServiceBus.csproj

@@ -43,6 +43,7 @@
     <Compile Include="$(AzureCoreSharedSources)ValueStopwatch.cs" LinkBase="SharedSource\Azure.Core" />
     <Compile Include="$(AzureCoreSharedSources)PageResponseEnumerator.cs" LinkBase="SharedSource\Azure.Core" />
     <Compile Include="$(AzureCoreSharedSources)AzureResourceProviderNamespaceAttribute.cs" LinkBase="SharedSource\Azure.Core" />
+    <Compile Include="$(AzureCoreSharedSources)GuidUtilities.cs" LinkBase="SharedSource\Azure.Core" />
   </ItemGroup>
 
   <ItemGroup>

sdk/servicebus/Azure.Messaging.ServiceBus/src/Compatibility/ServiceBusAmqpExtensions.cs

@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 
 using System;
+using Azure.Core.Shared;
 using Azure.Messaging.ServiceBus.Amqp;
 using Microsoft.Azure.Amqp;
 using AmqpLib = Microsoft.Azure.Amqp;
@@ -35,7 +36,10 @@ public static ServiceBusReceivedMessage FromAmqpBytes(BinaryData messageBytes, B
             var amqpMessage = AmqpLib.AmqpMessage.CreateInputMessage(bufferStream);
 
             var message = AmqpMessageConverter.Default.AmqpMessageToSBReceivedMessage(amqpMessage);
-            message.LockTokenGuid = AmqpMessageConverter.Default.ParseGuidBytes(lockTokenBytes);
+            if (GuidUtilities.TryParseGuidBytes(lockTokenBytes, out Guid lockToken))
+            {
+                message.LockTokenGuid = lockToken;
+            }
             return message;
         }
     }