DispatchProxy.Create reads AssemblyLoadContext.Name but treats it as an assembly name #80387

Activity

1. ghost added

   untriagedNew issue has not been triaged by the area ownerNew issue has not been triaged by the area owner

   on Jan 4, 2023
3. KalleOlaviNiemitalo commented on Jan 4, 2023

   

   KalleOlaviNiemitalo

   on Jan 4, 2023

   More actions

   I think it's a .NET Runtime bug. DispatchProxyGenerator should not attempt to parse AssemblyLoadContext.Name as AssemblyName. Rather it should copy the string as is to AssemblyName.Name. AssemblyName.FullName is able to quote whatever you put in AssemblyName.Name.
4. KalleOlaviNiemitalo commented on Jan 4, 2023

   

   KalleOlaviNiemitalo

   on Jan 4, 2023

   More actions

   Can you work around by setting MSBUILDSINGLELOADCONTEXT=1 in the environment?
5. KalleOlaviNiemitalo commented on Jan 4, 2023

   

   KalleOlaviNiemitalo

   on Jan 4, 2023

   Last edited by KalleOlaviNiemitalo

   More actions

   MSBuild generates an AssemblyLoadContext.Name that likely includes backslashes that AssemblyNameParser treats as escape characters:

   https://github.com/dotnet/msbuild/blob/2badb37d109910fbd3155cf8743224b7a27494d8/src/Shared/MSBuildLoadContext.cs#L35-L36
   

   runtime/src/libraries/System.Private.CoreLib/src/System/Reflection/AssemblyNameParser.cs

   Lines 384 to 410 in 97203d3

   	if (c == '\\')
   	{
   	c = GetNextChar();
   	switch (c)
   	{
   	case '\\':
   	case ',':
   	case '=':
   	case '\'':
   	case '"':
   	sb.Append(c);
   	break;
   	case 't':
   	sb.Append('\t');
   	break;
   	case 'r':
   	sb.Append('\r');
   	break;
   	case 'n':
   	sb.Append('\n');
   	break;
   	default:
   	ThrowInvalidAssemblyName();
   	break; //unreachable
   	}
   	}
6. KalleOlaviNiemitalo commented on Jan 4, 2023

   

   KalleOlaviNiemitalo

   on Jan 4, 2023

   More actions

   This use of AssemblyLoadContext.Name in DispatchProxy came from #62095, which has been locked, so I cannot comment there. It has not been fixed on the main branch of dotnet/runtime yet, and I did not find any dotnet/runtime issue filed for this problem.
7. philiphendry commented on Jan 4, 2023

   

   philiphendry

   on Jan 4, 2023

   Author

   More actions

   @KalleOlaviNiemitalo great analysis, thank you :)

   I've tried the MSBUILDSINGLELOADCONTEXT environment variable and that does seem to resolve the issue but what would be the consequence of this and is this a better workaround than using a global.json?

   Should I create a new issue in dotnet/runtime and close this one?
8. KalleOlaviNiemitalo commented on Jan 4, 2023

   

   KalleOlaviNiemitalo

   on Jan 4, 2023

   More actions

   I expect the team can move this issue to dotnet/runtime.

   For solutions that target .NET 6.0, I'd prefer global.json rather than the environment variable. One reason is that .NET SDK 7 will go out of support before .NET SDK 6.

   👍React with 👍1Reacted by Philip Hendry
9. KalleOlaviNiemitalo commented on Jan 4, 2023

   

   KalleOlaviNiemitalo

   on Jan 4, 2023

   More actions

   Here's a demo that shows the .NET Runtime regression without involving WCF.

   Sdk29734.csproj:

   <Project Sdk="Microsoft.NET.Sdk">
   
     <PropertyGroup>
       <OutputType>Exe</OutputType>
       <TargetFrameworks>net6.0;net7.0</TargetFrameworks>
       <LangVersion>7.3</LangVersion>
     </PropertyGroup>
   
   </Project>

   Program.cs:

   using System;
   using System.Reflection;
   using System.Runtime.Loader;
   
   namespace Sdk29734
   {
       public static class Program
       {
           static void Main()
           {
               var alc = new AssemblyLoadContext(name: @"Don't parse C:\");
               Assembly assembly = alc.LoadFromAssemblyPath(typeof(Program).Assembly.Location);
               Type type = assembly.GetType(typeof(Program).FullName);
               MethodInfo method = type.GetMethod("Demo", BindingFlags.Public | BindingFlags.Static);
               method.Invoke(null, null);
           }
   
           public static void Demo()
           {
               Console.WriteLine(AssemblyLoadContext.GetLoadContext(typeof(Program).Assembly).Name);
               IDemo proxy = DispatchProxy.Create<IDemo, Proxy>();
               proxy.Method();
           }
       }
   
       internal interface IDemo
       {
           void Method();
       }
   
       internal class Proxy : DispatchProxy
       {
           protected override object Invoke(MethodInfo targetMethod, object[] args)
           {
               Console.WriteLine(targetMethod);
               return null;
           }
       }
   }

   Runs with .NET 6.0.12:

   $ dotnet run --framework=net6.0
   Don't parse C:\
   Void Method()
   

   Fails with .NET 7.0.1:

   $ dotnet run --framework=net7.0
   Don't parse C:\
   Unhandled exception. System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
    ---> System.IO.FileLoadException: The given assembly name was invalid.
   File name: 'DispatchProxyTypes.Don't parse C:\'
      at System.Reflection.AssemblyNameParser.ThrowInvalidAssemblyName()
      at System.Reflection.AssemblyNameParser.GetNextToken(String& tokenString)
      at System.Reflection.AssemblyNameParser.Parse()
      at System.Reflection.AssemblyNameParser.Parse(String name)
      at System.Reflection.AssemblyName..ctor(String assemblyName)
      at System.Reflection.DispatchProxyGenerator.ProxyAssembly..ctor(AssemblyLoadContext alc)
      at System.Reflection.DispatchProxyGenerator.<>c.<CreateProxyInstance>b__6_0(AssemblyLoadContext x)
      at System.Runtime.CompilerServices.ConditionalWeakTable`2.GetValueLocked(TKey key, CreateValueCallback createValueCallback)
      at System.Runtime.CompilerServices.ConditionalWeakTable`2.GetValue(TKey key, CreateValueCallback createValueCallback)
      at System.Reflection.DispatchProxyGenerator.CreateProxyInstance(Type baseType, Type interfaceType)
      at System.Reflection.DispatchProxy.Create[T,TProxy]()
      at Sdk29734.Program.Demo() in [REDACTED]\Program.cs:line 21
      at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
      at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
      --- End of inner exception stack trace ---
      at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
      at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
      at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
      at Sdk29734.Program.Main() in [REDACTED]\Program.cs:line 15
   
10. KalleOlaviNiemitalo commented on Jan 4, 2023

    

    KalleOlaviNiemitalo

    on Jan 4, 2023

    Last edited by KalleOlaviNiemitalo

    More actions

    Rather it should copy the string as is to AssemblyName.Name. AssemblyName.FullName is able to quote whatever you put in AssemblyName.Name.

    Ugh, it isn't that simple. Even though one can set AssemblyName.Name = @"DispatchProxyTypes.MSBuild plugin C:\temp\DotNet7BuildBug\BuildTools\bin\Debug\net6.0\publish\BuildTools.dll", and AssemblyName.FullName nicely doubles the backslashes, AssemblyBuilder.DefineDynamicAssembly then rejects the name.

    System.ArgumentException: The given assembly name was invalid.
       at System.Reflection.Emit.AssemblyBuilder.CreateDynamicAssembly(AssemblyLoadContext assemblyLoadContext, AssemblyName name, AssemblyBuilderAccess access)
       at System.Reflection.Emit.AssemblyBuilder..ctor(AssemblyName name, AssemblyBuilderAccess access, Assembly callingAssembly, AssemblyLoadContext assemblyLoadContext, IEnumerable`1 assemblyAttributes)
       at System.Reflection.Emit.AssemblyBuilder.InternalDefineDynamicAssembly(AssemblyName name, AssemblyBuilderAccess access, Assembly callingAssembly, AssemblyLoadContext assemblyLoadContext, IEnumerable`1 assemblyAttributes)
       at System.Reflection.Emit.AssemblyBuilder.DefineDynamicAssembly(AssemblyName name, AssemblyBuilderAccess access)
    

    I guess it blows up in unmanaged Assembly::CreateDynamic, which rejects backslashes, colons, and slashes:

    runtime/src/coreclr/vm/assembly.cpp

    Lines 400 to 406 in 97203d3

    	if (COMCharacter::nativeIsWhiteSpace(pAssemblyNameParts->_pName[0])
    	|| wcschr(pAssemblyNameParts->_pName, '\\') != NULL
    	|| wcschr(pAssemblyNameParts->_pName, ':') != NULL
    	|| wcschr(pAssemblyNameParts->_pName, '/') != NULL)
    	{
    	COMPlusThrow(kArgumentException, W("InvalidAssemblyName"));
    	}

    So, DispatchProxyGenerator needs to be fixed in some other way. The simplest change would be to make DispatchProxyGenerator always use $"DispatchProxyTypes.{alc.GetHashCode()}", as if AssemblyLoadContext.Name were null.

    runtime/src/libraries/System.Reflection.DispatchProxy/src/System/Reflection/DispatchProxyGenerator.cs

    Lines 120 to 129 in 97203d3

    	string name;
    	if (alc == AssemblyLoadContext.Default)
    	{
    	name = "ProxyBuilder";
    	}
    	else
    	{
    	string? alcName = alc.Name;
    	name = string.IsNullOrEmpty(alcName) ? $"DispatchProxyTypes.{alc.GetHashCode()}" : $"DispatchProxyTypes.{alcName}";
    	}

    For debuggability though, it would be nicer to mangle the name, e.g. replace disallowed characters with underscores.

    👍React with 👍1Reacted by Buyaa Namnan
11. KalleOlaviNiemitalo commented on Jan 5, 2023

    

    KalleOlaviNiemitalo

    on Jan 5, 2023

    Last edited by KalleOlaviNiemitalo

    More actions

    https://github.com/dotnet/corefx/issues/34791, it was suggested:

    • The Assembly.LoadFile(path) generated ALCs would be constructed with name = "Assembly.LoadFile(path)" .

    which is what Assembly.LoadFile does:

    runtime/src/libraries/System.Private.CoreLib/src/System/Reflection/Assembly.cs

    Line 273 in 97203d3

    AssemblyLoadContext alc = new IndividualAssemblyLoadContext($"Assembly.LoadFile({normalizedPath})");

    Therefore, this bug can also be reproduced without explicitly specifying AssemblyLoadContext.Name.

    Sdk29734.csproj:

    <Project Sdk="Microsoft.NET.Sdk">
    
      <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFrameworks>net6.0;net7.0</TargetFrameworks>
        <LangVersion>7.3</LangVersion>
      </PropertyGroup>
    
    </Project>

    Program.cs:

    using System;
    using System.Reflection;
    using System.Runtime.Loader;
    
    namespace Sdk29734
    {
        public static class Program
        {
            static void Main()
            {
                Assembly assembly = Assembly.LoadFile(typeof(Program).Assembly.Location);
                Type type = assembly.GetType(typeof(Program).FullName);
                MethodInfo method = type.GetMethod(nameof(Demo), BindingFlags.Public | BindingFlags.Static);
                method.Invoke(null, null);
            }
    
            public static void Demo()
            {
                Console.WriteLine(AssemblyLoadContext.GetLoadContext(typeof(Program).Assembly).Name);
                IDemo proxy = DispatchProxy.Create<IDemo, Proxy>();
                proxy.Method();
            }
        }
    
        internal interface IDemo
        {
            void Method();
        }
    
        internal class Proxy : DispatchProxy
        {
            protected override object Invoke(MethodInfo targetMethod, object[] args)
            {
                Console.WriteLine(targetMethod);
                return null;
            }
        }
    }

    Runs with .NET 6.0.12:

    $ dotnet run --framework=net6.0
    Assembly.LoadFile(C:\[REDACTED]\Sdk29734\bin\Debug\net6.0\Sdk29734.dll)
    Void Method()
    

    Fails with .NET 7.0.1:

    $ dotnet run --framework=net7.0
    Assembly.LoadFile(C:\[REDACTED]\Sdk29734\bin\Debug\net7.0\Sdk29734.dll)
    Unhandled exception. System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
     ---> System.IO.FileLoadException: The given assembly name was invalid.
    File name: 'DispatchProxyTypes.Assembly.LoadFile(C:\[REDACTED]\Sdk29734\bin\Debug\net7.0\Sdk29734.dll)'
       at System.Reflection.AssemblyNameParser.ThrowInvalidAssemblyName()
       at System.Reflection.AssemblyNameParser.GetNextToken(String& tokenString)
       at System.Reflection.AssemblyNameParser.Parse()
       at System.Reflection.AssemblyNameParser.Parse(String name)
       at System.Reflection.AssemblyName..ctor(String assemblyName)
       at System.Reflection.DispatchProxyGenerator.ProxyAssembly..ctor(AssemblyLoadContext alc)
       at System.Reflection.DispatchProxyGenerator.<>c.<CreateProxyInstance>b__6_0(AssemblyLoadContext x)
       at System.Runtime.CompilerServices.ConditionalWeakTable`2.GetValueLocked(TKey key, CreateValueCallback createValueCallback)
       at System.Runtime.CompilerServices.ConditionalWeakTable`2.GetValue(TKey key, CreateValueCallback createValueCallback)
       at System.Reflection.DispatchProxyGenerator.CreateProxyInstance(Type baseType, Type interfaceType)
       at System.Reflection.DispatchProxy.Create[T,TProxy]()
       at Sdk29734.Program.Demo() in C:\[REDACTED]\Sdk29734\Program.cs:line 20
       at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
       at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
       --- End of inner exception stack trace ---
       at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
       at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
       at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
       at Sdk29734.Program.Main() in C:\[REDACTED]\Sdk29734\Program.cs:line 14
    

    👍React with 👍1Reacted by Buyaa Namnan
12. KalleOlaviNiemitalo commented on Jan 5, 2023

    

    KalleOlaviNiemitalo

    on Jan 5, 2023

    Last edited by KalleOlaviNiemitalo

    More actions

    MSBuild could be changed to work around this issue by not naming the assembly load contexts. Such a change could be helpful in case the AssemblyLoadContext fix is considered too risky for .NET 7.

    The custom MSBuild task cannot work around this issue by assigning null to AssemblyLoadContext.Name, because it is a read-only property.

    The custom MSBuild task could perhaps work around this issue by creating another AssemblyLoadContext without a name, and delegating the virtual methods to the AssemblyLoadContext that MSBuild created. Such a task would not be able to target netstandard2.0, though. Also, it might be difficult to forward the AssemblyLoadContext events to MSBuild's AssemblyLoadContext so that MSBuild can handle them.
13. marcpopMSFT

    transferred this issue fromdotnet/sdkon Jan 9, 2023
14. rainersigwald commented on Jan 9, 2023

    

    rainersigwald

    on Jan 9, 2023

    Member

    More actions

    I agree with @KalleOlaviNiemitalo that this sounds like more of a runtime bug than MSBuild.

    MSBuild could be changed to work around this issue by not naming the assembly load contexts. Such a change could be helpful in case the AssemblyLoadContext fix is considered too risky for .NET 7.

    Agreed, though I would rather not do this as the info we put in the name seems handy. But not that handy, since it's only in the debugger. If we have to I wouldn't object too strongly.
15. rainersigwald

    removed

    untriagedNew issue has not been triaged by the area ownerNew issue has not been triaged by the area owner

    on Jan 9, 2023
17. ghost added

    untriagedNew issue has not been triaged by the area ownerNew issue has not been triaged by the area owner

    on Jan 9, 2023
18. rainersigwald

    transferred this issue fromdotnet/msbuildon Jan 9, 2023
19. rainersigwald

    changed the title [-]MSBuild task calling WCF service fails with exception with dotNet 7.0[/-] [+]DispatchProxy.Create reads AssemblyLoadContext.Name but treats it as an assembly name[/+] on Jan 9, 2023
20. jeffschwMSFT

    added

    area-System.Reflection

    on Jan 10, 2023
22. buyaa-n

    added this to the 8.0.0 milestone on Feb 11, 2023
23. ghost removed

    untriagedNew issue has not been triaged by the area ownerNew issue has not been triaged by the area owner

    on Feb 11, 2023
24. buyaa-n commented on Feb 13, 2023

    

    buyaa-n

    on Feb 13, 2023

    Contributor

    More actions

    Thank you for the broad investigation @KalleOlaviNiemitalo, it seems we don't need to use the ALC name or hash for the builder name, can just use the same name ProxyBuilder for all ALC.
25. KalleOlaviNiemitalo commented on Feb 13, 2023

    

    KalleOlaviNiemitalo

    on Feb 13, 2023

    Last edited by KalleOlaviNiemitalo

    More actions

    Is it feasible to publish the AssemblyLoadContext.Name to debuggers in some other way, so that they can find it if they have a reference to the generated assembly?

    AssemblyBuilder.DefineDynamicAssembly does not take an AssemblyLoadContext parameter, but it uses AssemblyLoadContext.CurrentContextualReflectionContext here:

    runtime/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.cs

    Lines 94 to 106 in 97203d3

    	[RequiresDynamicCode("Defining a dynamic assembly requires dynamic code.")]
    	[DynamicSecurityMethod] // Required to make Assembly.GetCallingAssembly reliable.
    	public static AssemblyBuilder DefineDynamicAssembly(
    	AssemblyName name,
    	AssemblyBuilderAccess access,
    	IEnumerable<CustomAttributeBuilder>? assemblyAttributes)
    	{
    	return InternalDefineDynamicAssembly(name,
    	access,
    	Assembly.GetCallingAssembly(),
    	AssemblyLoadContext.CurrentContextualReflectionContext,
    	assemblyAttributes);
    	}

    So, DispatchPrpxy could use this API to place the generated assembly into the AssemblyLoadContext for which it was generated. However, I don't know what side effects that would have. Likely it's too risky for .NET 7.

    Alternatively, DispatchProxy could generate an AssemblyMetadataAttribute that carries the AssemblyLoadContext.Name. That would have lower risk.

    I don't know whether either of these would make it easy to view the ALC name in debuggers, especially in postmortem WinDbg + SOS.
26. buyaa-n commented on Feb 18, 2023

    

    buyaa-n

    on Feb 18, 2023

    Contributor

    More actions

    So, DispatchPrpxy could use this API to place the generated assembly into the AssemblyLoadContext for which it was generated. However, I don't know what side effects that would have. Likely it's too risky for .NET 7.

    Probably this should be fixed with #62234 not in DispatchProxy

    Alternatively, DispatchProxy could generate an AssemblyMetadataAttribute that carries the AssemblyLoadContext.Name. That would have lower risk.

    I would prefer replacing disallowed characters with underscores as you suggested above instead. I am still not sure how useful of having AssemblyLoadContext.Name as assembly name for debuggers. Though it seems there is no other way to know the custom ALC name, but I think that issue can be solved when #62234 is resolved
27. buyaa-n

    closed this as completedon Mar 10, 2023
28. ghost locked as resolved and limited conversation to collaborators on Apr 10, 2023