Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DataContractSerialization has Type.GetMethod extension method that is not trim-safe #42754

Closed
eerhardt opened this issue Sep 25, 2020 · 1 comment · Fixed by #42824
Closed
Labels
area-Serialization linkable-framework Issues associated with delivering a linker friendly framework
Milestone

Comments

@eerhardt
Copy link
Member

eerhardt commented Sep 25, 2020

We have some reflection code in DataContractSerialization that is not trim-safe:

s_writeEndElementMethod = typeof(XmlWriterDelegator).GetMethod("WriteEndElement", Globals.ScanAllMembers, Array.Empty<Type>());

s_createSerializationExceptionMethod = typeof(XmlObjectSerializerReadContext).GetMethod("CreateSerializationException", Globals.ScanAllMembers, new Type[] { typeof(string) });

On the surface, this appears to be safe since the ILLinker should be able to see this usage of reflection and preserve the referenced methods. However, the issue is that overload of Type.GetMethod doesn't exist. (See #42753 for proposal to create it.)

Instead, DataContractSerialization has its own extension method that implements this API:

internal static MethodInfo? GetMethod(this Type type, string methodName, BindingFlags bindingFlags, Type[] parameterTypes)
{
var methodInfos = type.GetMethods(bindingFlags);
var methodInfo = FilterMethodBases(methodInfos.Cast<MethodBase>().ToArray()!, parameterTypes, methodName);
return methodInfo != null ? (MethodInfo)methodInfo : null;
}

The ILLinker can't see through this usage of Reflection, so all 41 usages of this method are broken when an application is trimmed.

We should remove this method and call directly into Reflection APIs, so the linker has a chance of preserving these necessary methods.

This is the underlying problem that was discovered in dotnet/aspnetcore#25909. Here's a repro program from that issue that fails when PublishTrimmed=true and TrimMode=link are set.

using System;
using System.IO;
using System.Threading.Tasks;
using System.Net.Http;
using System.Text;
using System.Runtime.Serialization;
using MyApplication.Blazor.Shared.Types;

namespace LinkerEHBug
{
    class Program
    {
        static async Task Main(string[] args)
        {
            string v = @"<AuthState xmlns:i=""http://www.w3.org/2001/XMLSchema-instance"" xmlns=""http://schemas.datacontract.org/2004/07/MyApplication.Blazor.Shared.Types""> <GivenName>Admin User</GivenName> <IsAuthenticated>true</IsAuthenticated> <Name>admin@fmdevsql.onmicrosoft.com</Name> </AuthState>";

            var s = DeserializeDataContract<AuthState>(v);
            Console.WriteLine(s);
        }

        public static T DeserializeDataContract<T>(string serialized)
        {
            using (var inStream = new MemoryStream(new UTF8Encoding().GetBytes(serialized)))
            {
                Console.WriteLine($"{inStream.Length} bytes to deserialize for type {typeof(T).Name}");
                var ser = new DataContractSerializer(typeof(T));
                Console.WriteLine("Calling ReadObject");
                return (T)ser.ReadObject(inStream); // <---- FAILS HERE
            }
        }
    }
}

namespace MyApplication.Blazor.Shared.Types
{
    public class AuthState
    {
        public AuthState()
        {
        }

        public AuthState(bool isAuthenticated, string name, string givenName)
        {
            IsAuthenticated = isAuthenticated;
            Name = name;
            GivenName = givenName;
        }

        public bool IsAuthenticated { get; set; }
        public string Name { get; set; }
        public string GivenName { get; set; }
    }
}
@eerhardt eerhardt added area-Serialization linkable-framework Issues associated with delivering a linker friendly framework labels Sep 25, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Sep 25, 2020
@jkotas
Copy link
Member

jkotas commented Sep 25, 2020

FWIW, .NET Framework called the Type.GetMethod directly https://referencesource.microsoft.com/#System.Runtime.Serialization/System/Runtime/Serialization/XmlFormatGeneratorStatics.cs . This method is left-over from .NET Core 1.0 era reduced reflection surface that made people to introduce stub implementations like this in many places.

However, the issue is that overload of Type.GetMethod doesn't exist

The calls to this method are long and verbose already because of the need to create array of types. The .NET Framework implementation passed in the extra nulls and it looks just fine to me.

eerhardt added a commit to eerhardt/runtime that referenced this issue Sep 28, 2020
DataContractSerialization has some Reflection "shim" methods that the ILLinker can't see through. This causes critical methods to be trimmed and applications to fail. These methods were put in place in .NET Core 1.0 when the full Reflection API wasn't available.

The fix is to remove these "shim" Reflection APIs and use Reflection directly.

Fix dotnet#41525
Fix dotnet#42754
eerhardt added a commit that referenced this issue Sep 30, 2020
DataContractSerialization has some Reflection "shim" methods that the ILLinker can't see through. This causes critical methods to be trimmed and applications to fail. These methods were put in place in .NET Core 1.0 when the full Reflection API wasn't available.

The fix is to remove these "shim" Reflection APIs and use Reflection directly.

Fix #41525
Fix #42754
@stephentoub stephentoub added this to the 6.0.0 milestone Sep 30, 2020
@stephentoub stephentoub removed the untriaged New issue has not been triaged by the area owner label Sep 30, 2020
jkotas pushed a commit that referenced this issue Sep 30, 2020
DataContractSerialization has some Reflection "shim" methods that the ILLinker can't see through. This causes critical methods to be trimmed and applications to fail. These methods were put in place in .NET Core 1.0 when the full Reflection API wasn't available.

The fix is to remove these "shim" Reflection APIs and use Reflection directly.

Fix #41525
Fix #42754
danmoseley pushed a commit that referenced this issue Oct 1, 2020
…link (#42911)

* DataContractSerialization doesn't work with TrimMode - link

DataContractSerialization has some Reflection "shim" methods that the ILLinker can't see through. This causes critical methods to be trimmed and applications to fail. These methods were put in place in .NET Core 1.0 when the full Reflection API wasn't available.

The fix is to remove these "shim" Reflection APIs and use Reflection directly.

Fix #41525
Fix #42754

* add copyright header

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Serialization linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants