Roslyn: Using the new C# 7.2 'in' parameters can lead to memory corruptions

[csrakowski]

Version Used:
Microsoft Visual Studio Enterprise 2017, Version 15.5.0
Microsoft (R) Visual C# Compiler version 2.6.0.62329 (5429b35d)

Steps to Reproduce:

1. Compile reproduction project (will share privately)
2. Run console application
3. Observe crash when it attempts to print the values

Expected Behavior:
It should print the strings it found to the console output

Actual Behavior:
AccessViolationException when referencing the strings

Description
With the release of Visual Studio 15.5 and C# 7.2 'in' support, we sprinkled this around parts of our code base.
When running the unit tests to confirm everything still worked, we found a big portion of them now failed with varying reasons. Some manifested as OutOfMemoryException, others as AccessViolationException and a few NullReferenceExceptions (one even appeared to be thrown inside String.IsNullOrEmpty)
After confirming everything still worked in our other branch, I began digging into this in an attempt to find the root cause, but ended up only getting more and more confused.
I was able to create a small reproduction console app that references a single library project containing our custom message parser, but don't really know how to move forward from here.

I have created a zip file (220KB) of the reproduction project, which includes the source code and the outputted binaries I got, and was hoping someone here could take a look to see if they can find the cause.

I was only able to get permission to share the code in a private manner, so I'm afraid I can't just upload it or link to it here.
I can either upload it to a more private environment, or share a download link via E-mail, Slack/Twitter DM, etc.

[khionu]

Suggestion: eliminate as many factors in the code as possible, till you can reproduce the issue without sharing private code. That would be a process whoever helped you would have to do anyways.

[Joe4evr]

in parameters (real name ref readonly) are really only meant to be used for passing around large structs without copying with the guarantee that they would not be changed. Using it around primitives (or, as you experienced, reference types) can potentially lead to worse performance than just passing them by-val.

There was quite some pushback that in is not an appropriate word to use, but it was too little, too late. 7.2 has shipped, and now we have to live with this mistake for as long as C# exists. 🙄

[jcouv]

Tagging @VSadov @OmarTawfik

[gulbanana]

regardless of what the keyword is called, i think we can agree that it should be memory-safe :)
assuming of course that the repro code doesn’t use unsafe blocks containing marshalling or something.

[OmarTawfik]

Thanks @csrakowski for reporting this!
As suggested, it would be great f you can refactor the private code into a public smaller repro you can share here. It would save us a lot of time and effort to root cause this (we would need to do the same). If however this is impossible, please feel free to DM it to me.

[csrakowski]

Thanks for all the comments.
I will look into stripping out as much code as possible to get a smaller sample.

[csrakowski]

I have managed to cut it down do a single file with 3 methods, and even found the "breaking point". (and a new Exception)
When I remove the in from ParseLine, it runs fine.
With the in, it breaks with the following error in the VS Output Window:

Managed Debugging Assistant 'FatalExecutionEngineError' 
The runtime has encountered a fatal error. The address of the error was at 0x6f9acfb5, on thread 0x2e88. The error code is 0xc0000005.
This error may be a bug in the CLR or in the unsafe or non-verifiable portions of user code.
Common sources of this bug include user marshaling errors for COM-interop or PInvoke, which may corrupt the stack.

An unhandled exception of type 'System.ExecutionEngineException' occurred in Unknown Module.

Program.cs

using System;
using System.Collections.Generic;
using System.Linq;

namespace IssueReproRepo
{
    class Program
    {
        static void Main(string[] args)
        {
            const string testMessage = "Line1|Field1|Field2\rLine2|Field3|Field4\r\r";

            var message = ParseMessage(testMessage);
            var line2 = message.FirstOrDefault(s => s[0] == "Line2");

            Console.WriteLine(line2);
        }

        public static IReadOnlyList<string[]> ParseMessage(in string message)
        {
            var lines = message.Split(new[] { "\r" }, StringSplitOptions.RemoveEmptyEntries);
            return lines.Select(ParseLine).ToList();
        }

        private static string[] ParseLine(in string segmentLine)
        {
            return segmentLine.Split(new[] { '|' }, StringSplitOptions.None);
        }
    }
}

Download project

[gulbanana]

I played around with this for a bit trying to simplify the repro and found something odd: the bug reproduces using the old project system, but not using an Sdk project. Here's a repro with two projects made as similar & minimal as possible:
https://github.com/gulbanana-bugs/repro-csharp72-in

Using Visual Studio 15.5 and dotnet SDK 2.1.2, the "-broken" project produces the described symptoms for me but the "-working" project, with the same code and config, does not.

[gulbanana]

! i found the difference between them via ILDASM: Prefer32Bit. the bug does not seem to occur, in this case, when running 64-bit

[gulbanana]

that opens the way to a really simple repro:

1. clone https://github.com/gulbanana-bugs/repro-csharp72-in-minimal
2. dotnet run
3. observe segfault

Or inline:

static void Main() => new[]{ 0 }.Select(InMethod).ToList();
static int InMethod(in int param) => param;

[ufcpp]

One more code:
(TergetFrameworks net46;netcoreapp2.1)

using System;

class Program
{
    static void Main()
    {
        // buffer overrun
        A(1);

        // throw NullReference or AccessViolation Exception
        B(1, 2);
    }

    static void A(in double x = 1, in string y = "") => Console.WriteLine(y);
    static double B(in float x, in float y, in float z = 1.0f) => x * y * z;

}

[tannergooding]

For the example @ufcpp gave in particular, looking at the decompilation shows that the default values are not being passed by ref and are instead being passed directly: https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQBMQGoA+ABADAAmwEYBuAWAChLsAmQogdkoG9L936A2QgFnoA4AFAEo2HVhQ5T8Aehn5gAVwBmyxPgD2AN0QJFAOzHT8AQUFFh5KpOOz5MABYINAd3wA5RQBsvAJThqCHD6AMZwmgimIWFQUABqAJYaXhAwSfr4AKIAHmEADmkahjbGAELmSPg0lkb4AL6UtcTc2HxmCRkoGorAXuHZ+AC8+ESVHfQEAJ5D+ABEs8JDAHz0AJyCkzUlXPhdPX345ePKXhqp+NljGSdnMPiTV/g35wBeM0QAdLjKi4MrAwAqe74IEvKyUOpAA===

[tannergooding]

Same here, value is being passed directly: https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQBMQGoA+ABADAAmwEYBuAWACg9CiA6AGQEsA7AR3IsuwCYaB2SgG9K+UTQBshACw0AHAAoAlPgC8APnzM4AdwDaAXXyD8BAL60AynAA2cAMYx5ASWYBZODAAWAexSLaACreTLBKHGIS+Cww+C7uXr7yLFHMMQAOEAgQYMrq+BlZYBymlEA=

[tannergooding]

on @gulbanana's example, InMethod shouldn't be accepted as a valid parameter to Select, since it includes ref in the contract

[tannergooding]

@csrakowski's issue is the same as @gulbanana's example. The compiler is letting in be used as the equivalent byval delegate

[jcouv]

Thanks all for narrowing down the problem.

From discussion with @tannergooding, there are two separate bugs:
(1) default values are passed by-val instead of in,
(2) the compiler incorrectly matches a method with an in parameter to a by-val delegate signature.

Assigning to @OmarTawfik since Vlad is out. Thanks

[ufcpp]

Another one:

using System;

static class Ex
{
    public static void InMethod(in X arg) => Console.WriteLine(arg);
}

class X
{
    public void M()
    {
        // pass `this` by in-parameter.
        // `ldind.ref` is not emitted.
        // As a result, ExecutionEngineException is thrown.
        Ex.InMethod(this);
    }
}

class Program
{
    static void Main()
    {
        var x = new X();

        // No problem
        Ex.InMethod(x);

        // throws ExecutionEngineException
        x.M();
    }
}

[tannergooding]

Thanks @ufcpp.

@OmarTawfik, @VSadov, @jcouv: See https://sharplab.io/#v2:EYLgtghgzgLgpgJwD4AEAMACFBGA3AWACgicA2LAJgwFEAPIgbyIxawGYttyUAWDASQB2AWTgwAFgHsAJgAoAloIwANDBAQBzAJQYAvAD5OATlnrtBQgF8iJKssbNWKDrwzDZWxyyaFWfjAD0ARgADtBQGAAGEvJQkRjAAJ4YigC0YQgQYGKIAHRe/kFRADbSitK5CHAAZvGxGIKSMBhwYPIw8BUFfkUAghEQGFVQAK7FMAA0NLRwAMYjMPKSgtSCGopwdLNwIYvLKRESCJIA7oL5vv4sdLlCohIysjFQWhZ+1lY2hChUOADsDkuLDIWD4OAAHB4Cj4riwAG7qDC0PQNOAnFQeCzdVhFAByklCx2AxVa2OutFuIjEUjktFeX1hgWCR1OETocwWSxWaw2Wx2e0EZKRuXc9KBGA+liAA==

this is not passed by reference.

Attempting to explicitly call Ex.Inmethod(in this) results in error CS8354: Cannot return 'this' by reference.

[VSadov]

The delegate mismatch on ‘in’ has been recently fixed.

Passing reference type ‘in’ seems a separate issue.
Most likely because we never passed ‘this’ of a reference type by ref or as a receiver of a struct call (since class is never a struct), the HasHome and EmitAddress for such case are not implemented correctly.

[VSadov]

Thanks for reporting this!!

[VSadov]

“this” is a byval parameter in classes.

HasHome should return “false” for reference type “this”. (most likely this is done correctly to handle scenarios with fields).
EmitAddress should emit a reference to a temp.

[tannergooding]

@VSadov, this refers to an explicitly passed parameter (in the code example), rather than the implicit first parameter of an instance method.

[OmarTawfik]

Ok. To conclude issues here (some organization and filing bugs):

1. The issue that for delegate parameters, the compiler doesn't match in methods with val methods correctly. This is a duplicate of #23319 and was fixed in #23397.
2. The issue of passing optional in parameters by value. I've copied the repro here into a separate issue (#23691) and assigned to myself to investigate further.
3. The issue of passing this to in parameters. I've copied the repro here into another separate issue (#23692) and assigned to myself to investigate further.

Since the original root cause here is already fixed (as noted in the first point), I'll close this, and we should follow up on respective issues.
Thanks all for the reports!