bcgit / bc-csharp

BouncyCastle.NET Cryptography Library (Mirror)
https://www.bouncycastle.org/csharp
MIT License
1.68k stars 559 forks source link

Consider referencing System.Memory #550

Open Rob-Hague opened 4 months ago

Rob-Hague commented 4 months ago

Perhaps you've already considered it, but in case you haven't:

The library has many duplicate code paths, one using a Span implementation for higher targets (.NET) and one using arrays for lower targets (.NET Framework and .NET Standard 2.0)

Microsoft provide (and support) the System.Memory package, which allows use of Span etc. on .NET Framework and .NET Standard 2.0. It is fairly common for multi-targeting libraries to reference the package on lower targets. The source code is at https://github.com/dotnet/maintenance-packages/tree/main/src/System.Memory.

This reference would allow many code paths (and public API) to be unified.

Practically, the initial change would look like:

diff --git a/crypto/src/BouncyCastle.Crypto.csproj b/crypto/src/BouncyCastle.Crypto.csproj
index 8b7c8852..166dc1ae 100644
--- a/crypto/src/BouncyCastle.Crypto.csproj
+++ b/crypto/src/BouncyCastle.Crypto.csproj
@@ -88,6 +88,10 @@
     <None Include="..\..\README.md" Pack="true" PackagePath="\" />
   </ItemGroup>

+  <ItemGroup Condition=" '$(TargetFramework)' == 'net461' or '$(TargetFramework)' == 'netstandard2.0' ">
+    <PackageReference Include="System.Memory" Version="4.5.5" />
+  </ItemGroup>
+
   <ItemGroup>
     <PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.3">
       <PrivateAssets>all</PrivateAssets>

Example unified diff (note Index/Range operators [..32] not supported):

diff --git a/crypto/src/crypto/modes/ChaCha20Poly1305.cs b/crypto/src/crypto/modes/ChaCha20Poly1305.cs
index 56bef5e9..ca982f9d 100644
--- a/crypto/src/crypto/modes/ChaCha20Poly1305.cs
+++ b/crypto/src/crypto/modes/ChaCha20Poly1305.cs
@@ -783,29 +783,16 @@ private ulong IncrementCount(ulong count, uint increment, ulong limit)

         private void InitMac()
         {
-#if NETCOREAPP2_1_OR_GREATER || NETSTANDARD2_1_OR_GREATER
             Span<byte> firstBlock = stackalloc byte[64];
             try
             {
                 mChacha20.ProcessBytes(firstBlock, firstBlock);
-                mPoly1305.Init(new KeyParameter(firstBlock[..32]));
+                mPoly1305.Init(new KeyParameter(firstBlock.Slice(0, 32)));
             }
             finally
             {
                 firstBlock.Fill(0x00);
             }
-#else
-            byte[] firstBlock = new byte[64];
-            try
-            {
-                mChacha20.ProcessBytes(firstBlock, 0, 64, firstBlock, 0);
-                mPoly1305.Init(new KeyParameter(firstBlock, 0, 32));
-            }
-            finally
-            {
-                Array.Clear(firstBlock, 0, 64);
-            }
-#endif
         }

         private void PadMac(ulong count)
Rob-Hague commented 2 months ago

@peterdettman would you be open to this? Happy to get it started. As it's a big job, it can be done in pieces starting in the lower levels of the library.

It's also possible to keep the Index/Range syntax using a polyfill of those types and a reference to System.ValueTuple on net461.

scott-xu commented 1 week ago

Suggest ref https://www.nuget.org/packages/Microsoft.Bcl.Memory#readme-body-tab which includes Index and Range. It also depends on System.Memory.

Rob-Hague commented 1 week ago

Good idea. Unfortunately this library targets .NET Framework 4.6.1 which is out of support and that library targets 4.6.2, so we would at least still need an explicit reference to System.ValueTuple on net461