The method returns inconsistent expected results after running it about 10,000 times

[czd890]

Description

I use fixed parameters to loop the test. After running about 10,000 times, the returned decryptedByteCount will change from 9 to 1. This happens widely on multiple net8 versions of Linux and Windows.

the code should use release to build. If it is debug mode, this bug cannot be reproduced.

Does this seem to be related to the JIT not optimizing correctly?

        public int Decrypt(byte[] buffer, string key, string iv, out byte[] decryptedData)
        {
            int decryptedByteCount = 0;
            decryptedData = new byte[buffer.Length];

            using var aes = Aes.Create();
            aes.Mode = CipherMode.CBC;
            aes.KeySize = 16;
            aes.Padding = PaddingMode.Zeros;
            
            var instPwdArray = Encoding.ASCII.GetBytes(key);
            var instSaltArray = Encoding.ASCII.GetBytes(iv);

            using (var decryptor = aes.CreateDecryptor(instPwdArray, instSaltArray))
            {
                using (var memoryStream = new MemoryStream(buffer))
                {
                    using (var cryptoStream = new CryptoStream(memoryStream, decryptor, CryptoStreamMode.Read))
                    {
                        int read;
                        do
                        {
                            read = cryptoStream.Read(
                                decryptedData,
                                decryptedByteCount,
                                decryptedData.Length - decryptedByteCount);
                            decryptedByteCount += read;
                        } while (read != 0);
                    }
                }
            }

            // Found the accurate length of decrypted data
            while (decryptedData[decryptedByteCount - 1] == 0 && decryptedByteCount > 0)
            {
                decryptedByteCount--;
            }

            return decryptedByteCount;
        }

test parameters

var hex = "3AE46205A50ED00E7612A91692552B7A";
var key = "abcdef1234567890";
var iv = "abcdef1234567890";
Decrypt(convert_hex_to_bytes(hex),key,iv, out decrptorBytes)

If I modify the code to the following two methods, this problem can be fixed.

method1: using(){} struct for aes

            ......
            using (var aes = Aes.Create())
            {
                aes.Mode = CipherMode.CBC;
                ......
            }
            while (decryptedData[decryptedByteCount - 1] == 0 && decryptedByteCount > 0)
            ......

method2: add MemoryBarrier.

            ......
            Thread.MemoryBarrier();
            while (decryptedData[decryptedByteCount - 1] == 0 && decryptedByteCount > 0)
            ......

Reproduction Steps

Repeat the run more than 10,000 times.

Expected behavior

The results should always be consistent.

Actual behavior

After running it about 10,000 times, it will always return wrong results.

Regression?

has never happened on net6.

Known Workarounds

No response

Configuration

No response

Other information

No response

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[czd890]

Before the bug appears, the codeaddr of the method is 0000018f23a1c864, it shows is QuickJitted

> ip2md 00007FFE0312323A
MethodDesc:   00007ffe031de8d0
Method Name:          [......].Decrypt(Byte[], System.String, System.String, Byte[] ByRef)
Class:                00007ffe031ef318
MethodTable:          00007ffe031de918
mdToken:              000000000600036B
Module:               00007ffe031dd740
IsJitted:             yes
Current CodeAddr:     00007ffe03122e70
Version History:
  ILCodeVersion:      0000000000000000
  ReJIT ID:           0
  IL Addr:            0000018f23a1c864
     CodeAddr:           00007ffe03122e70  (QuickJitted)
     NativeCodeVersion:  0000000000000000

After the bug appears, the codeaddr of the method is 00007ffe03134620, it shows is OptimizedTier1

> ip2md 00007FFE03134AE3
MethodDesc:   00007ffe031de8d0
Method Name:          [......].Decrypt(Byte[], System.String, System.String, Byte[] ByRef)
Class:                00007ffe031ef318
MethodTable:          00007ffe031de918
mdToken:              000000000600036B
Module:               00007ffe031dd740
IsJitted:             yes
Current CodeAddr:     00007ffe03134620
Version History:
  ILCodeVersion:      0000000000000000
  ReJIT ID:           0
  IL Addr:            0000018f23a1c864
     CodeAddr:           00007ffe03129cb0  (QuickJitted + Instrumented)
     NativeCodeVersion:  0000018F0054A360
     CodeAddr:           00007ffe03134620  (OptimizedTier1)
     NativeCodeVersion:  0000018F02253050
     CodeAddr:           00007ffe03122e70  (QuickJitted)
     NativeCodeVersion:  0000000000000000

[JulieLeeMSFT]

@EgorBo PTAL.

[JulieLeeMSFT]

Cc @dotnet/jit-contrib .

[EgorBo]

@czd890 what is _keySize in your snippet? Also, your test data is string while the function accepts byte[] - do you convert it to utf8 or utf16. so could you please attach a full console-app snippet

[EgorBo]

Ah, I was able to repro. Seems like the issue is .NET8.0 specific. Does not repro on .NET9.0 or Main (.NET10). So we should probably find the fix and backport it.

[jakobbotsch]

This looks to have been fixed in .NET 9 by #95945.

@czd890 is updating to .NET 9 to pick up that fix an option for you?

[czd890]

@jakobbotsch The company has strict version control and many projects need to be updated. Upgrading to net9 is not a good option at the moment.😢

[czd890]

Hi @EgorBo Would you mind sharing why this bug is triggered?

[EgorBo]

Hi @EgorBo Would you mind sharing why this bug is triggered?

A codegen bug related to exception handling fixed by #95945 as Jakob found out.

@BruceForstall can we backport your change (#95945) to 8.0?

[BruceForstall]

@BruceForstall can we backport your change (#95945) to 8.0?

It's not realistic to backport that change; it's too large. There were also a number of related follow-up changes. This change in itself should not have caused any correctness changes, so I'm surprised that it is pinpointed as a fix to a problem.

[czd890]

Hi @BruceForstall @EgorBo have any updates for this?

[AndyAyersMS]

@czd890 sorry we haven't been more active on addressing this. Have you been able to work around it effectively?

@EgorBo do you still have the repro around somewhere? If so, I can try and look again.

[dotnet-policy-service]

This issue has been marked needs-author-action and may be missing some important information.

[AndyAyersMS]

@EgorBo do you still have the repro around somewhere? If so, I can try and look again.

@EgorBo ping...

[EgorBo]

@EgorBo do you still have the repro around somewhere? If so, I can try and look again.

@EgorBo ping...

Ah, oops, missed it. The repro is the same as is attached to the issue description, I only slightly reduced it and validated that it was fixed in .NET 9.0 and then Jakob (with his collection of coreruns) confirmed that the issue was fixed by #95945

using System.Runtime.CompilerServices;
using System.Security.Cryptography;
using System.Text;

public class Program
{
    [MethodImpl(MethodImplOptions.AggressiveOptimization)]
    public static void Main()
    {
        var hex = "3AE46205A50ED00E7612A91692552B7A";
        var key = "abcdef1234567890";
        var iv = "abcdef1234567890";
        var bytes = Convert.FromHexString(hex);
        for (int i = 0; i < 200; i++)
        {
            var result = new Program().Decrypt(bytes, key, iv, out _);
            if (result == 1)
                throw new Exception("Bug!");
            Console.WriteLine(result);
            Thread.Sleep(10);
        }
    }

    public int Decrypt(byte[] buffer, string key, string iv, out byte[] decryptedData)
    {
        int decryptedByteCount = 0;
        decryptedData = new byte[buffer.Length];

        using var aes = Aes.Create();
        aes.Mode = CipherMode.CBC;
        aes.KeySize = 128;
        aes.Padding = PaddingMode.Zeros;
z
        var instPwdArray = Encoding.ASCII.GetBytes(key);
        var instSaltArray = Encoding.ASCII.GetBytes(iv);

        using (var decryptor = aes.CreateDecryptor(instPwdArray, instSaltArray))
        {
            using (var memoryStream = new MemoryStream(buffer))
            {
                using (var cryptoStream = new CryptoStream(memoryStream, decryptor, CryptoStreamMode.Read))
                {
                    int read;
                    do
                    {
                        read = cryptoStream.Read(
                            decryptedData,
                            decryptedByteCount,
                            decryptedData.Length - decryptedByteCount);
                        decryptedByteCount += read;
                    } while (read != 0);
                }
            }
        }
        // Found the accurate length of decrypted data
        while (decryptedData[decryptedByteCount - 1] == 0 && decryptedByteCount > 0)
            decryptedByteCount--;
        return decryptedByteCount;
    }
}

(Run on Windows-x64, .NET 8.0, default settings, so TC=1 - it will throw an exception).

[jakobbotsch]

I just checked again -- on d2cbd1c this results in

Assert failure(PID 33896 [0x00008468], Thread: 35560 [0x8ae8]): Assertion failed 'this->Next()->isEmpty()' in 'Program:Decrypt(ubyte[],System.String,System.String,byref):int:this' during 'Hoist loop code' (IL size 200; hash 0x38b0beed; Tier1)

    File: C:\dev\dotnet\runtime4\src\coreclr\jit\block.cpp Line: 1590
    Image: D:\dev\core_roots\d2cbd1c54eb59780d369496a8817c9d821c555b3\corerun.exe

after tiering. On 8725a6e, it produces "9" forever.

[AndyAyersMS]

Ok, thanks.

Looks like the bug is that we think a callfinally pair tail is a viable loop preheader, and we hoist some code there, which is a no-no. Seems like there might be a surgical fix.

[AndyAyersMS]

I think the fix is simply:

--- a/src/coreclr/jit/optimizer.cpp
+++ b/src/coreclr/jit/optimizer.cpp
@@ -1801,7 +1801,7 @@ private:
     //
     BasicBlock* FindEntry(BasicBlock* head, BasicBlock* top, BasicBlock* bottom)
     {
-        if (head->bbJumpKind == BBJ_ALWAYS)
+        if ((head->bbJumpKind == BBJ_ALWAYS) && !head->isBBCallAlwaysPairTail())
         {
             if (head->bbJumpDest->bbNum <= bottom->bbNum && head->bbJumpDest->bbNum >= top->bbNum)
             {

In .NET 9 all this was redone; we no longer find loops this way and we no longer have BBJ_ALWAYS pair tails.

[AndyAyersMS]

This should be fixed in 8.0.16, coming soon....

[czd890]

Hi @AndyAyersMS, Very happy to hear this good news, I will close this issue.