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

Consider referencing System.Memory #550

Open
Rob-Hague opened this issue Jul 9, 2024 · 1 comment
Open

Consider referencing System.Memory #550

Rob-Hague opened this issue Jul 9, 2024 · 1 comment

Comments

@Rob-Hague
Copy link
Contributor

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
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant