Dereferencing fixed byte array pointer into decimal crash the CLR with stack overflow error

[ghost]

Decimal type is essentially a struct of 4 int as shown here: https://referencesource.microsoft.com/#mscorlib/system/decimal.cs,145

I've specified byte array to allocate 16 bytes and then call Random.NextBytes on the buffer and then de-reference that byte array as Decimal, this causes the CoreCLR and Microsoft CLR to crash without exception being thrown or any console output being shown as to why this crashed.

using System;
using System.ComponentModel;
using System.Runtime.InteropServices;

namespace RandomSnippet
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine(sizeof(decimal));
            Console.WriteLine(Marshal.SizeOf<decimal>());
            for (int I = 0; I < 1000000; I++)
                TestSnippet();
        }

        static void TestSnippet()
        {
            try
            {
                var random = new Random();
                var decArray = new byte[16];
                decimal decimalVal;
                unsafe
                {
                    fixed (byte* ptr = decArray)
                    {
                        random.NextBytes(decArray);
                        decimalVal = *(decimal*)ptr;
                        Console.WriteLine(decimalVal);
                    }
                }
            }
            catch (Win32Exception ex)
            {
                System.Diagnostics.Debugger.Break();
            }
            catch (Exception ex)
            {
                System.Diagnostics.Debugger.Break();
            }
            catch
            {
                System.Diagnostics.Debugger.Break();
            }
        }
    }
}

When investigating it further with CoreCLR, the error would be shown as followed:

0xc0000409
-1073740791
STATUS_STACK_BUFFER_OVERRUN
The system detected an overrun of a stack-based buffer in this application. This overrun could potentially allow a malicious user to gain control of this application.

On Mono CLR, this runs without a problem. When doing the same approach for double and float with properly sized buffer, it runs fine for all CLR. Decimal is the only one that is having this problem and it is inconsistent, because it would sometime dereference from byte array pointer just fine and when attempted the same thing repeatingly, it would crash the CLR by chance.

My impression is that this is a bug with CLR implementation.

[danmoseley]

What version are you using? does this repro for you with latest master? I tried and it does not repro for me, with /o+ or not. I assume tihs is Windows.

Assuming it reproes on latest master, can you get the CLR stack?

[danmoseley]

I also tried on 4.7.1 with no repro.

[ghost]

This is done on Windows and it seems to happen only on Windows, I am able to reproduce on Windows CoreCLR and Microsoft CLR.
Screenshot: https://imgur.com/Mbuq8kC
Dotnet Core version: 2.1.4
I am trying to get stack trace.

[ghost]

Unhandled exception at 0x00007FF865E95ACC (coreclr.dll) in dotnet.exe: Stack cookie instrumentation code detected a stack-based buffer overrun.

[danmoseley]

I see it now. Writing random values to these fields is not supported. Is there some reason you are doing this?

[ghost]

How exactly is it not supported? That isn't a valid reasoning as to why a struct can't support those, struct should support de-referencing from pointer and decimal is a struct. It can be misrepresented sure, but it should still be supported regardless. As for why I'm doing it, it's a for fun snippet I've created on a whim that is processed by Modix bot on C# Discord Server which causes the bot to crash.

[ghost]

Regardless of the feature around struct, it's a bug with CLR that needed to be addressed otherwise just about anyone can crash the CLR.

[benaadams]

How exactly is it not supported?

Values in the private fields of decimal can't be random and have some rules as to what they can be:

// The lo, mid, hi, and flags fields contain the representation of the
// Decimal value. The lo, mid, and hi fields contain the 96-bit integer
// part of the Decimal. Bits 0-15 (the lower word) of the flags field are
// unused and must be zero; bits 16-23 contain must contain a value between
// 0 and 28, indicating the power of 10 to divide the 96-bit integer part
// by to produce the Decimal value; bits 24-30 are unused and must be zero;
// and finally bit 31 indicates the sign of the Decimal value, 0 meaning
// positive and 1 meaning negative.
//
// NOTE: Do not change the order in which these fields are declared. The
// native methods in this class rely on this particular order.

[ghost]

Having unused flag fields being required to be zero and having set of flags limited to a certain range of values isn't a good presumption to have when dealing with P/Invoke and unsafe programming, as for the CLR, how would this particular bug be addressed in the CLR? It currently enables just about anyone to crash the CLR.

[benaadams]

At a guess its the .ToString() from Console.WriteLine(decimalVal); that's crashing, not the dereferencing and assignment.

[ghost]

Alright, so CoreCLR doesn't do any bound check on it which led to stack overflow. I'm looking into CoreCLR implementation on FormatDecimal internal call.

Going to be a while to try and make sense out of these code:
https://github.com/dotnet/coreclr/blob/efebb38f3c18425c57f94ff910a50e038d13c848/src/mscorlib/shared/System/Number.Formatting.cs#L287

https://github.com/dotnet/coreclr/blob/master/src/classlibnative/bcltype/number.cpp#L1223

[ghost]

And I don't think I want to try and fix this, so I hope at the very least that CoreCLR team would address the crashing issue.

[mikedn]

Having unused flag fields being required to be zero and having set of flags limited to a certain range of values isn't a good presumption to have when dealing with P/Invoke and unsafe programming, as for the CLR, how would this particular bug be addressed in the CLR? It currently enables just about anyone to crash the CLR.

Anyone who uses unsafe code or P/Invoke can crash the CLR in various ways. Using unsafe to bypass the decimal constructor and produce an object with broken invariants is just one creative way of doing it.

That said, stack buffer overruns are usually bad and should be avoided so it's probably worth investigating and fixing this. Whatever the fix may be, it may very well involve throwing InvalidOperationException from the formatting code.

I can't seem to reproduce this problem on the latest CoreCLR build, it may have been fixed when some of the formatting code was moved from native to managed code.

[ghost]

Did you try to reproduce it on Windows?

[mikedn]

On Windows, yes, I can reproduce this. It crashes in the native formatting code.

On the current CoreCLR build it displays very large numbers (such as 0.0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000045447662082747980989522234092).

It's likely that the new managed code works fine because it uses a ValueStringBuilder. This starts with a stack buffer and if it's too small it switches to a heap allocated char[]. This enables the code to format decimal numbers that are basically not valid (they shouldn't have more than 30 digits or so).

[danmoseley]

We don't believe this to be a bug. None of the public constructors or factories can create a malformed Decimal structure that behaves in the way described here. It appears that the only way to get into this situation is to use bit-blitting to overwrite the private fields of the struct and to break its invariants. As mentioned earlier, this is not a supported scenario, and manipulating private fields can result in undefined behavior.

You mentioned this was an experiment for a chat bot. Can you describe your scenario a bit more? We don't believe there is any application which relies on the ability to create random Decimal instances in this fashion. If there's something missing in the runtime that's needed to support your scenario we'd like to hear about it.

[ghost]

Well, the point being that there are programs that allows any user to submit C# snippet and run them to print the output such as chat bot, website that utilize programming language as a game, and so forth. I could wait for the next Dotnet Core considering the note about ValueStringBuilder above and see if that fixed the issue for now.

As for some of us, we explore C# and IL as a passion and we try to write some of the more unorthodox code for CLR to handle on a whim. It's likely never to be seen in production environment and shouldn't be. There are no logical reasoning as to why decimal type shouldn't support the feature above, then again, I wouldn't even use Decimal type in production since I would've been using multi-precision floating point integer such as MPIR or libGMP which is the current standard for this.

[benaadams]

It does require unsafe and you can easily blow up the runtime using unsafe; so you may want to disallow the unsafe keyword and the Unsafe class?

Also you can easily cause a stack overflow by just recursively calling functions

class Program
{
    static void Main(string[] args)
    {
        Main(args);
    }
}

Process is terminating due to StackOverflowException.

[GrabYourPitchforks]

Well, the point being that there are programs that allows any user to submit C# snippet and run them to print the output as a chat bot.

What happens if you submit a C# snippet that includes Environment.FailFast(...); or Process.Start(...);?

[ghost]

It would filter out based on Namespace/References and as for Modix, it is basically using a docker container that basically create and dispose container for each snippet, so when running the code above, it would basically kill the CLR and error wouldn't be printed at all.

At the very least, I was expecting that the basic types for integers and floating point integers in C# to be safe when being used in this manner when dereferencing from a pointer. I am fine with CLR crashing for any other reasons, but for fundamental types such as those?

@benaadams I know that you can crash the CLR with recursive function of itself, the context here is that having the fundamental type crashing the CLR by a simple dereference and printing to console shouldn't be a thing, it should at the very least have an exception being thrown. Otherwise you guy need to consider removing fixed keyword and unsafe keyword and just basically make everything a managed programming, in fact, why even have P/invoke at all?

[GrabYourPitchforks]

You're correct in that bit blitting of fundamental numeric types is "safe". But decimal is not a fundamental primitive numeric type. The primitive numeric types are the integral types, bool, float, and double.

[benaadams]

If you use unsafe nothing is particularly safe; e.g. try

class Program
{
    static void Main(string[] args)
    {
        var array = new int[1];
        var i = 0;
        unsafe
        {
            fixed (int* p = array)
            {
                var ptr = p;
                while (true)
                {
                    i++;
                    Console.Write($"{i}: {*ptr:x}, ");
                    ptr++;
                }
            }
        }
    }
}

[ghost]

@benaadams I know, anyway, the point is randomizing a fixed size buffer of decimal size and de-referencing that buffer as decimal type itself shouldn't crash the CLR. Continuing with this nonsense is not productive and I've already long pass closed the issue.

[janvorli]

This is actually a dup of #5308 where the same issue happens when modifying the fields of a decimal via reflection. The decimal formatting code is not resilient to invalid data. See the #5308 for details. The conclusion was that it would be nice to make sure there is no buffer overflow in this case.