A possible ref safety "hole" around UnscopedRef method invoked on an expression classified as a value or readonly variable of a struct type #67435

AlekseyTs · 2023-03-22T11:26:03Z

Compile and run the following code:

https://sharplab.io/#v2:EYLgtghglgdgNAFxAJwK4wD4GIaoDZ4TB4CmABACZQDORpAsAFBMuMACATGQMJMDeTMkLJsAjADYyAZTIBZUQAoAlGQC8APjIwSAd2nKyfMgDE1ZURbIBfANythIiWVgI5HBchIAzZzFcAPOGkyAE8lQWEBRgcHNgB2Mn87aOEre2ExSRc5AGZlCKEomIyE2XdPH3llADoAJW9lIIpvCHwEJWSHNOYUoUzfV1kAFnzewwKY+LcPbzlFJTqGpSDtPSkDI1NVcw4c6w6J7on+7NkAVlGHIuKyADcIZESzVf0VTbNRDiHrTpupspmlXmiy8jUSBzG3QcxycbG+smgMEukQmsVE1TEAE4FLI8koITdHBjRNjhsoCX90VicRd8b8hN0jj02HtqAg0ABjVxSfjHPbZYzJCYAbTE1QAIlAIABzGAAezZUA51Gq3DlzQAgjAIHgQtQaNUAKowagcuUABxIFHqXgAunyyBUBmQbcjCqiSo7ZoLDkxukA=

class C
{
    static S M1() => new S() { F = 111 };

    static int M2(ref int x, S y)
    {
        return x;
    }

    static int M3()
    {
        return M2(ref M1().Ref(), default);
    }

    static int M4()
    {
        return M2(ref M1().Ref(), new S() { F = 123 });
    }

    static int M5()
    {
        var x = new S() { F = 124 };
        return M2(ref M1().Ref(), x);
    }
    
    static void Main()
    {
        System.Console.WriteLine(M3());
        System.Console.WriteLine(M4());
        System.Console.WriteLine(M5());
    }
}


public struct S
{
    public int F;

    [System.Diagnostics.CodeAnalysis.UnscopedRef]
    public ref int Ref()
    {
        return ref F;
    }
}

Observed:

0
123
111

Expected either an error for Ref invocations, or the following output:

111
111
111

The problem is the fact, that Ref is invoked on a value (vs. a variable) and its result refers to a memory in a short lived temp synthesized by compiler. The lifetime of that temp is the Ref invocation. Compiler is free to reuse it afterwards. Effectively, we end up with a reference to a memory that is no longer alive. This can be observed in IL for M3:

    .method private hidebysig static 
        int32 M3 () cil managed 
    {
        // Method begins at RVA 0x208c
        // Code size 28 (0x1c)
        .maxstack 2
        .locals init (
            [0] valuetype S
        )

        IL_0000: call valuetype S C::M1()
        IL_0005: stloc.0
        IL_0006: ldloca.s 0
        IL_0008: call instance int32& S::Ref()
        IL_000d: ldloca.s 0
        IL_000f: initobj S
        IL_0015: ldloc.0
        IL_0016: call int32 C::M2(int32&, valuetype S)
        IL_001b: ret
    } // end of method C::M3

Note that local 0 is used with different values in the process of calculating both arguments for M2.

Perhaps compiler should disallow invoking UnscopedRef methods on expressions classified as values of struct types. It should be fine to invoke such methods on expressions classified as variables of struct types.

The text was updated successfully, but these errors were encountered:

AlekseyTs · 2023-03-22T11:27:48Z

CC @cston, @jaredpar

AlekseyTs · 2023-03-22T21:23:02Z

Here is a repro for a readonly context:
https://sharplab.io/#v2:EYLgtghglgdgNAFxFANgHwMQwK4pRYFAUwAIATKAZwOIFgAoBgAQCYSBhBgbwZL5KYBGAGwkAyiQCyggBQBKEgF4AfCRhEA7uPkkuJAGJKSgkyQC+Abga9+Q0UwAsU6DHk2+Pev28DBAOiEAThlpeT9JAGZ5OSsvfjMGBMZ6JgiSSgQAJ2wAYwRxbncBNNh8/ViigG0hPwARKAgAcxgAewyoHMo/dhayIgBBGAgUAE9KKj8AVRhKHJaAByIyACUiADMAXSLUkkz1klKSVbW3OI8i7yYAdl398qKk7220vYgyFphRg5h8yNPvTw+Ww3SQsGR7NZHdbyODkdYQXAIGIPaxnXyiQ6g8H7Q4AD1hEhGciKgKBAhuuNi3iSSSAA==

class C
{
    static S M1() => new S() { F = 111 };

    static void Main()
    {
        System.Console.WriteLine(M1().M3());
    }
}

public struct S
{
    public int F;

    [System.Diagnostics.CodeAnalysis.UnscopedRef]
    public ref int Ref()
    {
        return ref F;
    }
    
    public readonly int M3()
    {
        return M2(ref Ref(), default);
    }

    static int M2(ref int x, S y)
    {
        return x;
    }
}

IL:

    .method public hidebysig 
        instance int32 M3 () cil managed 
    {
        .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsReadOnlyAttribute::.ctor() = (
            01 00 00 00
        )
        // Method begins at RVA 0x20b8
        // Code size 29 (0x1d)
        .maxstack 2
        .locals init (
            [0] valuetype S
        )

        IL_0000: ldarg.0
        IL_0001: ldobj S
        IL_0006: stloc.0
        IL_0007: ldloca.s 0
        IL_0009: call instance int32& S::Ref()
        IL_000e: ldloca.s 0
        IL_0010: initobj S
        IL_0016: ldloc.0
        IL_0017: call int32 S::M2(int32&, valuetype S)
        IL_001c: ret
    } // end of method S::M3

However, the behavior is fine when Ref method is itself readonly. I think this scenario should keep working:
https://sharplab.io/#v2:EYLgtghglgdgNAFxFANgHwMQwK4pRYFAUwAIATKAZwOIFgAoBgAQCYSBhBgbwZL5KYBGAGwkAyiQCyggBQBKEgF4AfCRhEA7uPkkuJAGJKSgkyQC+Abga9+Q0UwAsU6DHk2+Pev28DBAOiEAThlpeT9JAGZ5OSsvfjMGBMZ6JgiSSgQAJ2wAYwRxbncBNNh8/ViigG0hPwARKAgAcxgAewyoHMo/dhayIgBBGAgUAE9KKj8AVRhKHJaAByIyACUiADMAXSLUkkyiCDIWmFHd9dODo5PSklW1tziPIu8mAHZTtYNY7yTvbbS9i7HEYka6Re7eTw+WxvSQsGSwG7reRwcjrCC4BAxIpJbYiEEwfKw+EwfH5AAeKIkIzkRUhUIEbzJX3iiQYQA=

class C
{
    static S M1() => new S() { F = 111 };

    static void Main()
    {
        System.Console.WriteLine(M1().M3());
    }
}

public struct S
{
    public int F;

    [System.Diagnostics.CodeAnalysis.UnscopedRef]
    public readonly ref readonly int Ref()
    {
        return ref F;
    }
    
    public readonly int M3()
    {
        return M2(in Ref(), default);
    }

    static int M2(in int x, S y)
    {
        return x;
    }
}

AlekseyTs · 2023-03-23T14:26:37Z

Here is a scenario with the same readonly method that should be blocked:

class C
{
    static S M1() => new S() { F = 111 };

    static int M2(in int x, S y)
    {
        return x;
    }

    static int M3()
    {
        return M2(in M1().Ref(), default);
    }
   
    static void Main()
    {
        System.Console.WriteLine(M3());
    }
}

public struct S
{
    public int F;

    [System.Diagnostics.CodeAnalysis.UnscopedRef]
    readonly public ref readonly int Ref()
    {
        return ref F;
    }
}

It looks like the reason why receiver is a value matters.

AlekseyTs · 2023-04-04T14:11:07Z

Note that invoking non-readonly APIs on readonly variables causes receiver to be cloned. I think that should result in an error for UnscopedRef APIs.

RikkiGibson · 2023-07-11T20:08:53Z

@jjonescz I see you have a PR out for this, were you still interested in working on it?

jjonescz · 2023-07-12T07:53:40Z

@RikkiGibson not in near future, feel free to pick it up

RikkiGibson · 2023-08-02T21:08:22Z

@jaredpar pointed out that in parameters are impacted by a similar scenario

ref readonly int M(in int x) => ref x;
ref int ri1 = M(42);
ref int ri2 = M(43); // can't reuse temp created for '42'.
Console.Write(ri1);

I'm trying to determine if we should preserve compat and try to avoid reuse of temps here, or if we can/should just make the call an error. Maybe we should have a wider discussion about it.

jjonescz · 2023-08-03T06:05:07Z

FWIW, we had a small discussion and iirc Aleksey suggested to make this an error first and only if that's a problem, try to fix how the temps are reused: #67955 (comment)

RikkiGibson · 2023-09-12T20:38:38Z

@AlekseyTs @jaredpar and I discussed this issue today. Conclusions:

We want implicit temps created by the compiler to have "current block" scope. We want to prioritize making the change to give correct errors related to this. e.g. giving an error if a reference to implicit temp is escaping out of the current block.
We have flexibility with code gen. If we aren't able to reuse temps in some cases, where calls may be returning refs to temps, we can live with this, although we'd like to eventually reuse temps as much as possible, where it is valid.
Although we're aware of bugs related to temp reuse for in arguments, In the short term we want to reuse the machinery for temp reuse for in arguments, also for receiver arguments (i.e. the original scenario in this issue).
Depending on implementation complexity, we may kick out certain aspects of this fix to the next release, and/or delay fixing until we're able to refactor areas like RefSafetyAnalysis in order to better express/represent them.

jjonescz · 2024-05-21T07:35:37Z

For visibility, here's a simple repro from #73438:

using System;

ReadOnlySpan<bool> a = new(true);
ReadOnlySpan<bool> b = new(false);

Console.WriteLine(a[0]); // should print True; prints False
Console.WriteLine(b[0]); // prints False; that's ok

AlekseyTs added the Area-Compilers label Mar 22, 2023

jaredpar assigned RikkiGibson Mar 22, 2023

jaredpar added the Bug label Mar 22, 2023

jaredpar added this to the 17.6 milestone Mar 22, 2023

AlekseyTs mentioned this issue Apr 18, 2023

Improve detection of receiver cloning in ref safety analysis #67736

Merged

jjonescz mentioned this issue Apr 25, 2023

Avoid reusing ref receiver temp #67955

Closed

jcouv modified the milestones: 17.6, 17.7 Jun 12, 2023

RikkiGibson modified the milestones: 17.7, 17.8 Jul 12, 2023

jaredpar modified the milestones: 17.8, 17.9 Sep 26, 2023

jaredpar modified the milestones: 17.9, 17.10 Nov 28, 2023

jjonescz mentioned this issue Jan 24, 2024

ref safety broken with cast operators #71773

Closed

jaredpar modified the milestones: 17.10, 17.11 Mar 26, 2024

jjonescz mentioned this issue May 13, 2024

Possibly unintended reuse of ref local to in/ref readonly parameters causes unexpected mutation #73438

Closed

jaredpar removed this from the 17.11 milestone Jul 9, 2024

jaredpar added this to the 17.12 milestone Jul 9, 2024

jaredpar modified the milestones: 17.12, 17.13 Aug 27, 2024

jaredpar assigned jjonescz Oct 22, 2024

RikkiGibson removed their assignment Oct 31, 2024

jjonescz linked a pull request Nov 8, 2024 that will close this issue

Disallow escaping refs to implicit rvalue temporaries #75830

Draft

3 tasks

jaredpar added the New Language Feature - Ref Fields label Nov 11, 2024

arunchndr modified the milestones: 17.13, 17.13 P2 Nov 13, 2024

jjonescz linked a pull request Nov 21, 2024 that will close this issue

Avoid reusing temps whose refs might be captured #76009

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A possible ref safety "hole" around UnscopedRef method invoked on an expression classified as a value or readonly variable of a struct type #67435

A possible ref safety "hole" around UnscopedRef method invoked on an expression classified as a value or readonly variable of a struct type #67435

AlekseyTs commented Mar 22, 2023

AlekseyTs commented Mar 22, 2023

AlekseyTs commented Mar 22, 2023

AlekseyTs commented Mar 23, 2023

AlekseyTs commented Apr 4, 2023

RikkiGibson commented Jul 11, 2023

jjonescz commented Jul 12, 2023

RikkiGibson commented Aug 2, 2023

jjonescz commented Aug 3, 2023

RikkiGibson commented Sep 12, 2023

jjonescz commented May 21, 2024

A possible ref safety "hole" around UnscopedRef method invoked on an expression classified as a value or readonly variable of a struct type #67435

A possible ref safety "hole" around UnscopedRef method invoked on an expression classified as a value or readonly variable of a struct type #67435

Comments

AlekseyTs commented Mar 22, 2023

AlekseyTs commented Mar 22, 2023

AlekseyTs commented Mar 22, 2023

AlekseyTs commented Mar 23, 2023

AlekseyTs commented Apr 4, 2023

RikkiGibson commented Jul 11, 2023

jjonescz commented Jul 12, 2023

RikkiGibson commented Aug 2, 2023

jjonescz commented Aug 3, 2023

RikkiGibson commented Sep 12, 2023

jjonescz commented May 21, 2024