String.Concat optimization causes exception with MarshalByRefObject field access

[akoeplinger]

Version Used: 3.3.0-beta3-19406-05 (a190599)

Steps to Reproduce:

1. Compile this code in a .NET Framework console application:

using System;
using System.Runtime.Remoting;
using System.Runtime.Remoting.Messaging;
using System.Runtime.Remoting.Proxies;

class MyProxy : RealProxy
{
    readonly MarshalByRefObject target;

    public MyProxy(MarshalByRefObject target) : base(target.GetType())
    {
        this.target = target;
    }

    public override IMessage Invoke(IMessage request)
    {
        IMethodCallMessage call = (IMethodCallMessage)request;

        IMethodReturnMessage res = RemotingServices.ExecuteMessage(target, call);

        return res;
    }
}

class R1 : MarshalByRefObject
{
    public int test_field = 5;
}

class Test
{

    static void Main()
    {
        R1 myobj = new R1();

        MyProxy real_proxy = new MyProxy(myobj);

        R1 o = (R1)real_proxy.GetTransparentProxy();

        o.test_field = 2;

        Console.WriteLine("test_field: " + o.test_field);
    }
}

2. Run the application
3. It crashes with this exception:

Unhandled Exception: System.InvalidOperationException: An attempt was made to calculate the address of a value type field on a remote object. This was likely caused by an attempt to directly get or set the value of a field within this embedded value type. Avoid this and instead provide and use access methods for each field in the object that will be accessed remotely.
   at Test.Main() in C:\Users\Alexander\Source\Repos\ConsoleApp1\Program.cs:line 43

We hit this while upgrading the Roslyn we use in Mono from 3.1 to 3.3 and one of our unit tests broke (the repro code is a stripped down version of the test).

This is almost certainly because of the String.Concat optimization from #35006 and might be considered by design, but we still wanted to at least start a conversation.

Especially since this code doesn't trigger a CS1690 warning that would show up if you just did string s = o.test_field.ToString();.

/cc @jaredpar @marek-safar

[marek-safar]

@jaredpar @agocke this looks to me like a regression introduced by the Concat optimization.

[agocke]

I agree, this looks like it was caused by the optimization, and I think we should make a slight adjustment to the optimization to access MarshalByRefObjects appropriately to call through the transparent proxy.

[agocke]

@jcouv Could you take a look at this? I think we should consider it for 3.0 servicing

[agocke]

I think the fix should be pretty simple -- checking to see if the receiver at

roslyn/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs

Line 430 in 2ac72ce

if (structToStringMethod != null && expr.Type.SpecialType != SpecialType.None)

is a MarshalByRefObject is good enough to restore compat.

[jaredpar]

Are we sure this is the only bug of this nature? When implementing the readonly member support we allowed for avoiding copies when invoking ToString explicitly in developer code if the type of the expression had not overriden ToString (and hence was using object.ToString).

Test verfiying the behavior: https://github.com/dotnet/roslyn/blob/master/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs#L2137

Don't we need to add the same check there to truly close out this bug?

@RikkiGibson

[RikkiGibson]

We actually emitted object.ToString calls on readonly refs to structs without a defensive copy even before the readonly members feature. See the following example using the Roslyn 2.9.0 compiler: https://sharplab.io/#v2:C4LglgNgNAJiDUAfAAgJgIwFgBQyDMABAM7ABOArgMbAEDKBA3jgSwWAHY1gDczr+BZOgBsggCwEAsgAoOdYgEpGfVqwD6BALzEAdABUA9rTIcA5tIW9sqgL44bQA===

[jcouv]

Okay. I'm able to repro the issue.

The old code would just load the field value:

        // Console.WriteLine("test_field: " + r.test_field);
	IL_0021: ldstr "test_field: "
	IL_0026: ldloc.2
	IL_0027: ldfld int32 R1::test_field
	IL_002c: box [mscorlib]System.Int32
	IL_0031: call string [mscorlib]System.String::Concat(object, object)
	IL_0036: call void [mscorlib]System.Console::WriteLine(string)

But the new code (after Concat optimization) tries to load the field address:

	// Console.WriteLine("test_field: " + r.test_field.ToString());
	IL_0021: ldstr "test_field: "
	IL_0026: ldloc.2
	IL_0027: ldflda int32 R1::test_field
	IL_002c: call instance string [mscorlib]System.Int32::ToString()
	IL_0031: call string [mscorlib]System.String::Concat(string, string)
	IL_0036: call void [mscorlib]System.Console::WriteLine(string)

[jcouv]

Fixed in 16.4 preview 2.