Fix resx test name mangling logic

[GrabYourPitchforks]

There's an edge case in the logic for System.Resources.Extensions.Tests that isn't being handled properly. This is blocking my progress on #36252.

Here's the existing logic:

runtime/src/libraries/System.Resources.Extensions/tests/TestData.cs

Lines 303 to 317 in 87d8f7d

	// workaround for https://github.com/dotnet/runtime/issues/31289
	assemblyQualifiedTypeName = assemblyQualifiedTypeName.Replace(s_coreAssemblyName, s_mscorlibAssemblyName);
	int pos = assemblyQualifiedTypeName.IndexOf(',');
	if (pos > 0 && pos < assemblyQualifiedTypeName.Length - 1)
	{
	assemblyName = assemblyQualifiedTypeName.Substring(pos + 1).TrimStart();
	string newTypeName = assemblyQualifiedTypeName.Substring(0, pos);
	if (!string.Equals(newTypeName, serializedType.FullName, StringComparison.InvariantCulture))
	{
	typeName = newTypeName;
	}
	return;
	}
	base.BindToName(serializedType, out assemblyName, out typeName);

I have a case where assemblyQualifiedTypeName is "System.Collections.Generic.GenericEqualityComparer`1[[System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089]]". However, the IndexOf above is improperly detecting the ',' as part of the generic term, which results in the corrupted output:

assemblyName = "mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089]]" (note the ]])
newTypeName = "System.Collections.Generic.GenericEqualityComparer`1[[System.String" (note the [[)

This PR updates the type name mangling logic to ignore any ',' characters that are within a generic parameters list. This affects only test code, not runtime code.

[ghost]

Tagging subscribers to this area: @tarekgh, @buyaa-n, @krwq
Notify danmosemsft if you want to be subscribed.

[tarekgh]

I recall @ericstj was aware about such case but I don't remember why we didn't bother handling this case. Anyway, this change LGTM.

[GrabYourPitchforks]

It's a little hard to see in the binary diff, but here are some screen shots showing before and after. You can see the "before" has a corrupted type name as part of the serialized stream, which I've highlighted inside a blue box. The "after" has the type name fixed.

[GrabYourPitchforks]

/azp run runtime

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[danmoseley]

@dotnet/dnceng

2020-07-18T08:14:55.7171015Z ##[command]/usr/bin/docker pull mcr.microsoft.com/dotnet-buildtools/prereqs:ubuntu-16.04-cross-arm64-alpine-20200413125008-406629a
2020-07-18T08:14:56.2216527Z Error response from daemon: Get https://mcr.microsoft.com/v2/: read tcp 10.0.0.71:36034->40.71.10.214:443: read: connection reset by peer
2020-07-18T08:14:56.2385227Z ##[error]Docker pull failed with exit code 1
2020-07-18T08:14:56.2393829Z ##[section]Finishing: Initialize containers

[ericstj]

This code was intentionally mangling the type name with generic parameters in order to test a problem that existed in the old resource writer (and thus in resources that needed to load on core).

This is described in the commit 01aeaa5, as well as in the name of the type you modified: TypeNameManglingSerializationBinder.

I believe this commit should be undone. How was this blocking you?

[MattGal]

@dotnet/dnceng

2020-07-18T08:14:55.7171015Z ##[command]/usr/bin/docker pull mcr.microsoft.com/dotnet-buildtools/prereqs:ubuntu-16.04-cross-arm64-alpine-20200413125008-406629a
2020-07-18T08:14:56.2216527Z Error response from daemon: Get https://mcr.microsoft.com/v2/: read tcp 10.0.0.71:36034->40.71.10.214:443: read: connection reset by peer
2020-07-18T08:14:56.2385227Z ##[error]Docker pull failed with exit code 1
2020-07-18T08:14:56.2393829Z ##[section]Finishing: Initialize containers

@danmosemsft there was a brief network blip and DNS outage on Azure exactly around this time (same problem you may see in the partners mail about the auto scaler), so aside from us hardening the services we own around this not much we can do for MCR.

[GrabYourPitchforks]

How was this blocking you?

I changed the implementation of Dictionary<TKey, TValue>.GetObjectData to pass the correct Comparer through in the SerializationInfo object. Here's the original code, and as you can see when _comparer is a NonRandomizedStringEqualityComparer it uses SerializationInfo.SetType to masquerade as a different type.

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs

Lines 301 to 303 in a366d63

	info.AddValue(VersionName, _version);
	info.AddValue(ComparerName, _comparer ?? EqualityComparer<TKey>.Default, typeof(IEqualityComparer<TKey>));
	info.AddValue(HashSizeName, _buckets == null ? 0 : _buckets.Length); // This is the length of the bucket array

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/NonRandomizedStringEqualityComparer.cs

Lines 27 to 31 in a366d63

	public void GetObjectData(SerializationInfo info, StreamingContext context)
	{
	// We are doing this to stay compatible with .NET Framework.
	info.SetType(typeof(GenericEqualityComparer<string>));
	}

Here's the modified code:

https://github.com/GrabYourPitchforks/runtime/blob/2c5d91e0a00e53dd8d6bb25359faf14a0704cc35/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs#L325-L327

https://github.com/GrabYourPitchforks/runtime/blob/2c5d91e0a00e53dd8d6bb25359faf14a0704cc35/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs#L142-L155

https://github.com/GrabYourPitchforks/runtime/blob/2c5d91e0a00e53dd8d6bb25359faf14a0704cc35/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/IInternalStringEqualityComparer.cs#L23-L37

https://github.com/GrabYourPitchforks/runtime/blob/dict_perf_dev/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/NonRandomizedStringEqualityComparer.cs

In a nutshell, the code path above now actually serializes a GenericEqualityComparer<string> rather than using SerializationInfo.SetType. This passes in every test except the .resx test. The reason for this is that BinaryFormatter doesn't honor the call to SerializationInfo.SetType when asking the binder "please decompose this type name into strings". You can see this much more clearly from the sample app below.

using System;
using System.IO;
using System.Runtime.Serialization;
using System.Runtime.Serialization.Formatters.Binary;

namespace SampleFormatterApp
{
    class Program
    {
        static void Main(string[] args)
        {
            var ms = new MemoryStream();
            var bf = new BinaryFormatter();
            bf.Binder = new MyBinder();

            Console.WriteLine("Serializing Foo...");
            bf.Serialize(ms, new Foo());
            Console.WriteLine("Finished serializing Foo.");

            Console.WriteLine();
            Console.WriteLine("Serializing Bar...");
            bf.Serialize(ms, new Bar());
            Console.WriteLine("Finished serializing Bar.");
        }
    }

    [Serializable]
    class Foo : ISerializable
    {
        public void GetObjectData(SerializationInfo info, StreamingContext context)
        {
            info.SetType(typeof(Bar));
        }
    }

    [Serializable]
    class Bar
    {
    }

    class MyBinder : SerializationBinder
    {
        public override Type BindToType(string assemblyName, string typeName)
        {
            throw new NotImplementedException();
        }

        public override void BindToName(Type serializedType, out string assemblyName, out string typeName)
        {
            Console.WriteLine($"BindToName({serializedType.FullName}) called.");
            base.BindToName(serializedType, out assemblyName, out typeName);
        }
    }
}

Outputs:

Serializing Foo...
BindToName(SampleFormatterApp.Foo) called.   <--- !!!!!
Finished serializing Foo.

Serializing Bar...
BindToName(SampleFormatterApp.Bar) called.
Finished serializing Bar.

Now, to see why this matters, tweak the sample app a bit.

Type nonRandomizedEqCmpType = typeof(object).Assembly.GetType("System.Collections.Generic.NonRandomizedStringEqualityComparer");
Type genericEqCmpType = EqualityComparer<string>.Default.GetType();

var ms = new MemoryStream();
var bf = new BinaryFormatter();
bf.Binder = new MyBinder();

Console.WriteLine("Serializing NonRandomizedComparer...");
bf.Serialize(ms, Activator.CreateInstance(nonRandomizedEqCmpType, nonPublic: true));
Console.WriteLine("Finished serializing NonRandomizedComparer.");

Console.WriteLine();
Console.WriteLine("Serializing GEC<string>...");
bf.Serialize(ms, Activator.CreateInstance(genericEqCmpType, nonPublic: true));
Console.WriteLine("Finished serializing GEC<string>.");

Outputs:

Serializing NonRandomizedComparer...
BindToName(System.Collections.Generic.NonRandomizedStringEqualityComparer) called.
Finished serializing NonRandomizedComparer.

Serializing GEC<string>...
BindToName(System.Collections.Generic.GenericEqualityComparer`1[[System.String, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]) called.
Finished serializing GEC<string>.

The first case is the "before", and the second case is the "after". In the before case, the SerializationBinder never sees the generic type. It only ever sees "NonRandomized...", determines that there's no type mangling that needs to take place, and ignores everything. BinaryFormatter then merrily writes out the verbatim string "System.Collections.Generic.GenericEqualityComparer`1[[System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089]]" into the serialized payload. This is, after all, what was specified in the call to SetType.

In the after case, the SerializationBinder sees the real type, correctly deduces that it's a generic, and mangles the name. But now the output is different between the two cases, since the before case didn't mangle the name, and the after case did mangle the name.

Hence why this very specific unit test is failing on my changes.

[ericstj]

I think this test was just failing because it observed that you changed the binary format of Dictionary. We need to keep the piece of code that breaks the type/assembly name as that's the specific behavior that this test is ensuring we continue to support.