dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.07k stars 4.04k forks source link

Low level struct improvements treat [UnscopedRef] differently in source code and in metadata, allowing returning pointers to current method's stack memory #63423

Open TessenR opened 2 years ago

TessenR commented 2 years ago

Version Used:

commit 65dc2845b5287ac972f7299616c13d3270720e44
Author: Chris Sienkiewicz <chsienki@microsoft.com>
Date:   Mon Aug 15 15:07:24 2022 -0700

    Dont throw on null return (#63267)

Steps to Reproduce:

Compile the following code as .dll with preview language version

using System.Diagnostics.CodeAnalysis;

public class UnscopedRefBug
{
  public ref int M([UnscopedRef] out int x) { x = 10; return ref x; }
}

namespace System.Diagnostics.CodeAnalysis
{
    [AttributeUsage(AttributeTargets.All, AllowMultiple = false, Inherited = false)]
    internal sealed class UnscopedRefAttribute : Attribute
    {
      public UnscopedRefAttribute(bool b = false) { }
    }
}

Reference this .dll in another project, compile it with preview language version and run the following code:


using System;

class Program
{
  static void Main()
  {
    ref var r = ref GetStackReference(); // get a reference pointing somewhere further on the stack memory
    Usage(ref r); // break things
    Console.ReadKey();
  }

  static ref int GetStackReference()
  {
    Span<int> s = stackalloc int[100];
    return ref new UnscopedRefBug().M(out s[50]);
  }

  static void Usage(ref int q)
  {
    q = -1;
    Console.WriteLine(q); // prints '-1'

    Span<long> span = stackalloc long[50]; // allocate something on the new stack

    Console.WriteLine(q); // prints '0', stackalloc zero-ed the memory

    long sum = 0;
    foreach (var x in span)
      sum += x;
    Console.WriteLine(sum); // prints '0', all elements of stackalloc are zero

    q = -1; // here we actually write to the memory allocated by 'stackalloc'

    sum = 0;
    foreach (var x in span)
      sum += x;
    Console.WriteLine(sum); // prints '4294967295', one of the element was set to '0xFFFFFFFF' and reinterpreted as long
  }
}

Expected Behavior: Compilation error for return ref x; in UnscopedRefBug.M

Actual Behavior: Both projects compile and the second program runs and corrupts stack memory

Notes: Note that if the argument is provided explicitly i.e. [UnscopedRef(false)], Roslyn will not treat out x as unscoped and correctly produce an error when compiling the .dll project

The problem here is that [UnscopedRef] attribute in the .dll project actually invokes a constructor with a default argument, which is saved in metadata. Consequently, the attribute is not recognized as a well-known attribute when the .dll is referenced in the second project. However, when the first project is being compiled, Roslyn doesn't check for default arguments and treats [UnscopedRef] as a well known constructor usage.

This leads to a situation when at compile time UnscopedRefBug.M.out x is treated as unscoped, allowing returning a reference to this parameter, whereas when the signature is read from metadata the attribute is ignored, so Roslyn treats this parameter as scoped and doesn't use arguments passed to this parameter to verify the ref-safe-to-escape scope of the invocation.

Note that it also makes [UnsocpedRef(false)] and [UnsocpedRef] behave differently in the source code, as the latter is treated as a special attribute by the compiler and the former isn't, even though they result in exactly the same metadata when compiled.

jaredpar commented 2 years ago

Agree this is an issue but more on the minor side. It's a case where the user is incorrectly replicating the source for types the compiler depends on. There are many examples in the language where this will produce errors. For instance the compiler just assumes that many parts of IEnumerable are or are not generic. When you violate that the compiler will often generate invalid IL. Where possible and reasonable we fix these but they tend to be on the lower priority end of the equation.