Generated IL for &local in an async method in debug contains a GC hole

[DaZombieKiller]

Steps to Reproduce:

1. Compile the following application under debug:

using System;
using System.Threading.Tasks;

await LogValueAsync();

static async Task LogValueAsync()
{
    int x = 50;
    
    unsafe
    {
        int* p = &x;
        Console.Write('\r');
        Console.WriteLine(*p);
    }
    
    await Task.CompletedTask;
}

2. Run the application with DOTNET_GCStress=2
3. Observe the garbage value output by the program.

Expected Behavior:
The program logs 50 to the console.

Actual Behavior:
The program logs a garbage value to the console.
The x local is emitted as a field in a class under debug, and Roslyn emits IL that takes its address.

Workarounds:
The GC hole can be avoided by using Unsafe.AsRef to allow the use of fixed:

unsafe
{
    fixed (int* p = &Unsafe.AsRef(in x))
    {
        Console.Write('\r');
        Console.WriteLine(*p);
    }
}

[CyrusNajmabadi]

is there any expectation that an unfixed pointer ever be safe to read? I don't get hwo the code above would be safe even if there was no async. The location the pointer points to has nothing ensuring it stays alive (as far as the c# language is concerned).

--

Specifically, we say:

For each address computed by a fixed_pointer_initializer the fixed statement ensures that the variable referenced by the address is not subject to relocation or disposal by the garbage collector for the duration of the fixed statement.

As such, any address not computed by a fixed-pointer-initializer does not have such a guarantee, and it could be subject to relocation.

[jkotas]

Locals are implicitly fixed. That's why C# allows you to take address of them without fixed keyword.

[jkotas]

The spec talks about it in https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/unsafe-code#224-fixed-and-moveable-variables

[CyrusNajmabadi]

Gotcha. Thanks! If the async-section doesn't state anything about these locals not really being locals, then i def agree with this assessment. I'm guessing that if it did though, then we'd also have an error about allowing & there in the first place then. So we have a bug one way or another. Either in not disallowing the &, or in allowing it but not having correct fixed-semantics.

[tmat]

If the variable is lifted to a closure the compiler reports an error (example below [*]). Seems like the same should happen for variables lifted to state machine. In DEBUG we lift all local variables to state machine, in RELEASE we might not if the variable life span doesn't cross await expression/yield statement. I guess we have two options:

1. disallow & for locals in async/iterators entirely. A breaking change and also preventing some useful scenarios (Improve experience when using stack-only types in interactive code #40213) which currently do not work for stack-only types but it would be useful if they did:

static async Task LogValueAsync()
{
    Span<int> s = stackalloc int[1];
    await Task.CompletedTask;
}

error CS4012: Parameters or locals of type 'Span<int>' cannot be declared in async methods or async lambda expressions.

2. do not lift those variables whose pointer is taken (and those whose type is stack only). I think the latter is better but we would need to update EnC to recognize this also and report rude edit.

[*]

static void LogValue()
{
    int x = 50;

    var f = new Func<int>(() => x);

    unsafe
    {
        int* p = &x;
        Console.Write('\r');
        Console.WriteLine(*p);
    }
}

error CS1686: Local 'x' or its members cannot have their address taken and be used inside an anonymous method or lambda expression

[tmat]

Interesting: we disallow any unsafe code in iterators.

error CS1629: Unsafe code may not appear in iterators

but stack-only types are allowed as long as their life span does not cross yield return:

OK:

static IEnumerable<int> Iter()
{
    Span<int> s = stackalloc int[1];
   Console.Write(s[0]);

    yield return 1;
    yield return 2;
}

not ok:

static IEnumerable<int> Iter()
{
    Span<int> s = stackalloc int[1];

    yield return 1;

    Console.Write(s[0]);

    yield return 2;
}

error CS4013: Instance of type 'Span<int>' cannot be used inside a nested function, query expression, iterator block or async method

[DaZombieKiller]

but stack-only types are allowed as long as their life span does not cross yield return:

Notably, you can still take the address of a local in an async method between awaits. This makes it trivial to produce a sample that still contains a GC hole even in release:

using System;
using System.Threading.Tasks;
using System.Runtime.CompilerServices;

await LogValueAsync();

static async Task LogValueAsync()
{
    int x = 50;
    await Task.Delay(1);
    
    unsafe
    {
        _ = Unsafe.AsPointer(ref x); // or Unsafe.AsRef(in x);
        int* p = &x;
        Console.Write('\r');
        Console.WriteLine(*p);
    }
    
    await Task.CompletedTask;
}

You don't even need to enable GCStress to get garbage output with this sample. I was mistaken, prior to this edit the generated code was not taking the field's address (though the output was still incorrect, maybe a separate bug?). Taking the local by ref/in prior to taking its address results in generated code with a GC hole, however.

The sample (for release builds) can be simplified further to:

using System;
using System.Threading.Tasks;
using System.Runtime.CompilerServices;

int x = 50;
await Task.Delay(1);
Unsafe.AsRef(in x);

unsafe
{
    int* p = &x;
    Console.Write('\r');
    Console.WriteLine(*p);
}

[tmat]

@DaZombieKiller Right, I think the compiler should implement the current behavior of iterators for async and lambdas as well and report errors in these cases. It should also allow stack-only types to be used for locals whose lifespan does not cross await/yield return and not lift them in debug builds.

[jcouv]

From discussion with @MadsTorgersen, another option is to rewrite the address-of into a fixed. This reduces the break and maintains the illusion that we're dealing with a local (that's not lifted into a field).
Since await is disallowed in an unsafe context, the lifetime of the pinning will not span across suspensions and so should be reasonable.
I'll take a look at that route.

[tmat]

@jcouv Any opinion on #63100 (comment)?

[tmat]

@jcouv Specifically, re

It should also allow stack-only types to be used for locals whose lifespan does not cross await/yield return and not lift them in debug builds.

[jcouv]

@tmat Yes, we discussed an option like that, for locals that don't need lifting and don't span across suspensions. The main drawback is that it is a deeper change to the async rewriter, as opposed to (hopefully) a localized change.

[jaredpar]

FYI: #66829 may be in the same area here when working on a fix.

[RikkiGibson]

We might want a warning when a local or parameter's address is taken without 'fixed' in an async method. Since the hoisting can depend on whether you build in debug or release mode, etc., it's maybe better to just have a consistent rule for it.

[RikkiGibson]

The analogous behavior for lambda captures is to just disallow taking the address.

https://sharplab.io/#v2:EYLgZgpghgLgrgJwgZwLQAUEEsC2UECeAwgPYB2yMCcAxjCQsgDQAmIA1AD4ACATAIwBYAFDcAzAAI+EohIDeIiUqmS4FKJCkAWCQFkAFFjIwJADwCU8xcpvd+vCVDpZyEgLwT9ltwD55EgDd8CQJ3CQAyUwBuCQBfKOslWJFYoA

Maybe it should be a warning or error (breaking change) to take the address of any local or parameter in an async method, since it may be hoisted. Not sure.

[RikkiGibson]

In #66915 my solution is tentatively to issue a warning in .NET 8 when & is used anywhere in an async method.
warning CS9105: The '&' operator should not be used in async methods..

The idea here is to just give you a nudge that: hey, things may go wrong here, maybe delegate this work to another method and call it instead. This is an alternative to us coming up with a more complex solution in the compiler, such as implicitly fixing variables that compile into class fields, or permitting+requiring fixed-statements with them, etc.

[RikkiGibson]

#47423 is tangentially related here.

[RikkiGibson]

Closed by #66915