Fix hard crashes for HMAC on Linux

Author: vcsjones

src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/HashProviderDispenser.Unix.cs

@@ -159,7 +159,7 @@ public override int GetCurrentHash(Span<byte> destination)
 
             public override void Dispose(bool disposing)
             {
-                // We don't need to worry about the callCount here because the SafeHandle takes care of reference
+                // We don't need to worry about the _callOperations here because the SafeHandle takes care of reference
                 // counting the dispose.
 
                 if (disposing)
@@ -198,34 +198,79 @@ private sealed class HmacHashProvider : HashProvider
             private readonly LiteHmac _liteHmac;
             private bool _running;
 
+            // HashAlgorithm and IncrementalHash are not thread safe. However, OpenSSL is not thread safe to the point
+            // that calling EVP_DigestUpdate on an EVP_MD_CTX concurrently may result in process crashes from illegal
+            // memory accesses. We don't want the process to crash, so we need to keep track of the number of operations
+            // that are in-flight. If the answer is more than one, throw a managed exception.
+            private int _callOperations;
+
             public HmacHashProvider(string hashAlgorithmId, ReadOnlySpan<byte> key)
             {
                 _liteHmac = LiteHashProvider.CreateHmac(hashAlgorithmId, key);
             }
 
             public override void AppendHashData(ReadOnlySpan<byte> data)
             {
-                _liteHmac.Append(data);
-                _running = true;
+                try
+                {
+                    if (Interlocked.Increment(ref _callOperations) != 1)
+                    {
+                        throw GetConcurrentUseException();
+                    }
+
+                    _liteHmac.Append(data);
+                    _running = true;
+                }
+                finally
+                {
+                    Interlocked.Decrement(ref _callOperations);
+                }
             }
 
             public override int FinalizeHashAndReset(Span<byte> destination)
             {
-                int written = _liteHmac.Finalize(destination);
-                _liteHmac.Reset();
-                _running = false;
-                return written;
+                try
+                {
+                    if (Interlocked.Increment(ref _callOperations) != 1)
+                    {
+                        throw GetConcurrentUseException();
+                    }
+
+                    int written = _liteHmac.Finalize(destination);
+                    _liteHmac.Reset();
+                    _running = false;
+                    return written;
+                }
+                finally
+                {
+                    Interlocked.Decrement(ref _callOperations);
+                }
             }
 
             public override int GetCurrentHash(Span<byte> destination)
             {
-                return _liteHmac.Current(destination);
+                try
+                {
+                    if (Interlocked.Increment(ref _callOperations) != 1)
+                    {
+                        throw GetConcurrentUseException();
+                    }
+
+                    return _liteHmac.Current(destination);
+                }
+                finally
+                {
+                    Interlocked.Decrement(ref _callOperations);
+                }
             }
 
             public override int HashSizeInBytes => _liteHmac.HashSizeInBytes;
 
             public override void Dispose(bool disposing)
             {
+                // We don't need to worry about the _callOperations here because the SafeHandle takes care of reference
+                // counting the dispose.
+
                 if (disposing)
                 {
                     _liteHmac.Dispose();
@@ -234,12 +279,27 @@ public override void Dispose(bool disposing)
 
             public override void Reset()
             {
-                if (_running)
+                try
                 {
-                    _liteHmac.Reset();
-                    _running = false;
+                    if (Interlocked.Increment(ref _callOperations) != 1)
+                    {
+                        throw GetConcurrentUseException();
+                    }
+
+                    if (_running)
+                    {
+                        _liteHmac.Reset();
+                        _running = false;
+                    }
+                }
+                finally
+                {
+                    Interlocked.Decrement(ref _callOperations);
                 }
             }
+
+            private static CryptographicException GetConcurrentUseException() =>
+                new CryptographicException(SR.Cryptography_ConcurrentUseNotSupported);
         }
     }
 }

src/libraries/System.Security.Cryptography/tests/IncrementalHashTests.cs

@@ -2,6 +2,8 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Collections.Generic;
+using System.Threading;
+using Microsoft.DotNet.RemoteExecutor;
 using Test.Cryptography;
 using Xunit;
 
@@ -622,6 +624,106 @@ public static void VerifyBounds_GetHashAndReset_Hash(HashAlgorithm referenceAlgo
             }
         }
 
+        [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
+        public static void Hash_GetHashAndReset_ConcurrentUseDoesNotCrashProcess()
+        {
+            static void ThreadWork(object obj)
+            {
+                try
+                {
+                    IncrementalHash hash = (IncrementalHash)obj;
+
+                    for (int i = 0; i < 10_000; i++)
+                    {
+                        hash.AppendData("potatos and carrots make for a fine stew."u8);
+                        hash.GetHashAndReset();
+                    }
+                }
+                catch
+                {
+                    // Ignore all managed exceptions. IncrementalHash is not thread safe, but we don't want process
+                    // crashes.
+                }
+            }
+
+            RemoteExecutor.Invoke(static () =>
+            {
+                foreach(object[] items in GetHashAlgorithms())
+                {
+                    if (items is [HashAlgorithm referenceAlgorithm, HashAlgorithmName hashAlgorithm])
+                    {
+                         referenceAlgorithm.Dispose();
+
+                        using (IncrementalHash hash = IncrementalHash.CreateHash(hashAlgorithm))
+                        {
+                            Thread thread1 = new(ThreadWork);
+                            Thread thread2 = new(ThreadWork);
+                            thread1.Start(hash);
+                            thread2.Start(hash);
+                            thread1.Join();
+                            thread2.Join();
+                        }
+                    }
+                    else
+                    {
+                        Assert.Fail("Test is not set up correctly.");
+                    }
+                }
+
+                return RemoteExecutor.SuccessExitCode;
+            }).Dispose();
+        }
+
+        [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
+        public static void HMAC_GetHashAndReset_ConcurrentUseDoesNotCrashProcess()
+        {
+            static void ThreadWork(object obj)
+            {
+                try
+                {
+                    IncrementalHash hash = (IncrementalHash)obj;
+
+                    for (int i = 0; i < 10_000; i++)
+                    {
+                        hash.AppendData("potatos and carrots make for a fine stew."u8);
+                        hash.GetHashAndReset();
+                    }
+                }
+                catch
+                {
+                    // Ignore all managed exceptions. IncrementalHash is not thread safe, but we don't want process
+                    // crashes.
+                }
+            }
+
+            RemoteExecutor.Invoke(static () =>
+            {
+                foreach(object[] items in GetHMACs())
+                {
+                    if (items is [HashAlgorithm referenceAlgorithm, HashAlgorithmName hashAlgorithm])
+                    {
+                         referenceAlgorithm.Dispose();
+
+                        using (IncrementalHash hash = IncrementalHash.CreateHMAC(hashAlgorithm, [1, 2, 3, 4]))
+                        {
+                            Thread thread1 = new(ThreadWork);
+                            Thread thread2 = new(ThreadWork);
+                            thread1.Start(hash);
+                            thread2.Start(hash);
+                            thread1.Join();
+                            thread2.Join();
+                        }
+                    }
+                    else
+                    {
+                        Assert.Fail("Test is not set up correctly.");
+                    }
+                }
+
+                return RemoteExecutor.SuccessExitCode;
+            }).Dispose();
+        }
+
         private static void VerifyGetCurrentHash(IncrementalHash single, IncrementalHash accumulated)
         {
             Span<byte> buf = stackalloc byte[2048];